The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({dev-doc-complete})

Trunk
mozilla11
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 1

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 672013
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #572540 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
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)
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+
(Assignee)

Comment 6

5 years ago
(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+!
(Assignee)

Comment 7

5 years ago
Pushed to Try for full test-run on Linux & Mac:
  https://tbpl.mozilla.org/?tree=Try&rev=8c9f8c4ac738
Flags: in-testsuite?
(Assignee)

Comment 8

5 years ago
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
Attachment #572551 - Attachment is obsolete: true
Attachment #572565 - Flags: review+
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
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.)
(Assignee)

Comment 11

5 years ago
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.

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
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 → ---
(Assignee)

Comment 21

5 years ago
Backed out from aurora:
 http://hg.mozilla.org/releases/mozilla-aurora/rev/288a354d72da
(Assignee)

Updated

5 years ago
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 → ---
(Assignee)

Comment 22

5 years ago
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).
Attachment #572565 - Attachment is obsolete: true
Attachment #574112 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

5 years ago
Created attachment 574113 [details] [diff] [review]
fix v2b

(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+
(Assignee)

Comment 25

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 27

5 years ago
(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.
(Assignee)

Comment 28

5 years ago
> 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

Comment 29

5 years ago
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.
Keywords: dev-doc-complete
(Assignee)

Comment 30

5 years ago
(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!
(Assignee)

Updated

5 years ago
Blocks: 688901

Comment 31

2 years ago
(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 ?
(Assignee)

Comment 32

2 years ago
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.)
(Assignee)

Comment 33

2 years ago
(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

2 years ago
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.
(Assignee)

Comment 35

2 years ago
(Agreed; thanks Robert.)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
(Assignee)

Updated

2 years ago
Blocks: 1195123
You need to log in before you can comment on or make changes to this bug.