1499000 - WebExtensions fail security check when attempting to use browser resource SVG as a mask-image

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Martin Giger [:freaktechnik]]

It appears extensions can no longer load the container icons (from https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/contextualIdentities/ContextualIdentity iconUrl). Loads produce the following errors:

> Security Error: Content at moz-extension://2a4941c5-f2bf-4940-a383-66bbc5c07914/popup.html may not load or link to resource://usercontext-content/fingerprint.svg.
> Security Error: Content at moz-extension://2a4941c5-f2bf-4940-a383-66bbc5c07914/popup.html may not load or link to resource://usercontext-content/briefcase.svg.
> Security Error: Content at moz-extension://2a4941c5-f2bf-4940-a383-66bbc5c07914/popup.html may not load or link to resource://usercontext-content/dollar.svg.
> Security Error: Content at moz-extension://2a4941c5-f2bf-4940-a383-66bbc5c07914/popup.html may not load or link to resource://usercontext-content/cart.svg.

The extension I've observed this with is https://addons.mozilla.org/en-US/firefox/addon/close-container/

Comment 1

[Timea Cernea [:tbabos][inactive]]

Hi Martin,

I reproduced the issue on the latest Nightly 64.0a1 (2018-10-17) (64-bit) on Windows 10 x64.
Also run mozregression to see if it is caused by any landed patches and I found the following:

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

Note that I don't have access to view Bug 1418470 since it is a Secure issue.

Brad, can you please take a look at this?

Comment 2

[Brad Werth [:bradwerth]]

Very likely this is the same as Bug 1496505, and there is a fix detailed at https://bugzilla.mozilla.org/show_bug.cgi?id=1496505#c9. In short, I believe this is spec correct behavior where the loading of mask-image now uses CORS anonymous headers, and to allow the load you need to add the resources to the manifest as web_accessible_resources.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/web_accessible_resources

Comment 3

[Martin Giger [:freaktechnik]]

(In reply to Brad Werth [:bradwerth] from comment #2)
> Very likely this is the same as Bug 1496505, and there is a fix detailed at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1496505#c9. In short, I believe
> this is spec correct behavior where the loading of mask-image now uses CORS
> anonymous headers, and to allow the load you need to add the resources to
> the manifest as web_accessible_resources.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/
> manifest.json/web_accessible_resources

the problem is that these resources are built in to Firefox and are not part of the extension. The URL to them is returned by a Firefox API. WebExtensions can not (by default) register any resource:// URIs, everything they contain is in a moz-extension:// URI.

Comment 4

[Brad Werth [:bradwerth]]

> the problem is that these resources are built in to Firefox and are not part
> of the extension. The URL to them is returned by a Firefox API.
> WebExtensions can not (by default) register any resource:// URIs, everything
> they contain is in a moz-extension:// URI.

Got it. I just re-opened Bug 1496505 to look for a real fix for this.

Comment 5

[Martin Giger [:freaktechnik]]

FTR this wasn't fixed by bug 1496505

Comment 6

[Brad Werth [:bradwerth]]

(In reply to Martin Giger [:freaktechnik] from comment #5)
> FTR this wasn't fixed by bug 1496505

Okay, I'll look at this case separately and try to find a fix.

Comment 7

[Brad Werth [:bradwerth]]

Right. Well, it looks like the extension is loading the images as a mask-image, which now requires CORS checks. To accomplish the CORS check, the call chain goes through:

nsContentSecurityManager::doContentSecurityCheck
::DoCORSChecks
nsCORSListenerProxy::Init
nsCORSListenerProxy::UpdateChannel
nsScriptSecurityManager::CheckLoadURIWithPrincipal
nsScriptSecurityManager::CheckLoadURIFlags

Where we detect that the resource URL has the nsIProtocolHandler::URI_IS_UI_RESOURCE flag, and categorically deny access and report the error at https://searchfox.org/mozilla-central/rev/50ba1dd30cf013bddce1ae756f1b3c95b26f0628/caps/nsScriptSecurityManager.cpp#1025

So the potential fixes I see are:

1) Treat this as correct behavior -- it may be -- and update the WebExtension to change its use of mask-image to background-image, which does not require CORS checks and should be able to load a "resource" scheme URL.

2) Create an exception in nsScriptSecurityManager::CheckLoadURIFlags to allow a "moz-extension" URL to load a "resource" URL. Perhaps limited to certain types of resources? Not sure how sensitive our resources are and whether or not they should be exposed to WebExtensions in general. The comment in the nsScriptSecurityManager::CheckLoadURIFlags function seems to say this is bad. If it's safe to do this, one easy way to do accomplish it is to pass in the nsIScriptSecurityManager::ALLOW_CHROME flag in the call to CheckLoadURIWithPrincipal. That would keep the logic to determine if the CORS check should be chrome-like within nsCORSListenerProxy, which feels right.

Christoph, what do you recommend?

Comment 8

[Martin Giger [:freaktechnik]]

(In reply to Brad Werth [:bradwerth] from comment #7)
> 1) Treat this as correct behavior -- it may be -- and update the
> WebExtension to change its use of mask-image to background-image, which does
> not require CORS checks and should be able to load a "resource" scheme URL.

The reason the extension uses mask-image and not background-image is to easily apply a color to the icons, since the contextual identity icons come with a black fill by default for extensions. Using a background image would mean using a filter that is somehow generated from an arbitrary CSS color returned by the API.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Brad Werth [:bradwerth] from comment #7)
> Right. Well, it looks like the extension is loading the images as a
> mask-image, which now requires CORS checks. To accomplish the CORS check,
> the call chain goes through:
> 
> nsContentSecurityManager::doContentSecurityCheck
> ::DoCORSChecks
> nsCORSListenerProxy::Init
> nsCORSListenerProxy::UpdateChannel
> nsScriptSecurityManager::CheckLoadURIWithPrincipal
> nsScriptSecurityManager::CheckLoadURIFlags
> 
> Where we detect that the resource URL has the
> nsIProtocolHandler::URI_IS_UI_RESOURCE flag, and categorically deny access
> and report the error at
> https://searchfox.org/mozilla-central/rev/
> 50ba1dd30cf013bddce1ae756f1b3c95b26f0628/caps/nsScriptSecurityManager.
> cpp#1025
> 
> So the potential fixes I see are:
> 
> 1) Treat this as correct behavior -- it may be -- and update the
> WebExtension to change its use of mask-image to background-image, which does
> not require CORS checks and should be able to load a "resource" scheme URL.
> 
> 2) Create an exception in nsScriptSecurityManager::CheckLoadURIFlags to
> allow a "moz-extension" URL to load a "resource" URL. Perhaps limited to
> certain types of resources? Not sure how sensitive our resources are and
> whether or not they should be exposed to WebExtensions in general. The
> comment in the nsScriptSecurityManager::CheckLoadURIFlags function seems to
> say this is bad. If it's safe to do this, one easy way to do accomplish it
> is to pass in the nsIScriptSecurityManager::ALLOW_CHROME flag in the call to
> CheckLoadURIWithPrincipal. That would keep the logic to determine if the
> CORS check should be chrome-like within nsCORSListenerProxy, which feels
> right.
> 
> Christoph, what do you recommend?

Kmag, what would you recommend? Does option (1) sound like a plausible solution or do we need to fix up our CORS code to accomodate?

Comment 10

[Shane Caraveo (:mixedpuppy)]

> 2) Create an exception in nsScriptSecurityManager::CheckLoadURIFlags to allow a "moz-extension" URL to load a "resource" URL. 

This should not be done.

The way to handle this probably would be to have a new protocol.  That protocol can load from a specific location, and have appropriate flags set on the protocol.  Ideally it could somehow also work with favicon cache to handle bug 1315616.

Comment 11

[Kris Maglione [:kmag]]

I think option 2 might be an OK short term solution, for image loads by extension principals. I thought these resources were already content accessible, though, so I'm surprised it doesn't Just Work already.

Comment 12

[Brad Werth [:bradwerth]]

Since this isn't as simple as I had hoped, I would like to take myself off it and let someone in Platform take over.

Comment 13

[Sean Voisen (:svoisen)]

Thanks for your help on this Brad. Emilio is going to investigate and come up with a strategy on how to proceed (new protocol or otherwise).

Sounds like unless we do option 2, if we continue to load mask-image through CORS, then extensions that load this type of resource will have to be updated one way or another.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

So, here's my counter-proposal. Instead of doing something really complex, what about just threading from the CSS image loader somehow the ALLOW_CHROME flag of the nsIScriptSecurityManager?

That should be fairly simple (just some plumbing because we have a gazillion layers of abstraction).

Consequence is that content CSS images can load UI resources, but that's already the case for all loads that aren't CORS-enabled, see https://crisal.io/tmp/mask-image-resource.html, where I load a resource:// URI from a background-image for example, or from an <img> element.

So this effectively has the effect of allowing content to load resource URIs for shape-image (and mask-image in nightly). Which I think is fine.

WDYT Christoph, Kris?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)
> WDYT Christoph, Kris?

I personally can live with such a workaround/hot fix. I am not entirely sure that approach works though. You could easily try it by adding SEC_ALLOW_CHROME to the loadinfo of the channel and then the ContentSecurityManager would do the rest automagically for you. Take a look at the styleLoader which does a similar thing:
  https://searchfox.org/mozilla-central/source/layout/style/Loader.cpp#1487

Comment 16

[ysard]

Hi, is there anything new about this problem, or a way to work around it, other than embed the colored icons in png format?

As an addon developer (cookie-quick-manager), this has been a problem for some versions of Firefox since it makes it difficult to manage containers, and raises alarming and unjustified security errors.

Comment 17

[Valdo]

I regret that Bugzilla doesn't automatically raise bugs' priority after some time (say 6 months?) or that Mozilla doesn't plan pauses in Firefox development (say once a year?) so the backlog gets deserved attention from time to time (including lower priority bugs) and everyone's involvement to reduce it. Without a similar organization, the sad fact is some bugs never get fixed... (this particular one is probably not the best example - "only" 10 months old, there are much more severe examples in terms of oldness, always treated as low priority and never solved)
Just my thoughts and humble proposals. But maybe reality is much more complex...

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Just took another look at this, fwiw. I don't think the SEC_ALLOW_CHROME thing works, since it's loaded from content.

Also it seems we already pass that flag for images anyway: https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/image/imgLoader.cpp#820

Though it seems like we should be hitting this:

https://searchfox.org/mozilla-central/rev/50ba1dd30cf013bddce1ae756f1b3c95b26f0628/caps/nsScriptSecurityManager.cpp#1006

And the images are marked as content-accessible: https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/browser/components/contextualidentity/jar.mn#8

So I'm unsure that the diagnostic in comment 7 is 100% correct... Since if my reading of the code is correct the check should be passing.

Sorry this fell down to the cracks. I'll take a closer look to see what's really going on here...

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Sorry, that ni? was supposed to go for me so I don't forget :)

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1499000 - Make CORS use the right security flags to check whether an URL can be loaded. r=ckerschb

This fixes at least part of the problem. Without this patch, some of the flags
may get lost, like the ALLOW_CHROME flag which controls whether stuff like
resource:// URLs can be loaded or not.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

So with the patch above we get further, but still not quite.

With that patch we end up erroring a bit later here

Christoph, would it be a good approach for this to do something like adding another flag to LoadInfo to bypass CORS if not http?

Or would you prefer something more explicit like what we do to allow fonts from file:// URIs?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

We already have a flag 'bypassCors'. Probably you could move the check before the |if (!http) { ...}|-check which should hopefully fix your problem, see:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#561

Comment 23

[Emilio Cobos Álvarez (:emilio)]

It doesn't, because I don't want to bypass CORS unconditionally. I want to do it if it's not an http channel.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1499000 - Don't do CORS checks on CSS images with the resource:// scheme.

CORS only works on http channels, so anything else that tries to do a
CORS-enabled request fails catastrophically.

resource:// images are useful for extension developers, so don't perform CORS
checks on them. We may want to also do file:// and fix bug 1565509, while at it,
if we consider it's causing developer pain.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

I wrote another patch on the lines of the second approach in comment 21 + a test... But it is somewhat weird that CORS on non-HTTP channels just bails completely.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Per discussion the first patch is worth landing on isolation, regardless of whether we go with the approach in comment 24 or a more generic approach (see the phabricator discussion for more details).

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7aace209324f
Make CORS use the right security flags to check whether an URL can be loaded. r=ckerschb

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7aace209324f

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebfc26247087
Don't do CORS checks on CSS images with the resource:// scheme. r=bzbarsky

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7c19d82c5a5
followup: Annotate the test as failing on Android as the image it uses is not available.

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ebfc26247087
https://hg.mozilla.org/mozilla-central/rev/e7c19d82c5a5

Comment 32

[Ryan VanderMeulen [:RyanVM]]

This looks kinda scary to backport - what are your thoughts, Emilio?

Comment 33

[Emilio Cobos Álvarez (:emilio)]

I don't think it's scary to backport D40651 if we wanted to. The other patch I agree is slightly more scary, but is not needed to fix the bug.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9083038 [details]
Bug 1499000 - Don't do CORS checks on CSS images with the resource:// scheme.

Beta/Release Uplift Approval Request

• User impact if declined: comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just an "if scheme is resource://, then don't do CORS" which returns to our previous behavior with these images.
• String changes made/needed: none

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9083038 [details]
Bug 1499000 - Don't do CORS checks on CSS images with the resource:// scheme.

Fixes CORS issues with some extensions by reverting us to prior behavior. Approved for 69.0b14 and 68.1esr.

Comment 36

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bab19dbd7043

Comment 37

[Bogdan Tara[:bogdan_tara | bogdant]]

I tried to uplift to esr68 but got conflicts:
grafting 559526:ebfc26247087 "Bug 1499000 - Don't do CORS checks on CSS images with the resource:// scheme. r=bzbarsky"
merging layout/reftests/svg/reftest.list
merging layout/style/ImageLoader.cpp
merging layout/style/ServoStyleConstsInlines.h
merging servo/ports/geckolib/cbindgen.toml
warning: conflicts while merging layout/reftests/svg/reftest.list! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/style/ImageLoader.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/style/ServoStyleConstsInlines.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging servo/ports/geckolib/cbindgen.toml! (edit, then use 'hg resolve --mark')

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/019937195b19

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please provide patches for ESR68.

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Attachment: ESR patch

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/8163e4c59a10

Comment 42

[Miruna Curtean]

Hello,

If this requires no manual testing, as seen in the Beta/Release Uplift Approval Request comment, could the "qe-" flag be added?
If manual testing is in fact required please set the "qe+" flag and provide some steps to reproduce this issue.
Thank you

Comment 43

[Emilio Cobos Álvarez (:emilio)]

STR:

• Load https://crisal.io/tmp/mask-image-resource.html
• Ensure the following renders: One big black fingerprint on red background, then two squares with small red fingerprints on white backgrounds, then one with small black fingerprints on red background.

Comment 44

[Miruna Curtean]

Verified fix successfully on Windows 10 64-bit and Ubuntu 18.04.0 LTS on Fx 70.0a1 (20190821215524) and 69.0b15 (20190819184224).
For Firefox ESR, I could not check, as the build from comment 41 could not be retrieved.

Comment 45

[Emilio Cobos Álvarez (:emilio)]

Any later build from here for example should do, right? https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&revision=d574e9e14ca210c752f005fb2b55af3f79a03c9d

Comment 46

[Miruna Curtean]

It worked, thank you. I've managed to install and test on Windows 10 -64 bit with Fx 68.1.0esr (20190821232631) and all the squares are rendered along with the fingerprints and the colors. I'm setting this as Verified.