Closed Bug 693940 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → 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.
Blocks: 672013
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #572540 - Flags: review?(bzbarsky)
Attached patch fix v1 (obsolete) — Splinter Review
Added a commit message, and noted the change in reftest behavior. (since the external resource load will now fail)
Attachment #572540 - Attachment is obsolete: true
Attachment #572540 - Flags: review?(bzbarsky)
Attachment #572551 - Flags: review?(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 on attachment 572551 [details] [diff] [review]
fix v1

r=me
Attachment #572551 - Flags: review?(bzbarsky) → review+
(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+!
Pushed to Try for full test-run on Linux & Mac:
  https://tbpl.mozilla.org/?tree=Try&rev=8c9f8c4ac738
Flags: in-testsuite?
Attached patch fix v1a [r=bz] (obsolete) — Splinter Review
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
Attachment #572551 - Attachment is obsolete: true
Attachment #572565 - Flags: review+
Summary: Use URI_INHERITS_SECURITY_CONTEXT instead of URI_IS_LOCAL_RESOURCE to selectively allow data-URI-encoded resources inside of SVG images → Use URI_INHERITS_SECURITY_CONTEXT along with URI_IS_LOCAL_RESOURCE to selectively allow data-URI-encoded resources inside of SVG images
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.
Keywords: dev-doc-needed
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.)
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
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla10
Currently people are using blob URIs (moz-filedata) in SVG images to do thing dynamically. Will this plus bug 672013 break that?
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.
Hmm.  Yes.  We should probably check for LOCAL_RESOURCE and either one of LOADABLE_BY_SUBSUMERS or INHERITS_PRINCIPAL.
Does restricting to LOCAL_RESOURCE actually prevent phoning home? IIRC, file:///// URLs can refer to servers or other computers on a LAN.
https://hg.mozilla.org/mozilla-central/rev/10814cd743e3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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.
(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?)
Yes, that is indeed what I mean.
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-testsuite+ → in-testsuite?
Summary: Use URI_INHERITS_SECURITY_CONTEXT along with URI_IS_LOCAL_RESOURCE to selectively allow data-URI-encoded resources inside of SVG images → 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
Target Milestone: mozilla10 → ---
Attached patch fix v2a (obsolete) — Splinter Review
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).
Attachment #572565 - Attachment is obsolete: true
Attachment #574112 - Flags: review?(bzbarsky)
Attached patch fix v2bSplinter Review
(fixed comment typo -- removed erroneous "-" prefix from "-moz-filedata")
Attachment #574112 - Attachment is obsolete: true
Attachment #574112 - Flags: review?(bzbarsky)
Attachment #574113 - Flags: review?(bzbarsky)
Comment on attachment 574113 [details] [diff] [review]
fix v2b

r=me
Attachment #574113 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da841d96925
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/2da841d96925
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(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.
> 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.
Keywords: dev-doc-needed
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.
(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!
Blocks: 688901
(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 ?
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.)
(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.)
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.
(Agreed; thanks Robert.)
Blocks: 1195123
You need to log in before you can comment on or make changes to this bug.