928790 - chrome-protocol (but not chrome-privileged) SVG-as-an-image should be able to load other chrome resources

Status: RESOLVED WONTFIX

(Core :: SVG, defect)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #628747 +++

Bug 628747 prevented SVG-as-an-image from loading external resources in many cases due to privacy concerns but this also blocked chrome SVG-as-an-image from loading a stylesheet in the same directory which seems unnecessary as it seems like the concerns from bug 628747 wouldn't apply as third-party content shouldn't be referenced by a chrome URI in the first place.

Security Error: Content at chrome://browser/skin/tabbrowser/tab-selected-start.svg may not load data from chrome://browser/skin/tabbrowser/tab-selected-svg.css.

The ability to share a single stylesheet between multiple (about 3-8) SVG files will reduce duplication and improve maintainability.

If special-casing this scenario is deemed not worthwhile, I'd like to know ASAP so we know how to proceed with Australis tabs.

It seems like changing this would require adjusting nsDataDocumentContentPolicy.cpp[1] and possibly CHECK_PRINCIPAL_AND_DATA[2]

[1] https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDataDocumentContentPolicy.cpp?rev=f2c7e4e24bea#75
[2] https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h?rev=f2c7e4e24bea#149

Comment 1

[Masatoshi Kimura [:emk]]

Loading between chrome:// URLs should not be rejected because the chrome: scheme is flagged as URI_IS_LOCAL_RESOURCE.
https://mxr.mozilla.org/mozilla-central/ident?i=URI_IS_LOCAL_RESOURCE

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> Loading between chrome:// URLs should not be rejected because the chrome:
> scheme is flagged as URI_IS_LOCAL_RESOURCE.

Per a code-comment in [1] above, URI_IS_LOCAL_RESOURCE is not enough -- we have an additional requirements which chrome URLs don't satisfy (the "and also" part of the comment).

I'll quote that comment here for clarification:
> 76     // We only allow SVG images to load content from URIs that are local and
> 77     // also satisfy one of the following conditions:
> 78     //  - URI inherits security context, e.g. data URIs
> 79     //   OR
> 80     //  - URI loadable by subsumers, e.g. blob URIs

Chrome URIs are neither INHERITS_SECURITY_CONTEXT nor LOADABLE_BY_SUBSUMERS, so they can't be loaded in SVG images.

It's possible we could get around this by just adding URI_IS_UI_RESOURCE to the list of allowed flags, though. (And presumably we have checks elsewhere that prevent random web-hosted content from loading chrome:// URIs, so this wouldn't be opening us up to anything.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: strawman patch 1: Don't block URI_IS_UI_RESOURCE URIs in SVG-as-an-image [introduces a security hole, as long as remote content can include chrome:// images]

Matt, does this fix things?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 819622 [details] [diff] [review]
strawman patch 1: Don't block URI_IS_UI_RESOURCE URIs in SVG-as-an-image [introduces a security hole, as long as remote content can include chrome:// images]

Yes it does, thanks for the quick response!

In this case I think checking aRequestOrigin would be a good idea (assuming it's non-null in this case) because I think who is requesting the load (either chrome or not) is more important than what they are requesting in this case. The URI_IS_UI_RESOURCE description[1] is a bit worrying with regards to the current patch:
> There are cases when such resources may be made accessible to untrusted 
> content such as web pages, so this is less restrictive than 
> URI_DANGEROUS_TO_LOAD but more restrictive than URI_LOADABLE_BY_ANYONE.  

Also, I don't even know that we need to limit what chrome URIs can load as I didn't think chrome documents have to follow a same-origin policy. This is good enough for my specific use-case so I can understand if we don't want to take unnecessary risk.

Thanks again

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProtocolHandler.idl#180

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 819622 [details] [diff] [review]
strawman patch 1: Don't block URI_IS_UI_RESOURCE URIs in SVG-as-an-image [introduces a security hole, as long as remote content can include chrome:// images]

Actually, turns out this is very bad from a security perspective.

I assumed that we already prevented content on the web from loading chrome:// URIs, but it turns out we don't. (at least, not for <img>)

e.g. <img src="chrome://branding/content/icon32.png"> works just fine on the web. This is fine, because no data is currently leaked, because the author can't tell what the image is displaying.

*BUT* if we give them the ability to render it in an SVG image (as this patch does) and then draw that SVG image to a canvas (which they can do), *then* they'll be able to read the pixels, and I don't think we want to enable that.

So: We definitely don't want this patch. :)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Matthew N. [:MattN] from comment #0)
> If special-casing this scenario is deemed not worthwhile, I'd like to know
> ASAP so we know how to proceed with Australis tabs.

I talked with jrmuizel, and we are both leaning towards this "not worthwhile" opinion.  It'd be better to keep SVG images atomic. (And presumably you can avoid code-duplication via build-time post-processing or something like that?)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: demo showing that web content can display chrome:// images

[For anyone else who (like me) was unsure about whether we allow web content to pull in chrome:// images, here's a demo.]

Comment 8

[Matthew N. [:MattN]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> I assumed that we already prevented content on the web from loading
> chrome:// URIs, but it turns out we don't. (at least, not for <img>)

OK, I thought there were things like this that were allowed (hence my concern) but I thought it was just for resource://…. I'm glad that you double-checked this.

(In reply to Daniel Holbert [:dholbert] from comment #6)
> (In reply to Matthew N. [:MattN] from comment #0)
> > If special-casing this scenario is deemed not worthwhile, I'd like to know
> > ASAP so we know how to proceed with Australis tabs.
> 
> I talked with jrmuizel, and we are both leaning towards this "not
> worthwhile" opinion.  It'd be better to keep SVG images atomic. (And
> presumably you can avoid code-duplication via build-time post-processing or
> something like that?)

What did you think about my idea of special-casing based on aRequestOrigin? It seems that if the scheme is "chrome" then we could allow the load. (We do this in the gecko in many places). That is what I did locally for development and it seems safer. I will be attaching a patch in bug 921038 within a few minutes that takes advantage of this feature request btw.

Yes, build-time pre-processing can reduce some duplication for developers but it will still lead to duplication in the obj-dir and in shipped packages (albeit the files are not large in this case). It's already a complicated pre-processing setup so I'd rather not complicate it more and at a minimum there will be 8 stub files (with %include's) required if this isn't fixed.

RE: atomicity, if I understand you correctly, don't the exceptions in the code already break that?

Overall, it feels like the restriction from bug 628747 goes against the general case that chrome-privileged code can bypass most security checks.

Thanks

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we just stop letting content load chrome URI images?

Comment 10

[Masatoshi Kimura [:emk]]

:bz dislikes the idea of hard-coding the scheme rather than using protocol flags.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Matthew N. [:MattN] from comment #8)
> What did you think about my idea of special-casing based on aRequestOrigin?

I suspect it would work. Jeff said something to me like "Chrome-only special cases are just security vulnerabilities waiting to happen", though, which I kind of agree with...

> It seems that if the scheme is "chrome" then we could allow the load. (We do
> this in the gecko in many places).

(I don't know enough about chrome URIs or about those other instances to comment on this, but if it's a semi-established practice, it might be OK here?)

> Yes, build-time pre-processing can reduce some duplication for developers
> but it will still lead to duplication in the obj-dir and in shipped packages
> (albeit the files are not large in this case).

Right. This is a tradeoff between duplicating some CSS vs. loosening a security check in a special case, and I'm not sure the latter option is worth it, given its security-jitters and platform-code-complexity penalty.

> RE: atomicity, if I understand you correctly, don't the exceptions in the
> code already break that?

Not really. A data URI's content is included inline in the SVG (albeit in an encoded way), and blob URI is sort of the same but more compact (and really only for dynamically-generated SVG images).

(In reply to Masatoshi Kimura [:emk] from comment #10)
> :bz dislikes the idea of hard-coding the scheme rather than using protocol
> flags.

That would favor khuey's suggestion [combined with my strawman patch], then, I suppose.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Wait.  Why is a chrome:// SVG image even ending up in content policy code.  I thought we short-circuited all content policy checks for the system principal.  Do these images not have the system principal (e.g. they're not in the "content" package)?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Or to put comment 12 another way, if the images are "chrome-privileged" then why are they ending up in this code, and if they're not why are we talking about "chrome-privileged" stuff in this bug at all?

Comment 14

[Matthew N. [:MattN]]

(In reply to On vacation Oct 12 - Oct 27 from comment #12)
> Wait.  Why is a chrome:// SVG image even ending up in content policy code. 
> I thought we short-circuited all content policy checks for the system
> principal.  Do these images not have the system principal (e.g. they're not
> in the "content" package)?

I thought the same thing which is why I filed the bug and was surprised to see the console error in comment 0.

(In reply to :bz from comment #13)
> Or to put comment 12 another way, if the images are "chrome-privileged" then
> why are they ending up in this code, and if they're not why are we talking
> about "chrome-privileged" stuff in this bug at all?

My assumption is that the chrome:// scheme automatically gives the resources "chrome privileges". Let me know if that isn't the case. You can see the code that needs this in bug 921038 attachment 819738 [details] [diff] [review] (tabs.inc.css). If you run the browser with that but without a patch for this bug, you will see the fill for the left tab curve will be the wrong color. It's using the following on a ::before pseudo-element on a tabs:
content: url(chrome://browser/skin/tabbrowser/tab-selected-start.svg);

Comment 15

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #14)
> [review] (tabs.inc.css). If you run the browser with that but without a
> patch for this bug, you will see the fill for the left tab curve will be the
> wrong color. It's using the following on a ::before pseudo-element on a tabs:
> content: url(chrome://browser/skin/tabbrowser/tab-selected-start.svg);

…and I forgot to mention that the rule with the "content" property gets pre-processed into chrome://browser/skin/browser.css which is used as an XML stylesheet from chrome://browser/content/browser.xul so all the URIs have a chrome scheme.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Let me know if that isn't the case.

It's not the case.  See comment 12.

> chrome://browser/skin/tabbrowser/tab-selected-start.svg

That's not in a content package, so doesn't have "chrome privileges".

Comment 17

[Matthew N. [:MattN]]

(In reply to On vacation Oct 12 - Oct 27 from comment #16)
> > Let me know if that isn't the case.
> 
> It's not the case.  See comment 12.
> 
> > chrome://browser/skin/tabbrowser/tab-selected-start.svg
> 
> That's not in a content package, so doesn't have "chrome privileges".

Sorry, I didn't read the mention of the "content package" closely (it's late here). I had never heard about this distinction in regards to security before. In this case I should be able to move these 2 files to the content package as they arguably belong there. If that works out then we can WONTFIX this bug unless someone thinks there is something worth fixing.

Comment 18

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #17)
> If that works out…

and of course it wouldn't be that easy. Presumably it's not working because the stylesheet reference doesn't belong in the content package but I suspect the skin/theme stylesheet isn't allowed to reference the content-packaged SVG file. There's no warning in the console this time and I'm heading to bed now. 

I think I can make the pre-processing approach a little less messy at the cost of making it a little messier for extensions to override the styles.

Thanks everyone for your quick responses!

Comment 19

[Daniel Holbert [:dholbert]]

So to sum up, IIUC, we're behaving as-expected here.  In particular, no chrome privileges --> no relaxations on security, per comment 16. And it sounds like MattN's got a pre-processing strategy in the works and isn't depending on this feature-request at this point.

So, absent any new information, I think this is WONTFIX.

Comment 20

[Matthew N. [:MattN]]

Yep, that's right. I was also going to WONTFIX this after attaching a new patch in bug 921038 in a little bit.

Comment 21

[Daniel Holbert [:dholbert]]

Cool. Removing Australis whiteboard flags, to keep this from coming up in "needed for australis"-type searches, then.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #18)
> the skin/theme stylesheet isn't allowed to reference the content-packaged
> SVG file.

That shouldn't be a problem, unless the stylesheet is being loaded in a non-chrome-privileged document (which I don't think is the case).