Last Comment Bug 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
: Check URI_INHERITS_SECURITY_CONTEXT or URI_LOADABLE_BY_SUBSUMERS (along with ...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 628747
Blocks: 672013 688901 1195123
  Show dependency treegraph
 
Reported: 2011-10-12 01:31 PDT by Daniel Holbert [:dholbert]
Modified: 2015-08-16 10:58 PDT (History)
8 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.62 KB, patch)
2011-11-07 11:17 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v1 (2.86 KB, patch)
2011-11-07 11:29 PST, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review
fix v1a [r=bz] (3.21 KB, patch)
2011-11-07 12:03 PST, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
fix v2a (9.87 KB, patch)
2011-11-12 19:43 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v2b (9.87 KB, patch)
2011-11-12 19:47 PST, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-10-12 01:31:58 PDT
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] 2011-11-07 10:42:00 PST
(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] 2011-11-07 11:17:35 PST
Created attachment 572540 [details] [diff] [review]
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] 2011-11-07 11:29:02 PST
Created attachment 572551 [details] [diff] [review]
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 [:bz] 2011-11-07 11:36:23 PST
> 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 [:bz] 2011-11-07 11:42:27 PST
Comment on attachment 572551 [details] [diff] [review]
fix v1

r=me
Comment 6 Daniel Holbert [:dholbert] 2011-11-07 11:47:05 PST
(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] 2011-11-07 11:51:31 PST
Pushed to Try for full test-run on Linux & Mac:
  https://tbpl.mozilla.org/?tree=Try&rev=8c9f8c4ac738
Comment 8 Daniel Holbert [:dholbert] 2011-11-07 12:03:01 PST
Created attachment 572565 [details] [diff] [review]
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] 2011-11-07 12:40:04 PST
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] 2011-11-07 13:11:52 PST
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] 2011-11-07 13:53:07 PST
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) (Exited; email my personal email if necessary) 2011-11-07 17:56:44 PST
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) (Exited; email my personal email if necessary) 2011-11-07 18:04:45 PST
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 [:bz] 2011-11-07 18:43:28 PST
Hmm.  Yes.  We should probably check for LOCAL_RESOURCE and either one of LOADABLE_BY_SUBSUMERS or INHERITS_PRINCIPAL.
Comment 15 Jesse Ruderman 2011-11-07 22:30:03 PST
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] 2011-11-08 01:41:45 PST
https://hg.mozilla.org/mozilla-central/rev/10814cd743e3
Comment 17 Daniel Holbert [:dholbert] 2011-11-08 11:36:23 PST
(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] 2011-11-08 11:40:20 PST
(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 [:bz] 2011-11-08 12:42:18 PST
Yes, that is indeed what I mean.
Comment 20 Daniel Holbert [:dholbert] 2011-11-08 12:54:05 PST
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] 2011-11-08 20:07:05 PST
Backed out from aurora:
 http://hg.mozilla.org/releases/mozilla-aurora/rev/288a354d72da
Comment 22 Daniel Holbert [:dholbert] 2011-11-12 19:43:12 PST
Created attachment 574112 [details] [diff] [review]
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] 2011-11-12 19:47:17 PST
Created attachment 574113 [details] [diff] [review]
fix v2b

(fixed comment typo -- removed erroneous "-" prefix from "-moz-filedata")
Comment 24 Boris Zbarsky [:bz] 2011-11-12 21:35:21 PST
Comment on attachment 574113 [details] [diff] [review]
fix v2b

r=me
Comment 25 Daniel Holbert [:dholbert] 2011-11-12 21:50:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da841d96925
Comment 26 Ed Morley [:emorley] 2011-11-14 04:43:21 PST
https://hg.mozilla.org/mozilla-central/rev/2da841d96925
Comment 27 Janet Swisher 2011-12-15 15:20:13 PST
(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] 2011-12-15 17:39:02 PST
> 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 2011-12-16 08:41:10 PST
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] 2011-12-16 08:45:33 PST
(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 2015-08-14 09:05:34 PDT
(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] 2015-08-14 09:11:25 PDT
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] 2015-08-14 09:29:16 PDT
(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 2015-08-14 11:25:49 PDT
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] 2015-08-14 11:37:02 PDT
(Agreed; thanks Robert.)
Comment 36 ytrezq 2015-08-14 11:47:14 PDT Comment hidden (offtopic)
Comment 37 Daniel Holbert [:dholbert] 2015-08-14 11:58:05 PDT Comment hidden (offtopic)
Comment 38 ytrezq 2015-08-16 10:43:27 PDT Comment hidden (offtopic)
Comment 39 Daniel Holbert [:dholbert] 2015-08-16 10:51:01 PDT Comment hidden (offtopic)
Comment 40 ytrezq 2015-08-16 10:51:25 PDT Comment hidden (offtopic)
Comment 41 Daniel Holbert [:dholbert] 2015-08-16 10:54:34 PDT Comment hidden (offtopic)

Note You need to log in before you can comment on or make changes to this bug.