Tanvi Vyas[:tanvi] Reporter
	Description • 9 years ago

There have been some improvements proposed to iframe sandbox.
https://wiki.whatwg.org/wiki/Iframe_sandbox_improvments
https://lists.w3.org/Archives/Public/public-whatwg-archive/2015Jul/0006.html

I believe the reasoning behind these improvements is to make iframe sandbox usable for framed advertisements.  If a user interacts with an ad and clicks on it, it should be able to open up as a top level page in a new tab without being sandboxed.

	Bob Owen (:bobowen)
	Comment 1 • 9 years ago

I think that allow-popups-to-escape-sandbox, should be pretty straightforward to implement.

I think just a matter of adding it to the flags [1], parsing [2] and then checking for it at [3].
Although it sounds like we would want to keep the setting of the one permitted sandboxed navigator at [3], assuming that part of the proposal gets agreement.
I might have missed something, but searching through the allow-popups (SANDBOXED_AUXILIARY_NAVIGATION) code should find anything else.

Tests will probably take longer. :-)

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsSandboxFlags.h
[2] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/dom/base/nsGkAtomList.h#l83
and https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/dom/base/nsContentUtils.cpp#l1321 
[3] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/embedding/components/windowwatcher/nsWindowWatcher.cpp#l81

I've not read all the linked information, but it's not entirely clear to me whether allow-popups-to-escape-sandbox implies allow-popups, or whether you would have to specify both flags.

Not so sure about allow-modal, off the top of my head.
But that sounds like it is something to improve security.
Not sure whether it should cover showModalDialog, which is currently covered by allow-popups.
Seems a bit confusingly named otherwise.

Also, maybe allow-modal should be in a separate bug.

	Bob Owen (:bobowen)
	Comment 2 • 9 years ago

(In reply to Bob Owen (:bobowen) from comment #1)

> [3]
> https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/embedding/
> components/windowwatcher/nsWindowWatcher.cpp#l81

Whoops, I seem to have lost a 3 there, that should have been:
[3] https://hg.mozilla.org/mozilla-central/file/2ddec2dedced/embedding/components/windowwatcher/nsWindowWatcher.cpp#l813

	Mike West
	Comment 3 • 9 years ago

This landed in the HTML spec, by the way, at [1] and [2], which I hope will clarify the implementation questions you've raised. In short, yes, you still need `allow-popups` (as UAs that don't support this flag will continue to require it):

[1]: https://html.spec.whatwg.org/multipage/browsers.html#sandbox-propagates-to-auxiliary-browsing-contexts-flag
[2]: https://html.spec.whatwg.org/multipage/browsers.html#sandboxed-modals-flag

	Erik Dubbelboer
	Comment 4 • 9 years ago

Is there any update on this? We at https://www.atomx.com/ are very interested in this development.

	Tanvi Vyas[:tanvi] Reporter
	Comment 5 • 9 years ago

(In reply to Erik Dubbelboer from comment #4)
> Is there any update on this? We at https://www.atomx.com/ are very
> interested in this development.

It is currently on our list, but not prioritized.  If you could share your use case and the demand for this feature, that could help.  Thanks!

	Phil Freo
	Comment 6 • 8 years ago

+1 for implementing "allow-popups-to-escape-sandbox" in Firefox. Firefox is the only major browser where clicking the Blue box link on https://googlechrome.github.io/samples/allow-popups-to-escape-sandbox/ opens a page where JavaScript cannot be run. 

There are lots of cases where displaying untrusted (sandboxed) content within a parent page may be desired (rendering advertisements, HTML email, etc.) where the desired behavior of opening a link in a new tab is that the new page has full non-sandboxed permissions since it's on a separate domain and can't break out of the iframe.

	Bin Lu
	Comment 7 • 8 years ago

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> (In reply to Erik Dubbelboer from comment #4)
> > Is there any update on this? We at https://www.atomx.com/ are very
> > interested in this development.
> 
> It is currently on our list, but not prioritized.  If you could share your
> use case and the demand for this feature, that could help.  Thanks!

AFAIK, publishers and ads networks are actively looking for ways to prevent auto-redirecting from bad ads , and one (maybe the best) option is to disallow top-level-navigation by putting ads inside sandboxed iframes. But without the "allow-popups-to-escape-sandbox" support, the ads landing page (not the ads itself) with plugins (like Flash, Java, Silverright) might break due the propagated sandboxed flags, which somehow prevents publishers/ad networks to sandbox ads on desktop.

So it would be great if Firefox could support it.

	Erik Dubbelboer
	Comment 8 • 8 years ago

This is indeed our main use case as well. As you may have read ad fraud is a big problem in the advertising space at the moment. Malicious ads that take over the page the are loaded in are a big problem. Placing these ads in sandboxed iframes would mitigate a lot of this malicious behavior. But as it is implemented at the moment the inheritance of the sandbox attribute on popups windows prevents us from using the sandbox attribute at all.

So in conclusion, implementing "allow-popups-to-escape-sandbox" would allow us and other exchanges to block a lot more malicious ads.

	Christoph Kerschbaumer [:ckerschb]
	Comment 9 • 8 years ago

Triaging at the moment, we need someone to take a look at this - putting it in the backlog for now.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 8 years ago

Automated testing for this seems rather difficult; I've done manual testing.

	Christoph Kerschbaumer [:ckerschb]
	Comment 14 • 8 years ago

Comment on attachment 8749498 [details] [diff] [review]
part 1.  Reorder nsSandboxFlags.h to match the spec order better so it's easier to tell where they diverge

Bob is the better reviewer for those bits.

	Bob Owen (:bobowen)
	Comment 15 • 8 years ago

Comment on attachment 8749500 [details] [diff] [review]
part 2.  Add the sandboxed modals flag to iframe sandboxing

Review of attachment 8749500 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of questions for my own understanding really.
I'm not familiar with this part.

::: dom/base/nsGlobalWindow.cpp
@@ +3425,5 @@
> +  // spec is daft.  See <https://github.com/whatwg/html/issues/1206>.  For now
> +  // just go ahead and check mDoc, since in everything except edge cases in
> +  // which a frame is allow-same-origin but not allow-scripts and is being poked
> +  // at by some other window this should be the right thing anyway.
> +  if (!mDoc || (mDoc->GetSandboxFlags() & SANDBOXED_MODALS)) {

So , this is the doc of the relevant settings object?
This has all moved on a bit since I last looked at it. :-)

Either way it seems the right thing to use, I guess.

I've not seen AreDialogsEnabled before, but it seems to be used by all the things we want to block here.

::: layout/base/nsDocumentViewer.cpp
@@ +1150,5 @@
>  
>    // NB: we nullcheck mDocument because it might now be dead as a result of
>    // the event being dispatched.
> +  if (!sIsBeforeUnloadDisabled && *aShouldPrompt && dialogsAreEnabled &&
> +      mDocument && !(mDocument->GetSandboxFlags() & SANDBOXED_MODALS) &&

Is this a different document's flags than will have been checked when dialogsAreEnabled is set by |dialogsAreEnabled = globalWindow->AreDialogsEnabled();| above?

	Bob Owen (:bobowen)
	Comment 16 • 8 years ago

Comment on attachment 8749501 [details] [diff] [review]
part 3.  Add the sandbox propagates to auxiliary browsing contexts flag to iframe sandboxing

Review of attachment 8749501 [details] [diff] [review]:
-----------------------------------------------------------------

I've not seen web platform tests before, but I follow what they're doing, even if I don't understand the harness bits.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 8 years ago

> So , this is the doc of the relevant settings object?

It's not quite, because we're on the outer.  It's the current document of the browsing context of the relevant settings object.  Again, once the spec gets sorted out here I expect this part to change.  If we stick with relevant settings object, I'll need to move this check into the inner-window methods involved.  If the spec goes with incumbent object, then we can leave the check here.

I agree that what I have right now is pretty weird, just like the current spec language.  If you prefer, I will move this check into the inner window methods for now...  This mostly matters for bareword calls, of course; anything qualified like window.alert() will be going through the current document anyway.

> I've not seen AreDialogsEnabled before, but it seems to be used by all the things we want to block here.

Yeah, I did check that it's called from precisely the right places... in nsGlobalWindow code.

> Is this a different document's flags than will have been checked when dialogsAreEnabled is set by
> |dialogsAreEnabled = globalWindow->AreDialogsEnabled();| above?

Oh, good question!  globalWindow came from mDocument->GetWindow() and is an outer.  So the AreDialogsEnabled() call there used the sandbox flags of the current document of that outer, which better be mDocument in this case, since we're just unloading now.  So no, this is the same document's flags.

I could replace this bit with an assert if we keep the check in AreDialogsEnabled(), but I'm a bit happier having it explicit, given the uncertainty above about whether the window code will check sandboxing in AreDialogsEnabled long-term.

	Bob Owen (:bobowen)
	Comment 18 • 8 years ago

Just following up from our IRC exchange.

(In reply to Boris Zbarsky [:bz] from comment #17)
 
> I agree that what I have right now is pretty weird, just like the current
> spec language.  If you prefer, I will move this check into the inner window
> methods for now...  This mostly matters for bareword calls, of course;
> anything qualified like window.alert() will be going through the current
> document anyway.

We agreed to stick with this until the spec is clarified.

> I could replace this bit with an assert if we keep the check in
> AreDialogsEnabled(), but I'm a bit happier having it explicit, given the
> uncertainty above about whether the window code will check sandboxing in
> AreDialogsEnabled long-term.

We agreed explicit is better here.

	Pulsebot
	Comment 19 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/733eacd2ed13
https://hg.mozilla.org/integration/mozilla-inbound/rev/b037f2642082
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1d1a433415

	Bob Owen (:bobowen)
	Comment 20 • 8 years ago

In case you miss the IRC post:

"I think if we fixed Bug 960545, then this would cause a problem without one permitted sandboxed navigator, although the wording (incumbent) on that bug might be wrong now"

But I agree we're doing what the spec says, even if you followed the spec to the letter for the subsequent navigation it would fail.

I wish there was a way of setting "for info" instead of "needinfo".

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 8 years ago

This caused some test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=27394820&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=27396526&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f168a1623065

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 8 years ago

Oops, accidentally backed out the wrong three-commit push. Actual backout in https://hg.mozilla.org/integration/mozilla-inbound/rev/933b5260480f

	Pulsebot
	Comment 23 • 8 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be22142cf4

	Tanvi Vyas[:tanvi] Reporter
	Comment 24 • 8 years ago

Comment on attachment 8749500 [details] [diff] [review]
part 2.  Add the sandboxed modals flag to iframe sandboxing

Thanks for taking this bug Boris!  If I'm reading the below code correctly, it appears that in the current codebase we allow modals for all sandboxed frames.  Is that correct?  The places in the code below where we add a SANDBOXED_MODALS flag, we previously didn't check if the page had a nullprincipal.


>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -3415,16 +3415,26 @@ nsGlobalWindow::AreDialogsEnabled()
> 
>+  // Dialogs are also blocked if the document is sandboxed with SANDBOXED_MODALS
>+  // (or if we have no document, of course).  Which document?  Who knows; the
>+  // spec is daft.  See <https://github.com/whatwg/html/issues/1206>.  For now
>+  // just go ahead and check mDoc, since in everything except edge cases in
>+  // which a frame is allow-same-origin but not allow-scripts and is being poked
>+  // at by some other window this should be the right thing anyway.
>+  if (!mDoc || (mDoc->GetSandboxFlags() & SANDBOXED_MODALS)) {
>+    return false;
>+  }
>+
>   return topWindow->mAreDialogsEnabled;
> }
> 

>diff --git a/layout/base/nsDocumentViewer.cpp b/layout/base/nsDocumentViewer.cpp
>--- a/layout/base/nsDocumentViewer.cpp
>+++ b/layout/base/nsDocumentViewer.cpp
>@@ -1143,17 +1145,18 @@ nsDocumentViewer::PermitUnloadInternal(b
>   }
> 
>   nsCOMPtr<nsIDocShell> docShell(mContainer);
>   nsAutoString text;
>   beforeUnload->GetReturnValue(text);
> 
>   // NB: we nullcheck mDocument because it might now be dead as a result of
>   // the event being dispatched.
>-  if (!sIsBeforeUnloadDisabled && *aShouldPrompt && dialogsAreEnabled && mDocument &&
>+  if (!sIsBeforeUnloadDisabled && *aShouldPrompt && dialogsAreEnabled &&
>+      mDocument && !(mDocument->GetSandboxFlags() & SANDBOXED_MODALS) &&
>       (!sBeforeUnloadRequiresInteraction || mDocument->UserHasInteracted()) &&
>       (event->WidgetEventPtr()->DefaultPrevented() || !text.IsEmpty())) {
>     // Ask the user if it's ok to unload the current page
> 
>     nsCOMPtr<nsIPrompt> prompt = do_GetInterface(docShell);
> 
>     if (prompt) {
>       nsCOMPtr<nsIWritablePropertyBag2> promptBag = do_QueryInterface(prompt);

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 25 • 8 years ago

Wes, thanks for backing that out.  I thought I had done a try run with these patches, but it looks like it never went through.  :(  The good news is that this gives me testcases for the allow-modals bits.  ;)

> it appears that in the current codebase we allow modals for all sandboxed frames.  Is that correct?

Yes, but in practice they can only be triggered when allow-scripts is used, because otherwise the relevant codepaths can't be reached at all.  But you're correct that we don't prevent nullprincipal things that can run script from putting up an alert or anything like that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 8 years ago

Actually, those tests are all using showModalDialog, and since we're dropping support for that anyway with e10s, I probably shouldn't base new automated tests on it.  I'll just add allow-modals to the relevant tests.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 8 years ago

> I think if we fixed Bug 960545, then this would cause a problem without one permitted sandboxed navigator

You're right.  This looks like a spec bug to me.  Filed https://github.com/whatwg/html/issues/1218

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 8 years ago

OK, so I added the allow-modals bits for the failure that actually involves sandboxed iframes.  That fixes things locally, but not on try.  And the other failure doesn't involve any sandboxing at all.  Tracking that down was a bit fun.

The problem is that nsDocumentViewer::PermitUnloadInternal has logic like so:

  bool dialogsAreEnabled = window->AreDialogsEnabled();
  window->DisableDialogs();
  // Fire an event
  if (dialogsAreEnabled) {
    window->EnableDialogs();
  }

Here DisableDialogs/EnableDialogs poke at the mAreDialogsEnabled state of the GetScriptableTop of the given window.  On the other hand, AreDialogsEnabled considers various state of the window itself as well.  In practice most of that state typically doesn't claim dialogs are disabled, so we end up returning the scriptable top mAreDialogsEnabled.

But with this change, we start considering the sandboxing state of the window itself.  So now say we have a window W1 which has a sandboxed (without allow-modals) iframe containing W2.  We navigate the subframe, which runs the document viewer code.  This calls W2->AreDialogsEnabled(), gets false because we're sandboxed.  Then it disables dialogs on _W1_.  And doesn't reenable them, because it doesn't think it needs to.

I think the right fix is to have the document viewer save the dialogs-enabled state of the scriptable-top window, since that's the window whose state it will mess with.  Or perhaps even better introduce an explicit RAII class for doing this correctly.  I filed bug 1271521 on this; will land this patch after that one.

	Pulsebot
	Comment 29 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c524d0bd4c8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54baf6957ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/15adf545163d

	Carsten Book [:Tomcat]
	Comment 30 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c524d0bd4c8f
https://hg.mozilla.org/mozilla-central/rev/b54baf6957ba
https://hg.mozilla.org/mozilla-central/rev/15adf545163d

	Jean-Yves Perrier [:teoli]
	Comment 31 • 8 years ago

Updated:
https://developer.mozilla.org/en-US/Firefox/Releases/49#HTML
and
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#attr-sandbox