Closed Bug 859454 Opened 12 years ago Closed 11 years ago

Document loading should check the script entry point's sandboxing and block the load if needed

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Version:
17 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: imelven, Assigned: imelven)

References

Details

See bholley's comment https://bugzilla.mozilla.org/show_bug.cgi?id=838692#c32:

"So for both this bug and bug 785310, I think we'll want to do a dynamic heck during loading and determine if the script entry point is sandboxed. If so, we'll want to do the relevant computation to determine if it's allowed."

this bug is about doing that. This will form the basis (if not the entirety) of fixing Bug 785310 - html5 sandboxed iframe should not be able to perform top navigation with scripts allowed (using code from the patches there) and Bug 766282 - implement allow-popups directive for iframe sandbox
Assignee: nobody → imelven
Blocks: 785310, 766282
Status: NEW → ASSIGNED
Here's an example to make sure I understand (and this bug documents) the right behavior.

Consider two documents A and B contained in iframes in the same top level document Z:

* document A is sandboxed with allow-scripts and allow-same-origin (it is same origin with Z)
* document B is in a standard iframe (same origin with Z and Aand contains a function F which calls window.top.location = 'http://foo.com';
* script in A calls B's F() - A is sandboxed without top navigation and since A is the script entry point its sandboxing flags are used and the top navigation is blocked.

also, bholley: i was looking at your fix for bug 809290 where you changed nsLocation::CheckURL and that looks like it might be a reasonable place to add the sandbox checks at first glance, I'll investigate that a bit.
I tried doing this :

  if (JSContext *cx = nsContentUtils::GetCurrentJSContext()) {
    nsCOMPtr<nsIDocument> doc;
    nsCOMPtr<nsPIDOMWindow> entryPoint =
      do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
    if (entryPoint) {
      doc = entryPoint->GetDoc();
    }  
  }   

in nsDocument::StartDocumentLoad but the results were pretty inconsistent - the first time through it seemed like doc was the script entry point and I could do the sandboxing checks with it, but this wasn't always true. For example, after hitting the back button after the window.top.location navigation happens the first time and then setting window.top.location from script again. I need to understand the bfcache better.
The JS isn't still on the stack when nsDocument::StartDocumentLoad is called, nsDocShell::LoadURI might be a better place for the sandboxing checks.
Which JS are you looking for?

For example, if a load is done via click() on an anchor, the JS that called click() won't be on the stack when LoadURI is called either.
(In reply to Boris Zbarsky (:bz) from comment #4)
> Which JS are you looking for?

Basically if the load is triggered from JS, I want to check the script entry point's document's sandboxing flags and abort the load if it should be blocked - I'm using the code in comment 2 to try and do this... The two main cases I'm trying to cover are setting window.top.location (bug 785310) and window.open (bug 766282) but there may definitely be more I'm not aware of yet.

> For example, if a load is done via click() on an anchor, the JS that called
> click() won't be on the stack when LoadURI is called either.

For clicking on anchors, I think the click() on an anchor case for top navigation or opening a new windows will covered by the existing checks, especially when bug 838692 lands.
If you want to cover location stuff, I suggest doing that in nsLocation.  And window.open is best handled in window.open.

But yes, in the end those both bottleneck through LoadURI.

Note that click() on anchor from script is not obviously identical to the user clicking on an anchor, esp. if the node and the script that did the click() might happen to have different sandboxing flags.  _Can_ that happen?
(In reply to Boris Zbarsky (:bz) from comment #6)
> If you want to cover location stuff, I suggest doing that in nsLocation. 
> And window.open is best handled in window.open.
> 
> But yes, in the end those both bottleneck through LoadURI.

Maybe this bug is INVALID then and it doesn't make sense to try and do this in some generic way. It seems pretty easy to add the check in nsLocation::CheckURL where we already get the script entry point - I'll work on a new patch for 785310 that takes that approach, I think. I'm not sure exactly what Bob's patch for window.open does at the moment but I'll take a look, I need to provide feedback on his patches anyways but was waiting until we figured out what to do here.

> Note that click() on anchor from script is not obviously identical to the
> user clicking on an anchor, esp. if the node and the script that did the
> click() might happen to have different sandboxing flags.  _Can_ that happen?

Ah, that is an interesting case - yes, it can happen.

Consider two documents A and B contained in iframes in the same top level document Z:

* document A is sandboxed with allow-same-origin (it is same origin with Z) and
contains an anchor e.g. <a href='example.com' target='_top>

* document B is in a standard iframe (same origin with Z and A and contains a function F which gets the anchor in A and calls click() on it

1) if the anchor in A is clicked directly, A's sandboxing flags should be used and the top navigation should be blocked

2) if F is called the script entry point's sandboxing flags should be used and the top navigation should be allowed

I'll test this case to see what the current behavior is, I suspect that right now both 1) and 2) are blocked. That seems like it's wrong if we always want to use the script entry point's document's sandboxing flags for sandboxing checks.
Just a note that Freddy ran into something along these lines this morning which is at least a little counter-intuitive : with a child sandboxed iframe that has allow-same-origin but not allow-scripts, you can run script in it via something like : 

var iframe = document.getElementById('iframe');
iframe.contentWindow.eval("alert(document.location)");

because the parent is the script entry point and is not sandboxed. It seems ok to me since the parent can just remove the allow-scripts or sandboxing completely anyways.
So.. presumably the spec actually says exactly whose sandbox flags should be checked and when.  Depending on whether what it says is sane, it might need changing too.
I don't want to complicate things further and I might have misunderstood things (or it could be the wine on a Friday night doing the thinking) but ...

it seems to me that the most secure way to deal with things is to take account of the sandboxing flags in both (or more) places.
If a document has been deliberately sandboxed, it seems that another browsing context calling through that document shouldn't be able to override that, in case it inadvertently calls dangerous code.

I'm not sure how easy this is to implement, but in theory what about this approach:

At the script entry point a copy of the document's sandboxing flag set is taken.
As the script wanders through other documents, it collects any other flags that might be set (like a cyber Vexillologist).
This is then the flag set that is used when any sandboxing decisions are made from a script point of view.
The spec talks about using the sandboxing flags of the source browsing context (see http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigate).

It also says "Navigation always involves source browsing context, which is the browsing context which was responsible for starting the navigation." (http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#source-browsing-context) which will be the script entry point for script-initiated navigations. 

Keep in mind that the cases in comment 1, comment 7 and comment 8 both require the iframe to be sandboxed with 'allow-same-origin' so script can call into it. My opinion is that sandboxing is about restricting the document itself that's sandboxed, not protecting the sandboxed document from other documents. If other documents should not be able to manipulate the sandboxed document, then they should be sandboxed as well :)

I think always using the script entry point's sandboxing flags solves some other quirky cases with allow-same-origin - for example in Chrome, you can't navigate top using window.top.location = .. without allow-top-navigation but you can do window.top.eval('window.location = ...') which is not blocked. Using the script entry point's sandboxing flags (as the spec says to do) solves this problem.

I'm very curious how other browsers that implement iframe sandbox behave in the cases we've discussed in this bug, I'll take a look at that. Also, if we need to change the handling of .click I'll file a bug. 

Boris, can you think of any other script-initiated navigations off hand ? :)
Re comment 8, this is what I found in the spec (http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#enabling-and-disabling-scripting) : 

Scripting is enabled in a browsing context if ... The browsing context's active document's active sandboxing flag set does not have its sandboxed scripts browsing context flag set.

Scripting is enabled for a node if the Document object of the node (the node itself, if it is itself a Document object) has an associated browsing context, and scripting is enabled in that browsing context.

This part seems a little vague in terms of the case in comment 8. I'll bring this up on the WHATWG list.
(In reply to Ian Melven :imelven from comment #12)
> Re comment 8, this is what I found in the spec
> (http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.
> html#enabling-and-disabling-scripting) : 
> 
> Scripting is enabled in a browsing context if ... The browsing context's
> active document's active sandboxing flag set does not have its sandboxed
> scripts browsing context flag set.
> 
> Scripting is enabled for a node if the Document object of the node (the node
> itself, if it is itself a Document object) has an associated browsing
> context, and scripting is enabled in that browsing context.
> 
> This part seems a little vague in terms of the case in comment 8. I'll bring
> this up on the WHATWG list.

Note that Firefox, Chrome and IE 10 all behave the same in this case - the alert fires with the location of the iframe sandboxed without allow-scripts.
> Boris, can you think of any other script-initiated navigations off hand ? :)

The obvious ones I know of are manipulation of the location object, click() on anchors (or equivalent with firing of click events), submit() on forms, window.open(), document.open() (which forwards to window.open in some cases), history traversals, changes to the src attribute on iframes (and similar for "data" on objects, etc).  I guess <meta refresh> is not script-initiated, yes?
(In reply to Ian Melven :imelven from comment #11)
> The spec talks about using the sandboxing flags of the source browsing
> context (see
> http://www.whatwg.org/specs/web-apps/current-work/multipage/history.
> html#navigate).
> 
> It also says "Navigation always involves source browsing context, which is
> the browsing context which was responsible for starting the navigation."
> (http://www.whatwg.org/specs/web-apps/current-work/multipage/history.
> html#source-browsing-context) which will be the script entry point for
> script-initiated navigations. 

So, if the browsing context of the script entry point is treated as the script's browsing context (see http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#script%27s-browsing-context) for all scripts we pass through, even if in a different browsing context. 
Then according to the above, if a browsing context calls a function in a child that opens a new window, then it (and not the child) should be set as the opener.
If in addition the browsing context is sandboxed, then it would also be the one permitted sandboxed navigator.

Also, should the browsing context of the script entry point be the one used for the |aOriginalRequestor| when searching in nsDocShell::FindItemWithName()?
Maybe this is how it works already, or it's being picked up by another bug as it's a wider issue than sandboxing? 

> 
> Keep in mind that the cases in comment 1, comment 7 and comment 8 both
> require the iframe to be sandboxed with 'allow-same-origin' so script can
> call into it. My opinion is that sandboxing is about restricting the
> document itself that's sandboxed, not protecting the sandboxed document from
> other documents. If other documents should not be able to manipulate the
> sandboxed document, then they should be sandboxed as well :)

I suppose the same-origin criterion, reduces the risk
But I was thinking if un-sandboxed code is calling through code in a document that is sandboxed, because it is felt that it is vulnerable or produced by another party that is not so trusted.
Then someone might spot this and try and change the function in the sandboxed document, in order to get their code executed without sandboxing.
(In reply to Boris Zbarsky (:bz) from comment #14)
> > Boris, can you think of any other script-initiated navigations off hand ? :)
> 
> The obvious ones I know of are manipulation of the location object, click()
> on anchors (or equivalent with firing of click events), submit() on forms,
> window.open(), document.open() (which forwards to window.open in some
> cases), history traversals, changes to the src attribute on iframes (and
> similar for "data" on objects, etc).  I guess <meta refresh> is not
> script-initiated, yes?

Thank you for that list ! Lots of interesting cases here - based on what you said previously in comment 6 I assume we can't handle all these cases in one central point e.g. LoadURI and they'll need to be addressed individually :/

FWIW, I wouldn't consider <meta refresh> to be script-initiated - Microsoft seem to consider that an 'automatic feature' (http://www.whatwg.org/specs/web-apps/current-work/multipage/origin-0.html#sandboxed-automatic-features-browsing-context-flag) based on http://samples.msdn.microsoft.com/ietestcenter/#html5Sandbox - Webkit and Gecko don't block it at all currently (blocking automatic features is bug 752551).
(In reply to Bob Owen from comment #15)
>
> So, if the browsing context of the script entry point is treated as the
> script's browsing context (see
> http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.
> html#script%27s-browsing-context) for all scripts we pass through, even if
> in a different browsing context. 
> Then according to the above, if a browsing context calls a function in a
> child that opens a new window, then it (and not the child) should be set as
> the opener.
>
> If in addition the browsing context is sandboxed, then it would also be the
> one permitted sandboxed navigator.

yes, that makes sense to me.
 
> Also, should the browsing context of the script entry point be the one used
> for the |aOriginalRequestor| when searching in
> nsDocShell::FindItemWithName()?
>
> Maybe this is how it works already, or it's being picked up by another bug
> as it's a wider issue than sandboxing? 

not sure, I'd have to debug to see how it works currently :) 

> I suppose the same-origin criterion, reduces the risk
> But I was thinking if un-sandboxed code is calling through code in a
> document that is sandboxed, because it is felt that it is vulnerable or
> produced by another party that is not so trusted.
> Then someone might spot this and try and change the function in the
> sandboxed document, in order to get their code executed without sandboxing.

i'm not clear on this last piece - how would the sandboxed document try to do this ?
> I assume we can't handle all these cases in one central point

The problem is that some of these call LoadURI async.

One thing worth considering is that for all of these things we propagate through the loading principal.  So if we just propagate through the loading sandbox flags, and determine those at the same time as the principal we might be in a good situation.  Maybe.
(In reply to Ian Melven :imelven from comment #17)
> (In reply to Bob Owen from comment #15)
> 
> > I suppose the same-origin criterion, reduces the risk
> > But I was thinking if un-sandboxed code is calling through code in a
> > document that is sandboxed, because it is felt that it is vulnerable or
> > produced by another party that is not so trusted.
> > Then someone might spot this and try and change the function in the
> > sandboxed document, in order to get their code executed without sandboxing.
> 
> i'm not clear on this last piece - how would the sandboxed document try to
> do this ?

The document itself wouldn't, but if a part of your site was maintained by a different team (or third party) and you need to call functions within this document.
You might think that sandboxing this document would help to protect you from any malicious (or just careless) changes that were made to the code you were calling.
If I understand the changes you are making, this wouldn't be the case.
A note that Bob's patches in bug 838692 don't take the sandboxing of the entry point into account in either the window.open(..., target) or the .click on an <a target= case. They do address navigations via names passed to window.open() or used as the target of <a> elements when a sandboxed document is also the script entry point, which is an improvement :) 

To be explicit, consider the case where a parent document P contains an unsandboxed iframe A and an iframe sandboxed with 'allow-scripts allow-same-origin' B. A contains <a href=someURL target='_top'. B obtains a reference to this anchor tag and calls click() on it - the navigation happens. Note that you need the allow-same-origin to be able to get a reference to A's <a> via P. 

Consider another case where A contains a global function that calls "window.open(someurl, '_top')". B gets a reference to A's window and calls this function - the navigation happens. Note that you also need allow-same-origin here to be able to get a reference to A's global function via P.

A note to myself also to see what IE10 and Chrome do for these cases. In general, it seems like they don't follow my current interpretation of the spec re script induced navigations - but this leads to surprising behavior at times, mostly when 'allow-same-origin' is specified, but also in some other cases (window.top.eval for example).

There's quite a few other things on Boris' list I still need to look into, any help welcome of course.
(In reply to Bob Owen from comment #19)
> 
> The document itself wouldn't, but if a part of your site was maintained by a
> different team (or third party) and you need to call functions within this
> document.
> You might think that sandboxing this document would help to protect you from
> any malicious (or just careless) changes that were made to the code you were
> calling.
> If I understand the changes you are making, this wouldn't be the case.

If the code in the sandboxed document is untrusted, I think it's a bad idea to load it with allow-same-origin from the origin of the parent page and especially to call any functions in it - I don't think this is an intended guarantee of iframe sandbox. In general, I think the purpose of iframe sandbox is intended to protect the parent page from the sandboxed document interacting with it, rather than protecting the parent page from interacting with the sandboxed document. This is all just my opinion/interpretation of the spec though :)
Ian - I think this can probably be closed now - what do you think?
Flags: needinfo?(ian.melven)
(In reply to Bob Owen (:bobowen) from comment #22)
> Ian - I think this can probably be closed now - what do you think?

Yeah, I'd say so. As I understand it the spec changes and other work you have done mean we don't need to take this approach, and this bug is INVALID :)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(ian.melven)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.