+++ This bug was initially created as a clone of Bug #849791 +++ Should <object>s containing HTML content--that is, rendered by us rather than plugin content--be considered equivalent to an <iframe> for sandbox purposes? Bug 849791 was about evading iframe sandbox restrictions by using a nested <frameset>, but you can accomplish the same evasion using <object> The following runs script data:text/html,<iframe%20sandbox%20src="data:text/html,<object%20data='data:text/html,<script>alert(parent.document.location)</script>'></object>"></iframe> I expected it to behave like the following which doesn't: data:text/html,<iframe%20sandbox%20src="data:text/html,<iframe%20src='data:text/html,<script>alert(parent.document.location)</script>'></iframe>"></iframe> If the sandbox has allow-same-origin set then the scripts in the object can interact with the top-level document.

Yes, they should be equivalent.

Chrome behaves as expected, blocking scripts in <option> the same as in the nested <iframe>; remove the sandbox attribute and the script executes. To verify in Chrome change the script slightly because data: urls don't inherit their origin and "parent." causes an exception. In Chrome try with alert(document.location) or alert(/FAIL/) instead.

Freddy, could you cc me on 897524 if that's alright ?

Adding :asuth and :pauljt as this severely impacts b2g security.

(In reply to Ian Melven :imelven from comment #3) > Freddy, could you cc me on 897524 if that's alright ? Added you on the other side as well.

imelven, will you still be working on this? I'd prefer if we don't miss this..

Do we need to be tracking this for any particular branches?

It would be nice to have this into b2g asap since CSP is a key mitigation for email app (and the email app use iframe-sandbox as well as CSP) (see blocked bug - 897524). I'm not sure what gecko version we are taking for koi (26?) so maybe we want to be tracking this version?

My understanding is that imelven is not working on this. johns do you want it?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #9) > My understanding is that imelven is not working on this. johns do you want > it? It's my understanding that the way nsObjectLoadingContent loads frames just bypasses a lot of nsDocShell, including these security checks. I know next to nothing about how all the docshell/uriloader/etc stuff works, so I'm willing to take a shot at this, but might not be the best person if we want this fixed quickly.

I'll try to find time for this.

(In reply to Frederik Braun [:freddyb] from comment #6) > imelven, will you still be working on this? I'd prefer if we don't miss > this.. I will likely not have free time to work on a fix for this for at least another month or so. Bob Owen also pinged me about this bug yesterday and said he thinks he has a fix for this and will take a look at it. I'm happy to answer any questions about the iframe sandbox stuff or look over a potential fix for this bug. Feel free to ping me via email directly.

Adding Andrew since this issue affects b2g email sanitization.

Comment on attachment 783470 [details] [diff] [review] Propagate the sandbox flags through in nsObjectLoadingContent. Ian, is the observed behavior in that test correct?

As Ian said in Comment 12, I thought I had a fix for this. I moved the sandbox flag setting in nsFrameLoader.cpp into MaybeCreateDocShell(), which seems like the correct place for it anyway. This way the flags get set whenever the docshell is created. It passed the original tests in the Description, but not the ones I had written. bz - it passes your tests as well, but your fix doesn't pass my tests, so I suspect I've messed up the tests somewhere. I'll upload my fix and tests as an attachment in case they help. Cheers, Bob

Propagating sandbox flags in MaybeCreateDocShell doesn't seem like it would correctly handle an <iframe> being navigated after its @sandbox changes. I guess we could set the flags both in MaybeCreateDocShell and in the URI-loading code in nsFrameManager, though.

(In reply to Boris Zbarsky (:bz) from comment #17) > Propagating sandbox flags in MaybeCreateDocShell doesn't seem like it would > correctly handle an <iframe> being navigated after its @sandbox changes. > > I guess we could set the flags both in MaybeCreateDocShell and in the > URI-loading code in nsFrameManager, though. Ah ... yes perhaps it should be in both places, however I thought there were some tests for this already and I've just ran the all the sandbox tests and only my new ones were failing.

Comment on attachment 783470 [details] [diff] [review] Propagate the sandbox flags through in nsObjectLoadingContent. So I assume we need to update the flag in both places.

Comment on attachment 783470 [details] [diff] [review] Propagate the sandbox flags through in nsObjectLoadingContent. So I assume we need to update the flag in both places. And this just copies flags from parent.

(In reply to Olli Pettay [:smaug] from comment #22) > And this just copies flags from parent. Oh, right, the usual spec inconsistency. sandbox attribute is only on iframe.

> Oh, right, the usual spec inconsistency. sandbox attribute is only on iframe. Yes, indeed. So we don't have to update it on navigation either, because it can't change unless the parent is navigated, at which point our element dies anyway.

Bob, I'll try to look at your stuff later today.

Not using firefox tracking flags since it looks like this is only wanted for b2g tracking and the koi? nom is enough to keep that on the radar for v1.2 timeframe.

Hmm. Looks like we explicitly set the sandbox flags on the docshell when @sandbox is changed in HTMLIFrameElement::AfterSetAttr. Given that, just setting it up front in docshell creation in the frameloader seems fine, and would fix this bug indeed. Bob, want to steal the bug?

(In reply to Boris Zbarsky (:bz) from comment #27) > Hmm. Looks like we explicitly set the sandbox flags on the docshell when > @sandbox is changed in HTMLIFrameElement::AfterSetAttr. > > Given that, just setting it up front in docshell creation in the frameloader > seems fine, and would fix this bug indeed. > > Bob, want to steal the bug? I'm happy to give it a go, I should have a bit of free time over the next few days. Now I just have to work out why my tests still aren't fixed by this code change, but ones using data URIs are. Cheers, Bob

Sorry, I'm seem to keep setting/unsetting flags by accident.

Had a bit of time to look at this last night. I think there is a problem in test_bug886262.html where e.data.gotSet is being checked, it should be !e.data.gotSet. Also gotSet2 is checked, but I can't see where it is set apart from its declaration. With both fixes, the flags seem to be being copied correctly, but I'm not sure the nullprincipal is getting set correctly for the SANDBOXED_ORIGIN flag. I think this is why my tests are failing. It looks like nsObjectLoadingContent.cpp sets the principal differently. Haven't had time to prove this yet, just been looking through the code. Also, I don't think we can rely on the current code in HTMLIFrameElement::AfterSetAttr. It doesn't appear to merge the new sandbox flags from the attribute with the owning document's flags. I don't think there is a test for this (changing sandbox attributes when already within a sandbox), but it may be being masked by the fact that currently the code in nsFrameLoader::ReallyStartLoadingInternal gets the flags from both every time. I'm tempted to add a function to nsFrameLoader called ApplySandboxFlags (or something) this could parse the flags and merge with the owning document's flags. It could be called by nsFrameLoader::MaybeCreateDocShell and HTMLIFrameElement::AfterSetAttr. Anyway I should have some time when I get back from work and over the weekend, to try these things out. Cheers, Bob

Not sure of the protocol here. bz, I assume by the fact that you said 'steal the bug', I just re-assign it to myself.

> I think there is a problem in test_bug886262.html where e.data.gotSet is being checked, > it should be !e.data.gotSet. That's one of the things I wanted Ian to check over, actually. The message is wrong, because initially I _did_ in fact check !e.data.gotSet but that turns out to not be the case here. Presumably because these are data: loads and hence end up inheriting the parent principal or something? Basically, the sandbox flags are definitely ending up on the child docshell, but the resulting behavior doesn't make that much sense to me. But I can't tell from the spec how it _should_ work. > Also, I don't think we can rely on the current code in HTMLIFrameElement::AfterSetAttr. > It doesn't appear to merge the new sandbox flags from the attribute with the owning > document's flags. Er, that seems like a flat-out bug! Nice catch. > It looks like nsObjectLoadingContent.cpp sets the principal differently. Hmm. That's because the null-principal bits are in nsDocShell::DoURILoad, I guess, which is never hit in this case. So we need to deal with that in either LoadObject or centrally in nsURILoader::OpenChannel or something. Again, good catch. It sounds like you have a much better idea of what's going on here than I do. ;) And yes, by 'steal the bug' I meant assign to yourself.

(In reply to Boris Zbarsky (:bz) from comment #32) > It sounds like you have a much better idea of what's going on here than I > do. ;) Well I'm sure that's not true, but I had a bit of time yesterday, in between cutting hedges in the garden, and I think I have a fix for this. It passes my and your tests and I've run all the tests under content/. The only ones that fail are for webgl, which I don't think are supported on my debian VM. I'm hoping to have some more time later to add more tests, including one for the issue in HTMLIFrameElement::AfterSetAttr, so I should get the patch uploaded today.

Pinched tests from test_bug886262.html that weren't already covered. Also added test for a change to the sandbox attribute for an iframe that is within another iframe. This was failing when the subsequent navigation was from within the iframe, presumably because this was all handled within the docshell and didn't go back up to the nsFrameLoader. Fix patch to follow.

In nsFrameLoader, set the sandbox flags as soon as the mDocshell is created. In nsObjectLoadingContent, set a null principal when owning document is sandboxed without allow-same-origin. Correct code in HTMLIFrameElement::AfterSetAttr to make sure flags are copied from owning document when sandbox attribute is changed.

Limited try run: https://tbpl.mozilla.org/?tree=Try&rev=2e711e73da02

Added allowance for assertion. New try run: https://tbpl.mozilla.org/?tree=Try&rev=b6718736ae17

Comment on attachment 785658 [details] [diff] [review] Fix for Bug 886262: Ensure sandbox flags and, where necessary, null principal are set for child objects. >+nsFrameLoader::ApplySandboxFlags(uint32_t sandboxFlags) { Curly on next line, please. r=me; I really like this setup!

Comment on attachment 785922 [details] [diff] [review] Tests for Bug 886262: HTML <object>s do not inherit sandbox flags from their parents. >+SimpleTest.expectAssertions(1, 1); SimpleTest.expectAssertions(1); and document which assertions this is expecting (ideally with bug number), please.

(In reply to Boris Zbarsky (:bz) from comment #38) > Comment on attachment 785658 [details] [diff] [review] > Fix for Bug 886262: Ensure sandbox flags and, where necessary, null > principal are set for child objects. > > >+nsFrameLoader::ApplySandboxFlags(uint32_t sandboxFlags) { > > Curly on next line, please. > > r=me; I really like this setup! Thanks for the review Boris. Corrected curly brace placement.

Comment on attachment 786299 [details] [diff] [review] Fix for Bug 886262: Ensure sandbox flags and, where necessary, null principal are set for child objects. r=me. Thanks!

(In reply to Boris Zbarsky (:bz) from comment #39) > Comment on attachment 785922 [details] [diff] [review] > Tests for Bug 886262: HTML <object>s do not inherit sandbox flags from their > parents. > > >+SimpleTest.expectAssertions(1, 1); > > SimpleTest.expectAssertions(1); > > and document which assertions this is expecting (ideally with bug number), > please. Corrected function call and added comment about the assertion, including a new bug 901876. There was already bug 855443 for a test causing this assertion to fail, but it seemed like quite a specific case in the description, so I wasn't sure whether they were the same thing.

New very limited try run, just to make sure I haven't broken anything: https://tbpl.mozilla.org/?tree=Try&rev=1d4881390d8c

https://hg.mozilla.org/integration/mozilla-inbound/rev/10df319c639d https://hg.mozilla.org/integration/mozilla-inbound/rev/17221c8fa982

https://hg.mozilla.org/mozilla-central/rev/10df319c639d https://hg.mozilla.org/mozilla-central/rev/17221c8fa982

Confirmed script execution using code samples from comment 0, using affected FF23 plus pre-patch m-c. Verified fixed in m-c 2013-08-29.

Thanks for cleaning up my mess again (and fixing this bug), Bob. Sorry I missed your earlier questions as well, Boris - I missed the r? notification since this is a secure bug :(