User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0 Build ID: 20150402191859 Steps to reproduce: index.html: <iframe sandbox="allow-same-origin" src="poc2.html" width="100%" scrolling="no" height="414" style=""></iframe> poc2.html: <iframe width="400" height="215" srcdoc="<META HTTP-EQUIV='refresh' CONTENT='0; URL=http://wp.pl/'>" src="#"></iframe> Actual results: Firefox run redirect when sandbox="allow-same-origin". Expected results: Firefox should block redirect when sandbox="allow-same-origin" like chrome do.

I think I see the same bug but on firefox 31 reported: https://www.mozilla.org/en-US/security/advisories/mfsa2014-66/

https://bugzilla.mozilla.org/show_bug.cgi?id=985135#c6 seems relevant.

This has nothing to do with MFSA 2014-66 which was about the redirect causing the sandbox itself to be lost. In your example the redirect happens, but the site is still sandboxed (no scripts run). The content is quite different if you remove the sandbox attribute entirely. This also has nothing to do with "allow-same-origin", I get the same results with or without that; plain sandbox does the same. Chrome spits out an error on the console when they block this: "Refused to execute the redirect specified via '<meta http-equiv='refresh' content='...'>'. The document is sandboxed, and the 'allow-scripts' keyword is not set." So Chrome is treating a declarative meta refresh equivalent to setting document.location via script. Can't fault that on logical grounds. On the other hand it also has the same effect as the user clicking on a link (though, they didn't) which is allowed. What does the spec say? Nothing, I bet. This seems more like a spec hole than a security bug.

> What does the spec say? The spec is at https://html.spec.whatwg.org/multipage/semantics.html#attr-meta-http-equiv-refresh Step 24 says: After the refresh has come due (as defined below), if the user has not canceled the redirect and if the meta element's node document's active sandboxing flag set does not have the sandboxed automatic features browsing context flag set, navigate the Document's browsing context to url, with replacement enabled, and with the Document's browsing context as the source browsing context. The "sandboxed automatic features browsing context flag" is set on a sandboxed iframe unless allow-scripts was used. See https://html.spec.whatwg.org/multipage/browsers.html#parse-a-sandboxing-directive step 3. So afaict Chrome is doing this right and we're not. One possible spec weirdness here is that the sandboxing flag is checked when the timer fires, not up front. I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=28520 on that.

thank you very much everyone for your comments, I agree 100% that can not be there js execution, is simple meta redirect, but iframe sandbox="allow-same-origin" when META redirect to different origin do you think it is not a security issue type? its breaks allow-same-origin policy.

> its breaks allow-same-origin policy. I don't think it does: allow-same-origin just means that the content _can_ end up same-origin (depending on where it's loaded from), no that it _will_ end up same-origin. So if you have, on foo.com: <iframe sandbox="allow-same-origin" src="http://bar.com"></iframe> The document in the iframe will not be same-origin with its parent.

The spec has the "automatic features" flag which only mentions autoplay media and focusing a form http://www.w3.org/TR/html5/browsers.html#sandboxed-automatic-features-browsing-context-flag but from the change comment it looks like Hixie meant to include meta refresh: https://html5.org/r/4982 (there was also whatwg mail to that effect). ... and it looks like we already know we haven't implemented this part of the spec: bug 752551

I just checked, and Chrome doesn't wait for the timer to fire to check sandbox flags.

I don't think we should conflate this with the media stuff. It can, and should, be fixed separately.

fair enough.

Ian, do you have time to fix this?

I might be able to pick this up, it might help me to keep some sort of handle on the iframe sandbox code and it looks like it will involve my old friend docshell as well. :-) One question ... it appears that Chrome still honours the Refresh HTTP header within an iframe sandbox without allow-scripts. Given that the meta refresh is supposed to be doing the equivalent of this, do you think this makes sense? I couldn't find anything in the spec about it, but I think it is not an official header. Section '4.2.5.4 Other pragma directives' somewhat implies that they should function equivalently. From a very brief look at the code I think that this will affect where the fix goes. I think keeping it consistent will probably be easier, but I'm not sure.

Hmm. Per spec, sandbox only affects the <meta> element, afaict. This makes some sense: the idea is to put content you don't trust on your server without it exploiting you, and then you control the HTTP headers. Worth raising a spec issue, but for now I'd go ahead and just do what Chrome does here.

OK, I'll try and look at this in the not too distant future.

(In reply to Not doing reviews right now from comment #14) > Hmm. Per spec, sandbox only affects the <meta> element, afaict. This makes > some sense: the idea is to put content you don't trust on your server > without it exploiting you, and then you control the HTTP headers. > > Worth raising a spec issue, but for now I'd go ahead and just do what Chrome > does here. I think control the Headers is not good in iframe sandbox. For example attacker can put into iframe sandbox META refresh redirect to site where is prepared simple HTTP AUTH BASIC https://php.net/manual/en/features.http-auth.php, then potential "protected" user by iframe sandbox will see browser alert to enter user and password and then save parameters on external origin. It can be also used to bypass sandbox="allow-forms" but with a limit (instead form use HTTP AUTH).

If you think the spec should change, please raise spec issues. This is not the right place to ask for spec changes...

I think it's unlikely that other tests will be relying on META REFRESH not being affected by sandboxing at the moment, but if they are I think they would be plain mochitests, so here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=508c62719bb4 (In reply to Not doing reviews right now from comment #14) > Hmm. Per spec, sandbox only affects the <meta> element, afaict. This makes > some sense: the idea is to put content you don't trust on your server > without it exploiting you, and then you control the HTTP headers. > > Worth raising a spec issue, but for now I'd go ahead and just do what Chrome > does here. Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=28563 to get this explicitly clarified.

This seemed like the best place to put the check. I briefly tried in HTMLMetaElement::BindToTree, but it seemed to be triggering the refresh anyway, then I found this code.

Comment on attachment 8597982 [details] [diff] [review] Part 1: Ignore META REFRESH when document is sandboxed from automatic features. '{' goes at end of if statement, not on the next line. r=me with that.

Comment on attachment 8597983 [details] [diff] [review] Part 2: Ensure that META REFRESH is blocked by iframe sandbox. >+ window.setTimeout(SimpleTest.finish, 0); This is racy. Nothing requires the subframe refreshes to have completed by then, afaict... A test testing that refreshes _do_ happen should use load events from the post-refresh pages to trigger page end. A test testing that refreshes _don't_ happen should use a longer timeout and accept that it will sometimes pass even if they _do_ happen.

Thanks for the reviews. I've fixed the { issue on the other patch locally. (In reply to Not doing reviews right now from comment #22) > Comment on attachment 8597983 [details] [diff] [review] > Part 2: Ensure that META REFRESH is blocked by iframe sandbox. > > >+ window.setTimeout(SimpleTest.finish, 0); > > This is racy. Nothing requires the subframe refreshes to have completed by > then, afaict... > > A test testing that refreshes _do_ happen should use load events from the > post-refresh pages to trigger page end. A test testing that refreshes > _don't_ happen should use a longer timeout and accept that it will sometimes > pass even if they _do_ happen. Hopefully this addresses these two points. I've gone for 500 ms for the timeout as this is more than double what the whole test took on the try push and the blocking tests are also in the first two iframes. I now remember the "fun" I had with these sorts of iframe sandbox tests. I long while ago I started to add in nsIObserverService stuff to improve this, but it got snarled in reviews and I haven't got back to it. I don't really have time to revisit that for this case, but maybe one day ... :-)

Comment on attachment 8598130 [details] [diff] [review] Part 2: Ensure that META REFRESH is blocked by iframe sandbox. v2 >+ ok(true, msg); How about: ok(numberOfPasses < numberOfPassesExpected, msg); ? Also, using onload handlers here is a bit weird, since it assumes onload won't fire on the pre-refresh document. I think it would be more robust to have the post-refresh document postMessage the parent... except that doesn't work without allow-scripts. :( I guess we can do the onload thing and just hope that the not-firing-on-pre-refresh-document bit is required by the spec. Maybe at least allow-same-origin those subframes that should in fact redirect and check that onload the URI is correct?

(In reply to Not doing reviews right now from comment #24) > Comment on attachment 8598130 [details] [diff] [review] > Part 2: Ensure that META REFRESH is blocked by iframe sandbox. v2 > Also, using onload handlers here is a bit weird, since it assumes onload > won't fire on the pre-refresh document. I think it would be more robust to > have the post-refresh document postMessage the parent... except that doesn't > work without allow-scripts. :( I guess we can do the onload thing and just > hope that the not-firing-on-pre-refresh-document bit is required by the > spec. Maybe at least allow-same-origin those subframes that should in fact > redirect and check that onload the URI is correct? Actually, I was relying on the pre-refresh load firing, as the first onload resets itself to a reporting function. Also, allow-same-origin wouldn't be enough because I'm using data URLs and it seems like relative URLs don't work in meta refreshes on Firefox in this case, possibly because I'm using srcdoc (this may be a separate bug). According to the spec the pre-refresh iframe load should always fire when the document is "completely loaded" (there is a case where it won't, but that involves document.open). However, it says it should run in parallel to other things that follow from "completely loaded", which includes the meta refresh timer. So, I then became concerned that in some extreme circumstance the post-refresh onload could run before the pre-refresh onload had changed it. I don't know enough about JavaScript's execution model to know if that could ever happen, but anyway round what I was doing clearly wasn't obvious. So, I've re-written it in a way that just records the loads as they happen and then checks the results at the end, after a short delay. I think this is generally clearer and the only thing I am relying on now is that the pre-refresh load events are always fired (which should be true as per spec). Sorry to take up more of your not doing reviews time. :-(

Comment on attachment 8598598 [details] [diff] [review] Part 2: Ensure that META REFRESH is blocked by iframe sandbox. v3 Yes, this is much nicer! r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/207302d04e83 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca5370dae3e

https://hg.mozilla.org/mozilla-central/rev/207302d04e83 https://hg.mozilla.org/mozilla-central/rev/8ca5370dae3e