686013 - SVG <use> and filters won't load fragments referenced via a data URI (ending with a fragment ID)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR: Load attached testcase.

EXPECTED RESULTS:
 Document is entirely lime. (<use> target is successfully rendered.)

ACTUAL RESULTS:
 Fully red. (<use> target isn't rendered)

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110908 Firefox/9.0a1

(BTW: Opera 11.51 gives EXPECTED RESULTS. Chromium 14.0.835.126 renders red like us.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (helper file for reference case 1)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reference case 1

Comment 3

[Daniel Holbert [:dholbert]]

Note: In reference case 1 & its helper file, I've just taken "testcase 1" and split its data URI out into a separate file, and I've made the <use> element target that instead.

(With that change, my nightly shows EXPECTED RESULTS. Chromium still renders it as red for some reason.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

We don't allow cross-document <use>, on purpose.  Does Opera allow it in general?

Comment 5

[Daniel Holbert [:dholbert]]

The reference case shows that we do allow cross-document <use>, if they're same-origin.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: anti-reference case (not same-origin, should render as red)

Here's a tweaked copy of the reference case, to test cross-domain behavior. (for security purposes, I'd expect the <use> to _not_ resolve in this case.)

I've simply replaced "bug686013.bugzilla.mozilla.org" with "bugzilla.mozilla.org" in the <use> target (which is a different origin from the "bug686013" subdomain, where this anti-reference-case gets served from).

Comment 7

[Daniel Holbert [:dholbert]]

Firefox correctly renders the anti-reference case as red. Opera renders it as lime -- so it appears they allow cross-origin <use>.

In any case, though -- for Firefox, I'd think that as long as we're allowing same-origin-use, we should also allow data-URI <use>. (since the data URI's data is inline, and is hence being served from the same origin)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  We probably need to propagate the owner to the channel when loading resource documents, then...  I was pretty sure we didn't allow cross-document <use>; it's surprising that we do!

You want to take this, or should I?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Ah, I see.  We probably need to propagate the owner to the channel when
> loading resource documents, then...

Ah -- I was imagining this might be a case where we'd need to add a URI_IS_LOCAL_RESOURCE check to nsDataDocumentContentPolicy::ShouldLoad, much like in the patch for bug 628747.  Perhaps we don't hit those checks in this case, though?  (or we check elsewhere too?)

> You want to take this, or should I?

I can take it.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (vacation 9/17-9/24) from comment #9)
> Ah -- I was imagining this might be a case where we'd need to add a
> URI_IS_LOCAL_RESOURCE check to nsDataDocumentContentPolicy::ShouldLoad

Nevermind -- we don't actually get there, in this case.  We bail out here:
> 1090 nsresult
> 1091 nsExternalResourceMap::PendingLoad::StartLoad(nsIURI* aURI,
> 1092                                               nsINode* aRequestingNode)
> 1093 {
[...]
> 1106   rv = requestingPrincipal->CheckMayLoad(aURI, PR_TRUE);
> 1107   NS_ENSURE_SUCCESS(rv, rv);

CheckMayLoad fails there.

Perhaps we want to blanket-allow data URIs inside of nsPrincipal::CheckMayLoad?

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This adds a blanket-approval for URIs with the "data" scheme, in nsPrincipal::CheckMayLoad.

Does this make sense?

Comment 12

[Daniel Holbert [:dholbert]]

(I've confirmed that the attached 'fix v1' fixes this bug, btw.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Hmm.  Jonas, what do you think of hacking CheckMayLoad like that?

If we do change CheckMayLoad (which I think is sensible here), I think it makes more sense to check for the "uri inherits security context" flag instead of the data: scheme.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Review of attachment 559607 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I agree with bz. The general idea seems fine, but use URI flags instead.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Ok, this checks aURI for URI_IS_LOCAL_RESOURCE uri flag, and allows us to proceed if it's set.

(I initially worried that this would break the restriction on web content loading local files, or local files loading files from other directories -- but that doesn't seem to be the case -- that gets caught elsewhere (and triggers 'Security Error: Content at http://whatver/foo.svg may not load or link to file:///path/to/used/file.svg.')

Comment 17

[Boris Zbarsky [:bzbarsky]]

Daniel, why IS_LOCAL_RESOURCE instead of INHERITS_SECURITY_CONTEXT?

Comment 18

[Daniel Holbert [:dholbert]]

I was just using the same flag as in bug 628747. (and wasn't aware of INHERITS_SECURITY_CONTEXT)

IS_LOCAL_RESOURCE works correctly AFAICT, and we have other checks that prohibit remote sites from loading *actual* local (on-disk) resources -- but it sounds like INHERITS_SECURITY_CONTEXT might be more correct here. I'll give that a shot.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, hmm.  Thank you for the reminder about bug 628747.

The key issue in that bug was avoiding sending information to the server, so allowing loads of arbitrary local resources subject to CheckLoadURI was OK.  That said, I think that patch may have made it leak information from local files to other local files (e.g. by linking them from an SVG <img> and then painting it to a canvas).  Can we double-check that?

The check here, though, is not to prevent sending information but to prevent the loading document from _getting_ any information out of the linked-to resource if that resource is cross-origin.  So on the face of it IS_LOCAL_RESOURCE is the wrong thing here.  In particular, if we allow some file in ~/Downloads to use some file in ~/work/SVG-stuff as a filter or mask, then it can read information from it, which it's not supposed to be able to do, right?

I really wish same-origin policy for file:// were saner.  :(

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> That said, I think that patch may have made it leak information
(to be fair: it rather "left one existing leak open while closing a bunch of others" :))

> from local
> files to other local files (e.g. by linking them from an SVG <img> and then
> painting it to a canvas).  Can we double-check that?

That's true, aside from the "painting it to a canvas" part -- SVG images taint canvases for now, though bug 672013 is filed on relaxing that.  Without that, I'm not sure there's any leakage that's particularly worrisome.  The attack scenarios that I laid out in bug 628747 comment 0 don't really make sense for local content.

I actually added reftests for the current semi-data-leaky-between-local-files behavior (not because it's "correct", but rather to be sure we know if it changes). Specifically, the reftest "svg-image-external-1.html" has different rendering when loaded as a file:// vs. http://, as shown in the reftest manifest here:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/as-image/reftest.list?mark=112-113#113
(the non-HTTP version fails in android because *all* Android reftests are served over HTTP.)

For reference, the SVG image used there is:
http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/svg/as-image/svg-image-external.svg

> In particular, if we allow some
> file in ~/Downloads to use some file in ~/work/SVG-stuff as a filter or
> mask, then it can read information from it, which it's not supposed to be
> able to do, right?

Right.

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: fix v3: with INHERITS_SECURITY_CONTEXT

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: reftests as patch

This tests patch includes 3 reftests:
 - one test for the original testcase here (with <use>)
 - one test like the one from bug 692806 (but with a mask instead of a filter, since masks are easier to write than filters)
 - one test to assert that we're not vulnerable to the attack bz suggested at the end of comment 19.

Comment 23

[Daniel Holbert [:dholbert]]

((In reply to Daniel Holbert [:dholbert] from comment #22)
>  - one test to assert that we're not vulnerable to the attack bz suggested
> at the end of comment 19.

(To clarify: fix v2 wasn't vulnerable to that attack, either, due to other same-origin checks. So, fix v2 and fix v3 are functionally equivalent, AFAICT -- v3 just uses a more appropriate flag to do its checking and isn't as reliant on our other file://-same-origin-checks working correctly.)

Comment 24

[Daniel Holbert [:dholbert]]

Hmm, the current patch makes us fail this mochitest:
> 56 ERROR TEST-UNEXPECTED-FAIL | /tests/content/xbl/test/test_bug379959.html | data-url load should have failed - got 1, expected 0

Canceling review and investigating...

Comment 25

[Daniel Holbert [:dholbert]]

So we currently have some code that depends on nsPrincipal::CheckMayLoad rejecting data URIs.

In particular, nsContentUtils::CheckSecurityBeforeLoad() has an explicit "aAllowData" flag, and if that's true (and we have a data URI), that function returns NS_OK -- but if it's false, it defers to nsPrincipal::CheckMayLoad()  (which it expects will reject data URIs).

This is part of what's going on in test_bug379959.html (where "aAllowData" is ultimately determined by the pref "layout.debug.enable_data_xbl", which that test sets).

I'm not sure whether we care about supporting "layout.debug.enable_data_xbl" anymore -- for now, I think it's probably simplest to just move this bug's "data is allowed" clause up one level, to nsExternalResourceMap::PendingLoad::StartLoad.  Patch coming up that does that.

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: fix v4: check for data URIs in nsExternalResourceMap

Comment 27

[Daniel Holbert [:dholbert]]

SIDE NOTE:
fix v3 (INHERITS_SECURITY_CONTEXT early-return in nsPrincipal::CheckMayLoad) also caused a jsreftest test-failure in regress-328897.js, with this failure message (newlines added for readability):
> REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-328897.js |
>  JS_ReportPendingException should Expected match to '/(Script error.|uncaught exception: Permission denied to get property UnnamedClass.classes)/',
>  Actual value 'Permission denied for <file://> to get property XPCComponents.classes'  item 17
https://tbpl.mozilla.org/php/getParsedLog.php?id=6736702&tree=Try

That jsreftest is here: 
http://mxr.mozilla.org/mozilla-central/source/js/src/tests/js1_5/Regress/regress-328897.js

It attempts to load the URI "javascript:Components.classes", and it expects that load to fail with a specific message.  The load still did still fail in my patched build, but with a mention of "XPCComponents.classes" instead of "UnnamedClass.classes".  Not sure why.

In any case, fix v4 doesn't have that problem.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 565656 [details] [diff] [review]
reftests as patch

r=me

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 565973 [details] [diff] [review]
fix v4: check for data URIs in nsExternalResourceMap

r=me, though the fact that CheckMayLoad doesn't test true for data: is sort of odd...  Maybe file a followup on considering changing that?

Comment 30

[Daniel Holbert [:dholbert]]

Ok.  And just to be sure -- is it an issue that this check (URI_INHERITS_SECURITY_CONTEXT) isn't data:-specific?  (It lets through javascript: URIs, too.)

I can't think of a way to do anything useful (or evil) with a javascript: URI as an external resource, but I want to be cautious...

Perhaps I should be checking for
  URI_IS_LOCAL_RESOURCE | URI_INHERITS_SECURITY_CONTEXT
to be more targeted at specifically data: URIs?  (note that javascript: doesn't have URI_IS_LOCAL_RESOURCE)

Comment 31

[Boris Zbarsky [:bzbarsky]]

> is it an issue that this check (URI_INHERITS_SECURITY_CONTEXT) isn't data:-specific? 

I think it's a plus, personally...  We really shouldn't be targeting particular schemes; we should be targeting properties of schemes.

Comment 32

[Daniel Holbert [:dholbert]]

Cool, I absolute agree on that point.  I just don't have a lot of experience dealing with javascript: URIs, and I wanted to make sure they didn't have some obscure property that I was unaware of that could cause unnecessary badness here.  (It didn't seem like it, but just wanted to check.)  Thanks!

Comment 33

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #29)
> r=me, though the fact that CheckMayLoad doesn't test true for data: is sort
> of odd...  Maybe file a followup on considering changing that?

Filed bug 693936.

Comment 34

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> Ah, hmm.  Thank you for the reminder about bug 628747.
[...]
(In reply to Daniel Holbert [:dholbert] from comment #20)
> > from local
> > files to other local files (e.g. by linking them from an SVG <img> and then
> > painting it to a canvas).  Can we double-check that?
> 
> That's true, aside from the "painting it to a canvas" part -- SVG images
> taint canvases for now, though bug 672013 is filed on relaxing that. 
> Without that, I'm not sure there's any leakage that's particularly
> worrisome.

I filed Bug 693940 on fixing up this behavior, FWIW.

Comment 35

[Daniel Holbert [:dholbert]]

Landed fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe49f0b3093
and tests:  https://hg.mozilla.org/integration/mozilla-inbound/rev/bc75b51a3b60

Comment 36

[Boris Zbarsky [:bzbarsky]]

javascript: URIs should be fine here; they'll just run in a sandbox.

Comment 37

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/fbe49f0b3093
https://hg.mozilla.org/mozilla-central/rev/bc75b51a3b60

Comment 38

[Jean-Yves Perrier [:teoli]]

I added dev-doc-needed. I think we need to document the requirement for the same-origin resource in https://developer.mozilla.org/en/SVG/Element/use .

Comment 39

[Jeremie Patonnier :Jeremie]

(In reply to Jean-Yves Perrier [:teoli] from comment #38)
> I added dev-doc-needed. I think we need to document the requirement for the
> same-origin resource in https://developer.mozilla.org/en/SVG/Element/use .

Agreed, I'll take care of this.

Comment 40

[Jeremie Patonnier :Jeremie]

Done

A technical review of the doc will be welcomed :)

Teoli, it seams that I can't remove the dev-doc-needed on my own, I'll let you handle this.

Comment 41

[Robert Longson [:longsonr]]

Jeremie, send a mail to Gerv asking for editbugs. See http://www.gerv.net/hacking/before-you-mail-gerv.html

Comment 42

[Jean-Yves Perrier [:teoli]]

Thanks Jeremie. I've set the keyword to dev-doc-complete. As said Robert Longson, mail Gerv for the bugzilla rights. Link to here and to a few of your numerous (and pertinent) bug reports. You're holding the MDN SVG doc almost alone these days, you need editbugs.

Comment 43

[Jeremie Patonnier :Jeremie]

Thanks for the confidence guys :) Gerv updated my rights, now I can do big mistake on bugzilla :D