Daniel Veditz [:dveditz] Reporter
	Description • 11 years ago

+++ 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.

	Benjamin Smedberg
	Comment 1 • 11 years ago

Yes, they should be equivalent.

	Daniel Veditz [:dveditz] Reporter
	Comment 2 • 11 years ago

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.

	Ian Melven :imelven
	Comment 3 • 11 years ago

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

	Frederik Braun [:freddy]
	Comment 4 • 11 years ago

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

	Frederik Braun [:freddy]
	Comment 5 • 11 years ago

(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.

	Frederik Braun [:freddy]
	Comment 6 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

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

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 8 • 11 years ago

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?

	Benjamin Smedberg
	Comment 9 • 11 years ago

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

	John Schoenick [:johns]
	Comment 10 • 11 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 11 years ago

I'll try to find time for this.

	Ian Melven :imelven
	Comment 12 • 11 years ago

(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.

	Ian Melven :imelven
	Comment 13 • 11 years ago

Adding Andrew since this issue affects b2g email sanitization.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 11 years ago

Comment on attachment 783470 [details] [diff] [review]
Propagate the sandbox flags through in nsObjectLoadingContent.

Ian, is the observed behavior in that test correct?

	Bob Owen (:bobowen) Assignee
	Comment 16 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 20 • 11 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 22 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 11 years ago

> 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.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 11 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 26 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 11 years ago

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?

	Bob Owen (:bobowen) Assignee
	Comment 28 • 11 years ago

(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

	Bob Owen (:bobowen) Assignee
	Comment 29 • 11 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 30 • 11 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 31 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 11 years ago

> 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.

	Bob Owen (:bobowen) Assignee
	Comment 33 • 11 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 34 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 35 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 36 • 11 years ago

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

	Bob Owen (:bobowen) Assignee
	Comment 37 • 11 years ago

Added allowance for assertion.

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

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 11 years ago

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!

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 11 years ago

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.

	Bob Owen (:bobowen) Assignee
	Comment 40 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 11 years ago

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!

	Bob Owen (:bobowen) Assignee
	Comment 42 • 11 years ago

(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.

	Bob Owen (:bobowen) Assignee
	Comment 43 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

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

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 46 • 11 years ago

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.

	Ian Melven :imelven
	Comment 47 • 11 years ago

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 :(