1054646 - Can't use createObjectURL inside sandboxed iframes

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Daniel Huigens]

Attachment: testcase-sandboxed-object-url.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140804060850

Steps to reproduce:

(See attached testcase.)

1. Within a sandboxed iframe:
2. Create an object url with URL.createObjectURL
3. Try to access it (e.g. with XMLHttpRequest, or set the src attribute of something to it)


Actual results:

On XMLHttpRequest#send(), Firefox throws NS_ERROR_DOM_BAD_URI: Access to restricted URI denied.

If you don't use XMLHttpRequest but set a src attribute instead, you get no error. If the previous src was already loaded, nothing happens.


Expected results:

The sandboxed script should be able to access object urls created inside the sandbox's origin.

Chrome does it right in this case, and the spec says that a sandbox should have an origin: http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#sandboxOrigin.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The basic issue here is that we do a CheckLoadURI, which sees that the target has URI_LOADABLE_BY_SUBSUMERS flag and delegates to CheckMayLoad on the principal.  But it passes false for aAllowIfInheritsPrincipal, and the check for nsIURIWithPrincipal in nsNullPrincipal::CheckMayLoad is guarded on aAllowIfInheritsPrincipal.

This seems bogus; why shouldn't we always allow loads of nsIURIWithPrincipal that matches our principal?  Ian, Johnny, was there a reason this code was written this way?

Comment 2

[Boris Zbarsky [:bzbarsky]]

And in particular, I'm not sure I understand how the patches from bug 341604 work, given this behavior of CheckLoadURI...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, they "work" because workers actually skip calling CheckLoadURI in the cases when they directly call CheckMayLoad, like when setting up the worker(!).  That actually seems fairly fishy to me; I'm not sure anything guarantees that CheckMayLoad is strictly more restrictive than CheckLoadURI.

The tests for bug 341604 should probably have tested some sane things with normal load codepaths, like images or iframes... ;)

Comment 4

[Ian Melven :imelven]

(In reply to Boris Zbarsky [:bz] from comment #1)
> The basic issue here is that we do a CheckLoadURI, which sees that the
> target has URI_LOADABLE_BY_SUBSUMERS flag and delegates to CheckMayLoad on
> the principal.  But it passes false for aAllowIfInheritsPrincipal, and the
> check for nsIURIWithPrincipal in nsNullPrincipal::CheckMayLoad is guarded on
> aAllowIfInheritsPrincipal.
> 
> This seems bogus; why shouldn't we always allow loads of nsIURIWithPrincipal
> that matches our principal?  Ian, Johnny, was there a reason this code was
> written this way?

I'm pretty sure this wasn't an explicit decision. I probably didn't dig too deep since things 'worked'. It seems right (and per spec) that a sandboxed iframe should be able to load anything with its same null principal. Comment 3 explains workers, how does it work for images and iframes etc ? I see what you're saying about "how did (does) this ever work ?" :/

Comment 5

[Boris Zbarsky [:bzbarsky]]

> how does it work for images and iframes etc ?

It doesn't.  See comment 0 which talks about setting src not working.  Presumably we have no tests for such things in sandboxed iframes...

Ian, do you have the bandwidth to look into this, or should I find someone else?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And I guess my other question is what the semantics of aAllowIfInheritsPrincipal are really supposed to be.  That is, should the boolean passed in CheckLoadURI be changed, or should the check for an nsIURIWithPrincipal not be conditioned on aAllowIfInheritsPrincipal?

Comment 7

[Ian Melven :imelven]

(In reply to Boris Zbarsky [:bz] from comment #5)
> > how does it work for images and iframes etc ?
> 
> It doesn't.  See comment 0 which talks about setting src not working. 
> Presumably we have no tests for such things in sandboxed iframes...

yeah, i think the only tests around createObjectURL are for workers (if there are those there even)

> Ian, do you have the bandwidth to look into this, or should I find someone
> else?

I'd be happy to take a look but to be honest, this would be a nights and weekends project for me at this point and my time even during those periods is pretty scant at the moment. Bob Owen knows the iframe sandboxing code pretty well if he has cycles.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ian, make sense.  I'm not trying to pressure you into fixing this; just making sure this doesn't get stuck.  ;)

Bob, do you have time to write up some tests here and figure out how this should work?

Comment 9

[Ian Melven :imelven]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Ian, make sense.  I'm not trying to pressure you into fixing this; just
> making sure this doesn't get stuck.  ;)

Oh I know ! :) I just feel guilty about not having time to clean up my mess and nominating Bob yet again to do it.. 

I was thinking the same thing as your question in comment 6 - without looking too deeply at things, my instinct is that aAllowsIfInheritsPrincipal should be changed, fwiw.

Comment 10

[Bob Owen (:bobowen)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Ian, make sense.  I'm not trying to pressure you into fixing this; just
> making sure this doesn't get stuck.  ;)
> 
> Bob, do you have time to write up some tests here and figure out how this
> should work?

I might, but it'll be late this week or (more likely) early next week.
I need to crack on with Windows content process sandboxing stuff.

I'm not familiar with this part of the sandboxing code, so it might take me a little while to get up to speed.

Comment 11

[Johnny Stenback (:jst)]

(In reply to Boris Zbarsky [:bz] from comment #1)
[...]
> This seems bogus; why shouldn't we always allow loads of nsIURIWithPrincipal
> that matches our principal?  Ian, Johnny, was there a reason this code was
> written this way?

My memory fails me on the details here, but that does indeed seem bogus, Ian is probably right here in that this detail got overlooked when this initially landed :(

Comment 12

[Bob Owen (:bobowen)]

Just setting myself a reminder to look at this when I get back.

Comment 13

[Bob Owen (:bobowen)]

https://tbpl.mozilla.org/?tree=Try&rev=5753a17d5e72

Comment 14

[Bob Owen (:bobowen)]

Attachment: Part 1: Change nsNullPrincipal::CheckMayLoad to always allow loads when the principal of the URI in the principal doing the load v1

The checks for loading seem to be a little convoluted, but I think the following reasoning makes sense.

The callers of CheckMayLoad that pass false (which this change affects) seem to fall into three camps.
* Those that are actually state they are doing same origin checks, which is exactly what this change does.
* Those where previous conditions preclude aURI from being an nsIURIWithPrincipal anyway.
* Those where it really is doing a load check, but for redirects where it doesn't want to allow security inheriting URIs. nsIURIWithPrincipals shouldn't be a problem for redirects.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 1: Change nsNullPrincipal::CheckMayLoad to always allow loads when the principal of the URI is the principal doing the load

Comment 16

[Bob Owen (:bobowen)]

Attachment: Part 2: test loading of blob and data URLs in unique origin iframe sandbox v1

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496599 [details] [diff] [review]
Part 1: Change nsNullPrincipal::CheckMayLoad to always allow loads when the principal of the URI in the principal doing the load v1

>+    if (principal && principal == this) {

I know you just reindented this line, but the null-check of "principal" is redundant, since "this" better not be null here.

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496601 [details] [diff] [review]
Part 2: test loading of blob and data URLs in unique origin iframe sandbox v1

>+        ok(this.status == 200 && this.response == "wibble", "XHR for a blob URL created by in this document should NOT be blocked in an iframe sandboxed WITHOUT 'allow-same-origin'");

s/by in/in/ ?

r=me

Comment 19

[Bob Owen (:bobowen)]

Attachment: Part 1: Change nsNullPrincipal::CheckMayLoad to always allow loads when the principal of the URI is the principal doing the load v2

r=bz - from comment 17

(In reply to Boris Zbarsky [:bz] from comment #17)

> >+    if (principal && principal == this) {
> 
> I know you just reindented this line, but the null-check of "principal" is
> redundant, since "this" better not be null here.

I'd like to know how we got here if it is. :)
Fixed, thanks.

Comment 20

[Bob Owen (:bobowen)]

Attachment: Part 2: test loading of blob and data URLs in unique origin iframe sandbox v2

r=bz - from comment 18

(In reply to Boris Zbarsky [:bz] from comment #18)

> >+        ok(this.status == 200 && this.response == "wibble", "XHR for a blob URL created by in this document should NOT be blocked in an iframe sandboxed WITHOUT 'allow-same-origin'");
> 
> s/by in/in/ ?

Oh dear, re-wording failure there.
Fixed, thanks.

Comment 21

[Bob Owen (:bobowen)]

Try push of v1 patches:
https://tbpl.mozilla.org/?tree=Try&rev=5753a17d5e72

Compiled and tested v2 patches locally, I think the changes are small enough as to not warrant another try push.

Please land in Part number order, thanks.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b98c10254b3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd5ac4a729a

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b98c10254b3d
https://hg.mozilla.org/mozilla-central/rev/1fd5ac4a729a