1496505 - mask-image CSS property does not load image bundled with extension

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Nicolás González-Deleito]

Attachment: mask-image.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

I develop an extension that displays a pie menu on a page for navigation purposes. The icons on that menu are shown using the mask-image CSS property, and are contained in a file bundled with that extension. Starting with Firefox (Developer Edition) 63.0b10 those icons are no longer loaded and displayed.

You can reproduce this by:
- temporarily installing the attached extension
- going to a page, e.g. https://developer.mozilla.org/en-US/
- clicking the left mouse button while pressing alt and shift

A small piece of HTML is injected at the top of the page. Up to version 63.0b9 two stars are visible. The red one (on top) is loaded via the mask-image property. The gray one (under the red one) via the background-image property.

Starting from version 63.0b10, the red one is no longer displayed, while the gray one is. On the console, a message of the type "Security Error: Content at https://developer.mozilla.org/en-US/ may not load or link to moz-extension://d54daaa3-3e9a-0f41-9113-674a4b23b7e8/star.svg." is shown. But, the image is bundled with the extension and background-image loads it anyhow.

Is this a bug or normal behavior? In the latter case, is there some explanation available on MDN about this change?

Thanks!

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8d4ed588ff4064d00d306438e95c295ba2d78743&tochange=a1c1aebbe0359707d9983e73bc297ca6df77c0ad

Regressed by: Bug 1418470

@Brad,

Your patch seems to cause the WebExt compatible issue, can you please look into this?

Comment 2

[Brad Werth [:bradwerth]]

Emilio, I'm not sure about this one. Should anonymous CORS block injected content from WebExtensions? It certainly IS content from another domain, but is this an area where we are supposed to carve out an exception? Or is there an alternative packaging for WebExtension developers that makes anonymous CORS fetches allowed?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I think it's reasonable for a WebExtension to load SVG masks.

But I don't know what the security model is supposed to be for those, and whether we should just avoid getting the moz-extension:// protocol checked by CORS or something more involved like passing the principal all the way through...

Cameron, Christoph, what's the best way to fix this? Should we avoid checking CORS for moz-extension resources? Or do something else?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

I haven't debugged that problem but looking at the provided console error suggests we are dealing with a same-origin policy violation and not a CORS error. (FWIW, CORS errors are prefixed with 'Cross-Origin Request Blocked'). To me it seems that the background image passes the right triggering principal but the mask-image property does not. How that problem is related to  Bug 1418470 I don't know at this point.

Kmag, what's your take on this one?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> I haven't debugged that problem but looking at the provided console error
> suggests we are dealing with a same-origin policy violation and not a CORS
> error. (FWIW, CORS errors are prefixed with 'Cross-Origin Request Blocked').
> To me it seems that the background image passes the right triggering
> principal but the mask-image property does not. How that problem is related
> to  Bug 1418470 I don't know at this point.

The difference between the properties is that background-image uses CORS_NONE but masks use CORS_ANONYMOUS (which is to say, the later uses LOAD_CORS_ANONYMOUS and the former does not).

Does that change your diagnostic?

Comment 6

[Daniel Veditz [:dveditz]]

This message sounds like the one we'd give if the resource is not content accessible, and I see nothing that says that star.svg should be accessible to web content. Maybe rules are different for extension-supplied CSS? On the face of it I think neither star should show up unless you've declared the resource as public.

thoughts kmag?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> The difference between the properties is that background-image uses
> CORS_NONE but masks use CORS_ANONYMOUS (which is to say, the later uses
> LOAD_CORS_ANONYMOUS and the former does not).
> 
> Does that change your diagnostic?

Nope, it does not. I still don't think this is a CORS issue but rather a problem with the right context, as explained in comment 4. Let's wait what other folks think, but I am pretty certain it's not a CORS issue.

Comment 8

[Kris Maglione [:kmag]]

Yeah, we should try to pass the correct triggering principal there

Comment 9

[Tomislav Jovanovic :zombie]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> This message sounds like the one we'd give if the resource is not content
> accessible, and I see nothing that says that star.svg should be accessible
> to web content. Maybe rules are different for extension-supplied CSS? On the
> face of it I think neither star should show up unless you've declared the
> resource as public.

Current situation is that the triggering principal determines if the resource should be loaded or not, and in case of CSS from a content script, the triggering principal is that same extension, so the load is allowed.  This is all as intended, though at some point in the future we may require any URL loaded in the content process to be listed in `web_accessible_resources` manifest field.

I don't understand why loading the mask is more restrictive (nor can see what bug 1418470 was about), but since this is the extension explicitly doing things, it should probably be allowed.


(In reply to Nicolás González-Deleito from comment #0)
> Is this a bug or normal behavior? In the latter case, is there some
> explanation available on MDN about this change?

As a workaround, and for best future compatibility, you could add the image to
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/web_accessible_resources

Comment 10

[Nicolás González-Deleito]

> As a workaround, and for best future compatibility, you could add the image
> to
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/
> manifest.json/web_accessible_resources

OK, thanks!

Comment 11

[Brad Werth [:bradwerth]]

Since including the content in the manifest makes it accessible via the anonymous CORS load, my feeling is that we should close this bug as INVALID.

Comment 12

[Sean Voisen (:svoisen)]

Agree with Brad. I'm going to close this since the behavior is as intended and inclusion in the manifest provides the right workaround. Though we should probably update the MDN documentation to be explicit about adding content to the manifest when loading masks (and anything else that might load CORS anonymous).

(In reply to Tomislav Jovanovic :zombie from comment #9)

> I don't understand why loading the mask is more restrictive (nor can see
> what bug 1418470 was about), but since this is the extension explicitly
> doing things, it should probably be allowed.

The spec for mask-image specifically says to use anonymous mode [1]. I'm not sure it's spec'ed as such for the other properties (?), so this may be a bit of inconsistency in the specs.

[1] https://www.w3.org/TR/css-masking-1/#security

Comment 13

[Tomislav Jovanovic :zombie]

(In reply to Sean Voisen (:svoisen) from comment #12)
> Agree with Brad. I'm going to close this since the behavior is as intended
> and inclusion in the manifest provides the right workaround.

Just a note that IMO this is not working as intended.  Listing the file in manifest has explicit security implications -- it's made readable to web content, and is not a good solution in all cases.

The semantics of an extension using an image as either background or the mask are the same from the security perspective, and the fact that we don't propagate the triggering principal here is a bug in our implementation detail, nobody "intended" it to behave that way.

You could still deem this low priority, or even outright WONTFIX it given the arguments, but closing with INVALID doesn't seem correct.

Comment 14

[Brad Werth [:bradwerth]]

(In reply to Tomislav Jovanovic :zombie from comment #13)
> The semantics of an extension using an image as either background or the
> mask are the same from the security perspective, and the fact that we don't
> propagate the triggering principal here is a bug in our implementation
> detail, nobody "intended" it to behave that way.

Good point. I'm happy to try to propagate the principal through. I'll replicate with the original testcase from comment 0 and see if I can fix the principal association.

Comment 15

[Sean Voisen (:svoisen)]

(In reply to Brad Werth [:bradwerth] from comment #14)

> Good point. I'm happy to try to propagate the principal through. I'll
> replicate with the original testcase from comment 0 and see if I can fix the
> principal association.

Thanks Brad! I just looked at the patches for bug 1418470 and now I see the new parse_with_cors_anonymous method, and didn't realize the principal wasn't propagated. I wrongly thought this was as intended.

Comment 16

[Brad Werth [:bradwerth]]

Attachment: Bug 1496505: Use the triggering principal rather than the loading principal for CORS checks.

Comment 17

[Brad Werth [:bradwerth]]

The patch changes CORS checks to the triggering principal instead of the loading principal, which makes this case work. But at what cost? I don't have the background to assert that this is correct/safe behavior.

Comment 18

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f12f79ca8eaf4297d4f3101ab7458b777b2c9de

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e79a4c8e2866
Use the triggering principal rather than the loading principal for CORS checks. r=ckerschb

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e79a4c8e2866

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Given comment 17, how would QA verify this is fixed? What do we expect the correct behavior to be?

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

After discussion with Ryan and reading related bugs, let's keep the possibility of fixing this in esr60 open.

Comment 23

[Sean Voisen (:svoisen)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Given comment 17, how would QA verify this is fixed? What do we expect the
> correct behavior to be?

I believe that question in comment 17 is basically answered since ckerschb reviewed and approved Brad's patch in which the change was made from propagating the loading principal to the triggering principal. ckerschb: Can you please confirm that this is correct/safe?

Given that, the correct behavior for QA to verify is that an extension should be able to load an image bundled in the extension using the CSS mask-image property. I think it would be a matter of either installing the extension mentioned in comment 1 and verifying the bug can no longer reproduce (assuming the author has not updated the extension yet with the provided manifest workaround), or creating a new extension that explicitly uses a CSS mask-image. Maybe folks more familiar with extensions can provide simpler QA verification steps than creating an entire extension?

Comment 24

[Sean Voisen (:svoisen)]

(In reply to Sean Voisen (:svoisen) from comment #23)
> I think it would be a matter of either installing the
> extension mentioned in comment 1 <snip>

Sorry, I mean the extension mentioned in the original bug summary.

Comment 25

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on the affected Nightly 64.0a1 (2018-10-04) (64-bit) with build ID: 20181004224156 on Windows 10 x64 using the extension and STR provided by the reporter.

On the latest Nightly 65.0a1 (2018-11-02) (64-bit) the issue can not be reproduced on Windows 7/10 x64, Mac OS 10.13 and Ubuntu 16.04, thus closing this issue as Verified - Fixed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta. Please nominate it for approval when you get a chance.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sean Voisen (:svoisen) [On parental leave Nov 15 - Jan 7] from comment #23)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> > Given comment 17, how would QA verify this is fixed? What do we expect the
> > correct behavior to be?
> 
> I believe that question in comment 17 is basically answered since ckerschb
> reviewed and approved Brad's patch in which the change was made from
> propagating the loading principal to the triggering principal. ckerschb: Can
> you please confirm that this is correct/safe?

I confirm. In the old days when we initially landed the CORS code we had no differentiation of loadingPrincipal and triggeringPrincipal. Please note that the TriggeringPrincipal represents the security context of what actually triggered the load and the loadingPrincipal represents the security context of where the resulting load will be used. In most cases the  triggeringPrincipal and the loadingPrincipal are identical, hence CORS checks work for the majority of cases, but the triggeringPrincipal is the one that should be used for doing the checks.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #26)
> This grafts cleanly to Beta. Please nominate it for approval when you get a
> chance.

Given bug 1503871 we should probably wait.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Bug 1418470 was backed out from 64, so this issue should be gone from there.