693940 - Check URI_INHERITS_SECURITY_CONTEXT or URI_LOADABLE_BY_SUBSUMERS (along with URI_IS_LOCAL_RESOURCE) to selectively allow data-URI-encoded resources inside of SVG images

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Bug 628747's patch prevented loading of external resources from SVG when it's being used as an image:
  http://hg.mozilla.org/mozilla-central/rev/21f96c423923
but it checks for URI_IS_LOCAL_RESOURCE, which isn't precise enough -- it allows SVG images on your local filesystem to load other local files, potentially causing data leakage.

As I described in bug 686013 comment 20, this isn't really a privacy issue right now since SVG images taint canvases, but it could be a problem if we change that canvas behavior, in bug 672013.  Our behavior is also counterintuitive ("This image worked fine when I viewed it locally, but it doesn't work when I put it on a web server!").

I think we should check for URI_INHERITS_SECURITY_CONTEXT in nsDataDocumentContentPolicy::ShouldLoad, instead of IS_LOCAL_RESOURCE.  IIUC, this would *actually* prevent all external file loads, including on the local file system.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> I think we should check for URI_INHERITS_SECURITY_CONTEXT in
> nsDataDocumentContentPolicy::ShouldLoad, instead of IS_LOCAL_RESOURCE. 

Or perhaps we should check *both* IS_LOCAL_RESOURCE and INHERITS_SECURITY_CONTEXT.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix

This checks both flags, per previous comment.

I verified that it "breaks" the reftest svg-image-external-1.html (i.e. it keeps us from loading file:// URLs from inside an SVG image).

Testing more thoroughly.  Requesting r?bz now though, in the hopes that we can land this and a simplified version of bug 672013's patch before the migration tomorrow.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Added a commit message, and noted the change in reftest behavior. (since the external resource load will now fail)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Or perhaps we should check *both* IS_LOCAL_RESOURCE and INHERITS_SECURITY_CONTEXT.

Hmm.  You mean check that both flags are set?  That would make some sense, yeah....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 572551 [details] [diff] [review]
fix v1

r=me

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Hmm.  You mean check that both flags are set?  That would make some sense,
> yeah....

Yup.  Justification for using both flags:

  - URI_IS_LOCAL_RESOURCE is important because we don't want to allow "phoning home" from inside an image, so we need to enforce that the resources are local. (see scenarios in bug 628747 comment 0)

  - URI_INHERITS_SECURITY_CONTEXT is important because we don't want to allow SVG images to pull in content that has an inherent security context (e.g. a file:// URL) and then read that data via canvas. (after we've relaxed SVG images' canvas-tainting)

Thanks for the r+!

Comment 7

[Daniel Holbert [:dholbert]]

Pushed to Try for full test-run on Linux & Mac:
  https://tbpl.mozilla.org/?tree=Try&rev=8c9f8c4ac738

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1a [r=bz]

Added one more reftest.list-tweak from roc's patch in bug 672013, for a different local-external-resource-in-SVG-as-an-image test. (Hadn't previously noticed this test's behavior changing, because it was marked as random.)

I'm adding this reftest.list tweak as a part of the patch here since this patch will change that test's behavior.

New try push with updated patch:
  https://tbpl.mozilla.org/?tree=Try&rev=2840dd00fbbed

Comment 9

[Daniel Holbert [:dholbert]]

We should document general external resource usage in SVG-as-an-image in MDN.

I'm planning on doing that -- I'll write up an MDN page for it soon.  Adding "dev-doc-needed" keyword to remind myself.

Comment 10

[Daniel Holbert [:dholbert]]

I documented this a bit here:
  https://developer.mozilla.org/en/SVG/SVG_as_an_Image

I should probably add a clarification about this bug's behavior-change. (right now I've just documented the new behavior.)

Comment 11

[Daniel Holbert [:dholbert]]

This has nearly completed a fully-green try run on Linux Opt, so I'm confident enough to push.
(I'll still watch the remaining Try jobs & back out if anything unexpected happens.)

Landed on inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/10814cd743e3

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Currently people are using blob URIs (moz-filedata) in SVG images to do thing dynamically. Will this plus bug 672013 break that?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it will, and I think we shouldn't do that. Blob URIs should be just fine in SVG images. We need to do something differently here. We probably should back this out of inbound/Aurora.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yes.  We should probably check for LOCAL_RESOURCE and either one of LOADABLE_BY_SUBSUMERS or INHERITS_PRINCIPAL.

Comment 15

[Jesse Ruderman]

Does restricting to LOCAL_RESOURCE actually prevent phoning home? IIRC, file:///// URLs can refer to servers or other computers on a LAN.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10814cd743e3

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Jesse Ruderman from comment #15)
> Does restricting to LOCAL_RESOURCE actually prevent phoning home? IIRC,
> file:///// URLs can refer to servers or other computers on a LAN.

That may be, but we want to block file:// URIs anyway (that's the point of this bug), so this won't be an issue (at least not for file:// URIs in particular.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> I think it will, and I think we shouldn't do that. Blob URIs should be just
> fine in SVG images. We need to do something differently here. We probably
> should back this out of inbound/Aurora.

OK -- I'll back this out of m-c / aurora later today, and then re-land a better patch just on m-c.

(In reply to Boris Zbarsky (:bz) from comment #14)
> Hmm.  Yes.  We should probably check for LOCAL_RESOURCE and either one of
> LOADABLE_BY_SUBSUMERS or INHERITS_PRINCIPAL.

OK. (By INHERITS_PRINCIPAL I assume you mean INHERITS_SECURITY_CONTEXT?)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes, that is indeed what I mean.

Comment 20

[Daniel Holbert [:dholbert]]

Backed out from m-c:
 https://hg.mozilla.org/mozilla-central/rev/0b79700b033c

Looks like the m-c --> aurora merge hasn't happened yet, so this didn't make it to aurora yet.  When that merge happens, I'll back it out there too (unless the merged m-c cset is chosen such that it's unnecessary).

Comment 21

[Daniel Holbert [:dholbert]]

Backed out from aurora:
 http://hg.mozilla.org/releases/mozilla-aurora/rev/288a354d72da

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: fix v2a

OK, this patch fixes this to also let LOADABLE_BY_SUBSUMERS URIs through, as suggested in comment 14.

I included two reftests with moz-filedata URIs:
 - The first one doesn't directly test this patch, but rather tests that we successfully render SVG-as-an-image encoded *as* a moz-filedata.
 - The second one *does* directly test this patch -- it checks that we successfully render moz-filedata URIs *inside of* svg-as-an-image.

Both reftests pass in current trunk (which is expected, since this bug is about imposing additional restrictions), and they also pass when this patch is applied. The second reftest fails with the earlier "fix v1a" applied (because that earlier fix broke moz-filedata URIs).

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: fix v2b

(fixed comment typo -- removed erroneous "-" prefix from "-moz-filedata")

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 574113 [details] [diff] [review]
fix v2b

r=me

Comment 25

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2da841d96925

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2da841d96925

Comment 27

[Janet Swisher]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> I documented this a bit here:
>   https://developer.mozilla.org/en/SVG/SVG_as_an_Image
> 
> I should probably add a clarification about this bug's behavior-change.
> (right now I've just documented the new behavior.)

Are you still planning to add more to this page? Do you need/want help with that? 

Do you know about how this is handled/supported in other browsers?

Feel free to write up random notes here or in email, and I can make them more presentable.

Comment 28

[Daniel Holbert [:dholbert]]

> Are you still planning to add more to this page? Do you need/want help with that? 
Thanks for following up -- I think that page is complete as far as I'm concerned, actually.

The main thing I had in mind was to add was that -moz-filedata (BlobBuilder-generated) URIs are allowed -- but Eli already added that (thanks Eli!), so that's covered.

I think at one point I was also considering adding something along the lines of:
  "Prior to Firefox 11, we allowed SVG images served over "file://" to load other local
   (e.g. other images off of the local filesystem). In Gecko 11, this special
   ability for locally-stored SVG images has been removed."

At this point, though, I don't think that's worth documenting in MDN.  Since it was an (unintentional) file://-only allowance, I doubt it's something that developers would encounter / use in actual live *web* content.  (If anyone disagrees, feel free to add a note along the lines of my paragraph above, though.)

> Do you know about how this is handled/supported in other browsers?

I seem to recall Opera allowing remotely-hosted resources vs. chromium not allowing any external resources at all (including via data: URIs), but I'm not sure about that.

In any case -- removing "dev-doc-needed" keyword, since the SVG_as_an_Image page has the main content that I was interested in documenting.

Comment 29

[Janet Swisher]

Found data for other browsers at http://caniuse.com/svg-css and http://caniuse.com/svg-img

The dev-doc-complete keyword is used for tracking bugs whose needed doc work is now done.

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to Janet Swisher from comment #29)
> Found data for other browsers at http://caniuse.com/svg-css and
> http://caniuse.com/svg-img

Ah, right -- sorry, I think I was answering a different question from what you were actually asking in my response on this in the previous comment. :)

Thanks for the doc-updating!

Comment 31

[ytrezq]

(In reply to Daniel Holbert [:dholbert] from comment #22)
Sorry, something which I don’t understand : why you don’t restrict the security context of URI_LOADABLE_BY_SUBSUMERS ?

Comment 32

[Daniel Holbert [:dholbert]]

I'm not sure I understand your question; can you clarify?

(Are you talking about the specific piece of code that was changed in this patch, or about Gecko in general? I'd also have to read up a bit on URI_LOADABLE_BY_SUBSUMERS again to answer knowledgeably, once I understand the question.)

Comment 33

[Daniel Holbert [:dholbert]]

(I'm pretty sure there's code elsewhere that actually enforces the implied "only" aspect of URI_LOADABLE_BY_SUBSUMERS.  The code in this bug's patch isn't about enforcing that.  Rather, this bug's patch is about doing an early screening to block *other* types of URLs, and allowing only certain types of URLs to proceed & be screened with further standard security checks.)

Comment 34

[Robert Longson [:longsonr]]

ytrezq it would be far better if you explained what you're trying to achieve/understand via the newsgroup: https://groups.google.com/forum/#!forum/mozilla.dev.tech.svg rather than adding cryptic comments to old closed bugs.

Comment 35

[Daniel Holbert [:dholbert]]

(Agreed; thanks Robert.)

Comment 36

[ytrezq]

(In reply to Daniel Holbert [:dholbert] from comment #32) (last comment here)
Yes about the patch.
Why    if (!HasFlags(aContentLocation,
                  nsIProtocolHandler::URI_IS_LOCAL_RESOURCE) ||
        (!HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT) &&
         !HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS))) {

instead of
    if (!HasFlags(aContentLocation,
                  nsIProtocolHandler::URI_IS_LOCAL_RESOURCE) ||
        (!HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT) ||
         !HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS))) {

Comment 37

[Daniel Holbert [:dholbert]]

See the comment in the patch:
    // We only allow SVG images to load content from URIs that are local and
    // also satisfy one of the following conditions:
    //  - URI inherits security context, e.g. data URIs
    //   OR
    //  - URI loadable by subsumers, e.g. moz-filedata URIs

The actual logic in the code is inverted (so there's a "!" on everything, and there's an "&&" instead of "||", and we say REJECT if "if"-check passes.)  But it's equivalent to the comment that I quoted above.

(If we used || instead of &&, as you suggest, then we'd be rejecting all URIs, unless they happen to be both URI_INHERITS_SECURITY_CONTEXT and URI_LOADABLE_BY_SUBSUMERS. This would, for example, make us reject both blob URIs and data URIs, since each of those only have one of these two flags.  But really we want to allow each of those types of URIs.)

Comment 38

[ytrezq]

(In reply to Daniel Holbert [:dholbert] from comment #37)
Yes sorry I was meaning
    if ((!HasFlags(aContentLocation,
                  nsIProtocolHandler::URI_IS_LOCAL_RESOURCE) &&
        (!HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS)) ||
         !HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT))) {

Comment 39

[Daniel Holbert [:dholbert]]

That first operator is "||", not "&&", because it's the inverse of the comment above it. (The comment describes what we allow; the actual check is for what we reject.) And inverse'ing flips operators -- it converts "||" to "&&". So the " and also satisfies..." in the comment becomes "||" in the actual code as a result.

(Perhaps this could be made more clear by pulling the "!" out of the conditions that it's more obvious that the code matches the comment.)

In any case -- if you have further questions, please post them on dev.tech.svg rather than here, as Robert suggested. I'm marking 36 through this comment as "offtopic" so that they'll autohide and won't distract as much from the main discussion/work in this bug.)

Comment 40

[ytrezq]

Yes sorry again I was really meaning
instead of
    if ((!HasFlags(aContentLocation,
                  nsIProtocolHandler::URI_IS_LOCAL_RESOURCE) ||
        (!HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT)) &&
        ((!HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_LOADABLE_BY_SUBSUMERS)) ||
         !HasFlags(aContentLocation,
                   nsIProtocolHandler::URI_INHERITS_SECURITY_CONTEXT)))) {

Comment 41

[Daniel Holbert [:dholbert]]

...at first glance, I believe comment 40 is equivalent to the logic you quoted in comment 38, but just more verbose.

Please stop posting new comments on this bug, and please instead post to dev.tech.svg. More info on how to post/subscribe to that list here:
 https://lists.mozilla.org/listinfo/dev-tech-svg