<iframe sandbox src="something-that-redirects"> doesn't sandbox because we do sandboxing via setting a nullprincipal owner and redirects don't propagate it. I'll attach a mochitest that shows the bug.... Properly fixing bug 965413 will automagically fix this, but we may want some sort of branch backports.

Er, didn't mean to take this.

Boris, can you please answer this separately for these items? - HTTP redirects (301, 302, ..) - META tag redirects - JavaScript redirects (assignments to location)

Bob is our new Sandboxing wizard. Bob, can you have a look at this, possibly after Boris lands bug 965413?

This is specific to redirects on the necko level. So this affects HTTP redirects, but also as far as I can tell proxy stuff (!) and a few other esoteric cases that shouldn't come up much in practice (.url files for file:// loads, say). For the branch backports, I think we basically need to propagate the .owner in SetupReplacementChannel (perhaps only if it's a null principal?).

Oh, and to be clear, <meta> and location sets aren't redirects of an existing load. They're new loads. This bug only affects redirects of existing loads.

(In reply to Bobby Holley (:bholley) from comment #4) > Bob is our new Sandboxing wizard. Bob, can you have a look at this, possibly > after Boris lands bug 965413? Sure, I'll add it to the list (although perhaps not at the bottom :-) ). Just for clarity, I think it is the allow-same-origin part of the sandbox that is broken, as this is what relies on the nullprincipal (possibly some of the other flags as well). Did a quick test and the navigation and script restrictions still seem to be in place, which makes sense. Of course that's not much comfort, because if allow-scripts is there, then this issue would allow someone to remove the sandbox completely.

> Just for clarity, I think it is the allow-same-origin part of the sandbox that is broken, Correct.

This is bz's original test, with just the bug numbers in the main test file changed to this one.

This just propagates the owner, when it is a null principal, as per Boris's suggestion in comment 5. I think it is safest to only do this for null principals, as if we always propagate then it might have unforeseen consequences.

Comment on attachment 8430063 [details] [diff] [review] When owner is a null principal, propagate to replacement channel on redirect v1 r=me

Comment on attachment 8430057 [details] [diff] [review] Test that sandboxed origin flag is honoured across redirects. Review of attachment 8430057 [details] [diff] [review]: ----------------------------------------------------------------- I doubt that you meant this to be the actual checked-in test for this bug, but I thought, why reinvent the cliché. :) The comments below are all very minor, but I thought I ought to look at it fairly critically, it being my first review. ::: content/html/content/test/file_sandbox_redirect.html^headers^ @@ +1,2 @@ > +HTTP 301 Moved Permanently > +Location: http://mochi.test:8888/tests/content/html/content/test/file_sandbox_redirect_target.html Would it be better to use a relative URL, in case the origin and path ever change? Although doing a quick search for mochi.test:8888 on dxr reveals that whole swathes of tests would fail, were this ever to change. ::: content/html/content/test/mochitest.ini @@ +146,5 @@ > file_iframe_sandbox_worker.js > file_imports_basics.html > + file_sandbox_redirect.html > + file_sandbox_redirect.html^headers^ > + file_sandbox_redirect_target.html nit: I think, for consistency with other tests, these (and the test file below) should be called *_iframe_sandbox_redirect_* ::: content/html/content/test/test_sandbox_redirect.html @@ +15,5 @@ > + addLoadEvent(function() { > + try { > + var doc = frames[0].document; > + ok(false, "Should not be able to get the document"); > + isnot(doc.body.textContent, "I have been redirected\n", "Should not happen"); nit: this |\n| causes the message in the log to bleed onto the next line. We could slice the last character off to avoid this.

I should have pointed out in comment 13, that Boris (on IRC) asked me to review the test, as it was originally written by him, although I'd made a cosmetic change to the bug number.

> I doubt that you meant this to be the actual checked-in test for this bug I actually did; that's why I wrote it up as a mochitest. ;) > Would it be better to use a relative URL, in case the origin and path ever change? Per spec, the value of a Location header is an absolute URI... I guess we do make it relative to the channel URI, so we could do that here. > should be called *_iframe_sandbox_redirect_* Makes sense. > We could slice the last character off to avoid this. Also makes sense. Are you going to update the test to those comments, or do you want me to?

Updated on behalf of Boris as per comment 15 and r+ed again by me. (In reply to Boris Zbarsky [:bz] from comment #15) > Per spec, the value of a Location header is an absolute URI... Well, I didn't know that ... every day's a school day. :) I did check that my suggestion worked before I made it though.

Full Linux try push with this patch and the previous version of the test: https://tbpl.mozilla.org/?tree=Try&rev=61be6ab794e8 I also did another quick try push, with just the relevant tests, for my changes in comment 16, just to make sure I hadn't broken things: https://tbpl.mozilla.org/?tree=Try&rev=0df0ee7604c3 It makes sense to land the fix before the test. Thanks.

Landed minus the test for now (though for a sec-moderate, we probably could have just gone ahead and landed it). https://hg.mozilla.org/integration/mozilla-inbound/rev/734a7a31ee9c

https://hg.mozilla.org/mozilla-central/rev/734a7a31ee9c Per IRC, setting checkin-needed for the test. How far back does this issue go, Bob?

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > https://hg.mozilla.org/mozilla-central/rev/734a7a31ee9c > > Per IRC, setting checkin-needed for the test. How far back does this issue > go, Bob? Thanks Ryan, and sorry for the confusion. I don't think anything has changed in the channel / redirection code recently to cause this, so I think this has probably been there since iframe sandbox landed (FF17 if my memory serves me well).

Thanks for the reply. It's unlikely this will be taken for esr24 since it is only sec-moderate. You can try to argue for it if you feel strongly about it, though. Otherwise, please nominate this for Aurora/b2g30/b2g28 approval when you get a chance.

Comment on attachment 8430063 [details] [diff] [review] When owner is a null principal, propagate to replacement channel on redirect v1 (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22) > Thanks for the reply. It's unlikely this will be taken for esr24 since it is > only sec-moderate. You can try to argue for it if you feel strongly about > it, though. Otherwise, please nominate this for Aurora/b2g30/b2g28 approval > when you get a chance. No problem. There's at least one other bug, that allowed you to escape the sandbox completely, that didn't get uplifted to esr24. So, unless they were all uplifted it doesn't really make sense to do this one on its own. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 341604, as I believe this issue will have existed since the feature was landed. User impact if declined: Can be used to remove the unique origin enforced on sandboxes without the allow-same-origin keyword. This normally prevents the sandboxed content from accessing other content from the same origin. Also, if the allow-scripts keyword is present, would allow same origin sandboxed content to remove the sandbox altogether. Testing completed (on m-c, etc.): Automated mochitest, no QA testing as yet. Risk to taking this patch (and alternatives if risky): Low risk - it is a small single area patch and the propagation of the owner is only being done when it is a null principal. String or IDL/UUID changes made by this patch: None

Comment on attachment 8430990 [details] [diff] [review] Test that sandboxed origin flag is honoured across redirects. v3 This is just the automated test for the uplift requested in comment 23.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22) > Thanks for the reply. It's unlikely this will be taken for esr24 since it is > only sec-moderate. You can try to argue for it if you feel strongly about > it, though. Otherwise, please nominate this for Aurora/b2g30/b2g28 approval > when you get a chance. Given this is a sec-moderate only and an old regression, we can let this ride the trains and get naturally fixed in 2.0 (gecko base 32) without uplifting on b2g branches.. If anyone disagrees and I missed any context why this would be neeeded for B2G, please NI me.

I used the tests within to observe the issue on Fx30, as well as pre-fix builds of Fx31. I verified fixed on builds of Fx31 and Fx32, 2014-07-07.