Add attributes allow-popups-to-escape-sandbox and allow-modals to iframe sandbox

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: tanvi, Assigned: bz)

Tracking

({dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [domsecurity-backlog])

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

2 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

Comment 3

2 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

Comment 4

2 years ago
Is there any update on this? We at https://www.atomx.com/ are very interested in this development.
(Reporter)

Comment 5

2 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!

Comment 6

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

Comment 7

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

Comment 8

a year 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.
Triaging at the moment, we need someone to take a look at this - putting it in the backlog for now.
Whiteboard: [domsecurity-backlog]
Created 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
Attachment #8749498 - Flags: review?(ckerschb)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8749500 [details] [diff] [review]
part 2.  Add the sandboxed modals flag to iframe sandboxing

Automated testing for this seems rather difficult; I've done manual testing.
Attachment #8749500 - Flags: review?(ckerschb)
Created attachment 8749501 [details] [diff] [review]
part 3.  Add the sandbox propagates to auxiliary browsing contexts flag to iframe sandboxing
Attachment #8749501 - Flags: review?(ckerschb)
Keywords: dev-doc-needed
Blocks: 1270648

Updated

a year ago
Duplicate of this bug: 1216480
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.
Attachment #8749498 - Flags: review?(ckerschb) → review?(bobowen.code)
Attachment #8749500 - Flags: review?(ckerschb) → review?(bobowen.code)
Attachment #8749501 - Flags: review?(ckerschb) → review?(bobowen.code)

Updated

a year ago
Attachment #8749498 - Flags: review?(bobowen.code) → review+
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?
Attachment #8749500 - Flags: review?(bobowen.code) → review+
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.
Attachment #8749501 - Flags: review?(bobowen.code) → review+
> 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.
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.

Comment 19

a year 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
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".
Flags: needinfo?(bzbarsky)
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
Oops, accidentally backed out the wrong three-commit push. Actual backout in https://hg.mozilla.org/integration/mozilla-inbound/rev/933b5260480f

Comment 23

a year ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7be22142cf4
(Reporter)

Comment 24

a year 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);
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.
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.
Summary: Add attributes allow-popups-to-escape-sandbox and allow-modal to iframe sandbox → Add attributes allow-popups-to-escape-sandbox and allow-modals to iframe sandbox
> 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
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.
Depends on: 1271521

Comment 29

a year 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

Comment 30

a year 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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(bzbarsky)
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
Keywords: dev-doc-needed → dev-doc-complete

Updated

11 months ago
Duplicate of this bug: 1287885

Updated

10 months ago
Duplicate of this bug: 1273141

Updated

10 months ago
Depends on: 1301138
Depends on: 1306384
Comment hidden (spam)
Comment hidden (off-topic)
Flags: needinfo?(gruzzerek)
You need to log in before you can comment on or make changes to this bug.