Bug 158875 – Add flag allow-popups-to-escape-sandbox to iframe sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Bin Lu]

There have been some improvements proposed to iframe sandbox:
https://wiki.whatwg.org/wiki/Iframe_sandbox_improvments
https://html.spec.whatwg.org/multipage/browsers.html#attr-iframe-sandbox-allow-popups-to-escape-sandbox
https://html.spec.whatwg.org/multipage/browsers.html#attr-iframe-sandbox-allow-modals

The reasoning behind these improvements is to make iframe sandbox usable for framed unsafe content (e.g., advertisements) and to disallow modal dialogs by default in sandboxed iframe:
allow-popups-to-escape-sandbox: With this flag, if a user interacts with an ad and clicks on it, it should be able to open up the ads landing page as a top level page in a new tab without being sandboxed.
allow-modals: The content inside sandboxed iframe should not be able to open modal dialogs unless this flag is specified.

Both Chrome and Firefox are supporting the new attributes now:
https://googlechrome.github.io/samples/allow-popups-to-escape-sandbox/index.html
https://googlechrome.github.io/samples/block-modal-dialogs-sandboxed-iframe/
https://bugzilla.mozilla.org/show_bug.cgi?id=1190641

[Radar WebKit Bug Importer]

<rdar://problem/26904051>

[John Wilander]

I don't remember if it's part of the spec but we should make sure the call to confirm() returns false if the call was blocked by the sandbox. Doing so provides a safe fallback if an iframe's code is denied to ask the user for confirmation.

Otherwise "allow-modals" will allow an attacker to grant JavaScript execution but block a confirm prompt. A benign service might want to allow iframe integration but do what it can to prohibit clickjacking. Knowing whether you were able to display a confirmation UI may be crucial.

[Mike West]

Regarding `confirm()`, the spec does indeed require returning false if this sandboxing flag is present: see step 2 of https://html.spec.whatwg.org/multipage/webappapis.html#dom-confirm.

[John Wilander]

Thanks, Mike!

[Bin Lu]

I'm wondering if there is any progress on this or if it's planned to be done early 2017.

Since many user complain about malicious ads, ads sandboxing have been been used to protect users from malicious ads on both mobile and desktop browsers. But Safari (or WebKit) not supporting "allow-popups-to-escape-sandbox" is blocking  sandboxing to be used to protect Safari users. Since Chrome, Firefox, Opera, etc are already supporting it, it would be great if Safari could support it soon.

[Dima Voytenko]

Likewise, asking for update. Adding this sandbox flag would improve security on Web overall. This is currently a blocker for many embeds to turn on sandbox.

[Frédéric Wang (:fredw)]

I plan to give a try to the allow-popups-to-escape-sandbox. I'm moving the allow-modals part to bug 171321 and renaming this one.

[Frédéric Wang (:fredw)]

Created attachment 308244 [details]
Patch

[Frédéric Wang (:fredw)]

OK, it was not so easy as I thought. There was a subtle case to handle in order to pass iframe_sandbox_popups_escaping-3... I basically copied the logic in Chromium to handle that case (see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Frame.cpp?q=Frame::CanNavigateWithoutFramebusting&dr=CSs&l=258).

[Build Bot]

Comment on attachment 308244 [details]
Patch

Attachment 308244 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3610077

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
fast/files/null-origin-string.html
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 308253 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308244 [details]
Patch

Attachment 308244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3610125

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 308254 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308244 [details]
Patch

Attachment 308244 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3610123

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
fast/files/null-origin-string.html
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 308256 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308244 [details]
Patch

Attachment 308244 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3610201

New failing tests:
http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html
http/tests/fileapi/create-blob-url-from-data-url.html
http/tests/security/xss-DENIED-window-open-parent.html

[Build Bot]

Created attachment 308261 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Bin Lu]

Thanks Fred for working on this!
Really looking forward to seeing its ship!

I've just created a follow-up bug for another flag 'allow-top-navigation-by-user-activation':
https://bugs.webkit.org/show_bug.cgi?id=171327

[Frédéric Wang (:fredw)]

Created attachment 308278 [details]
Patch

[Frédéric Wang (:fredw)]

@bfulgram: Since you reviewed bug 134850, could you please take a look at this? Thanks!

[Chris Dumez]

Comment on attachment 308278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308278&action=review

> Source/WebCore/dom/Document.cpp:2979
> +        const char* reason = nullptr;

Let's call this failureReason, for clarity.

> Source/WebCore/dom/Document.cpp:2980
> +        if (!targetFrame->tree().isDescendantOf(m_frame) && targetFrame->tree().parent())

I do not understand the need for the "&& targetFrame->tree().parent()" check. We know:
1. targetFrame != m_frame, otherwise we would have returned early.
2. targetFrame is a descendant of m_frame based on of isDescendantOf() check.

Therefore, I'd believe that targetFrame has to have a parent.

> Source/WebCore/dom/Document.cpp:2982
> +        else if (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))

Again, I do not understand, how can a frame NOT have a parent and NOT be the frame tree top?

The implementation of FrameTree::top() looks like so:
Frame& FrameTree::top() const
{
    Frame* frame = &m_thisFrame;
    for (Frame* parent = &m_thisFrame; parent; parent = parent->tree().parent())
        frame = parent;
    return *frame;
}

If frame does not have a parent, top() will return the frame itself...

[Chris Dumez]

Comment on attachment 308278 [details]
Patch

The rest of the change looks good otherwise but I'll r- for now as I think the logic in Document::canNavigate() can and needs to be simplified.

[Frédéric Wang (:fredw)]

Comment on attachment 308278 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308278&action=review

@Chris: Thank you for your review, will upload a new patch soon.

>> Source/WebCore/dom/Document.cpp:2979
>> +        const char* reason = nullptr;
> 
> Let's call this failureReason, for clarity.

OK.

>> Source/WebCore/dom/Document.cpp:2980
>> +        if (!targetFrame->tree().isDescendantOf(m_frame) && targetFrame->tree().parent())
> 
> I do not understand the need for the "&& targetFrame->tree().parent()" check. We know:
> 1. targetFrame != m_frame, otherwise we would have returned early.
> 2. targetFrame is a descendant of m_frame based on of isDescendantOf() check.
> 
> Therefore, I'd believe that targetFrame has to have a parent.

As I see, the condition here is "target frame is NOT a descendant of m_frame" so we can not infer it has a parent.

>> Source/WebCore/dom/Document.cpp:2982
>> +        else if (!targetFrame->tree().parent() && targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))
> 
> Again, I do not understand, how can a frame NOT have a parent and NOT be the frame tree top?
> 
> The implementation of FrameTree::top() looks like so:
> Frame& FrameTree::top() const
> {
>     Frame* frame = &m_thisFrame;
>     for (Frame* parent = &m_thisFrame; parent; parent = parent->tree().parent())
>         frame = parent;
>     return *frame;
> }
> 
> If frame does not have a parent, top() will return the frame itself...

I believe you are right. I copied that part from Chromium, but they their "top" implementation seems a bit more complicated.

[Frédéric Wang (:fredw)]

Created attachment 312289 [details]
Patch

[Chris Dumez]

Comment on attachment 312289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312289&action=review

> Source/WebCore/dom/Document.cpp:3041
> +        const char* failureReason = nullptr;

I am trying to map this implementation to:
https://html.spec.whatwg.org/multipage/browsers.html#allowed-to-navigate

But this is not straightforward.

> Source/WebCore/dom/Document.cpp:3044
> +        else if (targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))

How do we know targetFrame is a popup here? The check says that targetFrame is not m_frame's top frame.

What if targetFrame is one of m_frame's subframes?

> Source/WebCore/dom/Document.cpp:3047
> +            if (!isSandboxed(SandboxTopNavigation))

I don't think this is possible. See this code earlier in the function:
if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
    return true;

[Frédéric Wang (:fredw)]

Created attachment 312437 [details]
Patch

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #9)
> OK, it was not so easy as I thought. There was a subtle case to handle in
> order to pass iframe_sandbox_popups_escaping-3... I basically copied the
> logic in Chromium to handle that case (see
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/
> Frame.cpp?q=Frame::CanNavigateWithoutFramebusting&dr=CSs&l=258).

So I think it is a bit difficult to compare the current WebKit behavior, Chromium's implementation and the specification. So let's implement the basic behavior here and move the changes to Document::canNavigate into bug 173162.

[Frédéric Wang (:fredw)]

Comment on attachment 312289 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312289&action=review

>> Source/WebCore/dom/Document.cpp:3044
>> +        else if (targetFrame != &m_frame->tree().top() && isSandboxed(SandboxPropagatesToAuxiliaryBrowsingContexts) && (isSandboxed(SandboxPopups) || targetFrame->loader().opener() != m_frame))
> 
> How do we know targetFrame is a popup here? The check says that targetFrame is not m_frame's top frame.
> 
> What if targetFrame is one of m_frame's subframes?

OK, I think this is incorrect. I'll follow-up on bug 173162.

>> Source/WebCore/dom/Document.cpp:3047
>> +            if (!isSandboxed(SandboxTopNavigation))
> 
> I don't think this is possible. See this code earlier in the function:
> if (!isSandboxed(SandboxTopNavigation) && targetFrame == &m_frame->tree().top())
>     return true;

Right.

[Chris Dumez]

Comment on attachment 312437 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 312437 [details]
Patch

Clearing flags on attachment: 312437

Committed r218000: <http://trac.webkit.org/changeset/218000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.