Closed Bug 1054646 Opened 6 years ago Closed 5 years ago

Can't use createObjectURL inside sandboxed iframes

Categories

(Core :: Security, defect)

34 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: d.huigens, Assigned: bobowen)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jst)
Flags: needinfo?(ian.melven)
And in particular, I'm not sure I understand how the patches from bug 341604 work, given this behavior of CheckLoadURI...
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... ;)
(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 ?" :/
Flags: needinfo?(ian.melven)
> 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?
Flags: needinfo?(ian.melven)
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?
(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.
Flags: needinfo?(ian.melven)
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?
Flags: needinfo?(bobowencode)
(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.
(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.
Flags: needinfo?(bobowencode)
(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 :(
Flags: needinfo?(jst)
Just setting myself a reminder to look at this when I get back.
Flags: needinfo?(bobowencode)
https://tbpl.mozilla.org/?tree=Try&rev=5753a17d5e72
Assignee: nobody → bobowencode
Flags: needinfo?(bobowencode)
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.
Attachment #8496599 - Flags: review?(bzbarsky)
Attachment #8496600 - Attachment is obsolete: true
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
Attachment #8496599 - Flags: review?(bzbarsky) → review+
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
Attachment #8496601 - Flags: review?(bzbarsky) → review+
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.
Attachment #8496599 - Attachment is obsolete: true
Attachment #8497355 - Flags: review+
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.
Attachment #8496601 - Attachment is obsolete: true
Attachment #8497364 - Flags: review+
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.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.