Last Comment Bug 797909 - Docshell sandbox flags should apply to initial about:blank
: Docshell sandbox flags should apply to initial about:blank
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Olli Pettay [:smaug]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-10-04 09:43 PDT by Louis-Rémi BABE
Modified: 2012-10-15 13:21 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test-case (567 bytes, text/html)
2012-10-04 10:21 PDT, Louis-Rémi BABE
no flags Details
patch (3.61 KB, patch)
2012-10-04 15:10 PDT, Olli Pettay [:smaug]
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Louis-Rémi BABE 2012-10-04 09:43:01 PDT
There are two use-cases where people might want to access/modify the sandbox attribute of an iframe before inserting it into the document:
- when trying to detect if the browser supports that feature (as feature-detection scripts are usually in the <head> of the document and the body isn't accessible)
- when trying to create an iframe that is sandboxed from start

In Firefox, the sandbox attribute cannot be accessed or modified before the iframe is inserted into the document.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-10-04 09:44:44 PDT
Testcase please.
Comment 2 Louis-Rémi BABE 2012-10-04 10:05:29 PDT
Writing the test-case made me realize
Comment 3 Louis-Rémi BABE 2012-10-04 10:20:58 PDT
Oops, I inadvertently submitted the last comment by pressing "Enter" while editing the title, sorry about that.

Writing the test-case made me realize that accessing the attribute was actually possible (I'm pretty sure it wasn't possible a few days ago but anyway), but modifying it isn't.

Opening the attached test case in Firefox will display a text that has been inserted into the a sandboxed iframe, while opening the same test case in Chrome won't display any text but will cause sandbox access violation errors.

In practice this doesn't cause any security problem, it just made me think that it was possible to access the sandboxed iframe content from the parent document when the iframe wasn't sandboxed at all.
Comment 4 Louis-Rémi BABE 2012-10-04 10:21:33 PDT
Created attachment 668056 [details]
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-10-04 12:43:04 PDT
So the issue is that we're allowing access to the sandboxed iframe, which should be using a nullprincipal, right?

That seems to be because we don't sandbox the initial about:blank?  We should.

There were comments about this in bug 341604 that (incorrectly, it seems) concluded there wasn't a problem here.

Requesting tracking, since this seems like a bad bug to ship...
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-10-04 12:43:27 PDT
jst, do you know when imelven gets back?
Comment 7 Olli Pettay [:smaug] 2012-10-04 13:44:29 PDT
Hmm, we do set the sandbox flags to initial about:blank.
Why is that not working
Comment 8 Olli Pettay [:smaug] 2012-10-04 13:44:54 PDT
Ah, principal
Comment 9 Olli Pettay [:smaug] 2012-10-04 15:10:38 PDT
Created attachment 668200 [details] [diff] [review]
Comment 10 Olli Pettay [:smaug] 2012-10-04 15:31:23 PDT
Still trying to push to try. It is very very slow atm.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-10-04 15:47:52 PDT
Comment on attachment 668200 [details] [diff] [review]

Comment 12 Olli Pettay [:smaug] 2012-10-04 16:10:23 PDT
Comment 13 Johnny Stenback (:jst, 2012-10-04 16:16:28 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> jst, do you know when imelven gets back?

I'm told he's back on the 15th.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-05 16:27:12 PDT
Tracking based on comment 5, also updating status and milestone for 18 to help with proper triage searching.  Please nominate for uplift - if this doesn't get in before Monday October 8th on Aurora it will need to be nominated for beta.
Comment 15 Olli Pettay [:smaug] 2012-10-05 16:34:20 PDT
Comment on attachment 668200 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 341604
User impact if declined: sandbox doesn't work the way it should
Testing completed (on m-c, etc.): landed to m-c
Risk to taking this patch (and alternatives if risky): should be low risk
String or UUID changes made by this patch: NA
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-05 16:56:33 PDT
Comment on attachment 668200 [details] [diff] [review]

thanks - please land before Monday Oct 8th merge day.
Comment 18 Ian Melven :imelven 2012-10-15 13:21:00 PDT
Sorry about this, thanks for handling it Olli !

Note You need to log in before you can comment on or make changes to this bug.