Past non-security bugs related to this weird legacy feature:

1) issue 725115 - a change to blink caused the CTS tests that rely on this feature to start failing. The CL was reverted and relanded with special cases to support this feature, and then a subsequent CL restored more of the legacy behaviour that was missed the first time around. No actual app was observed to be broken here, only tests.

2) issue 762482 - a particular web page was broken when viewed in the WebView test browser, which does not enable multiple window support. The cause was that the page opened two popups and the gesture was being consumed by the first one - a CL was landed to not consume the gesture in this case. There's no evidence that any real apps actually had an issue here either.


I'm going to take a look at the relevant CTS tests and see if it's possible to remove the test's dependency on this feature (if it's incidental to what's actually being tested), as that will be informative :)

Jochen, do you know where the code that implements this special case in WebView went? You updated it in the CLs associated with the bugs I mentioned in #2, but that code doesn't exist any more and the OpenWindow code in blink no longer seems to contain a case for this, even though the behaviour clearly still works, which leaves me somewhat confused :)

do you mean this: https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_host_impl.cc?l=2925

Yup, thanks for the link Jochen.

So, two things:

1) It looks like it should be pretty simple to fix just the security issue here directly, by checking the allow-top-navigation sandbox flag in this case. This should have little compatibility risk and won't affect CTS/etc. I'll prepare a CL and regression test for this.

2) Only one CTS test even depends on this weird legacy behaviour, and it does so incidentally, not as an explicit test of this feature. It's pretty easy to fix the test to not rely on it, at which point we could consider removing this feature altogether. We can look at doing that in a followup non-security bug after this is resolved.

So I think the right way to fix this would be to execute something like the logic in LocalFrame::CanNavigateWithoutFramebusting (which is what checks the top navigation flags right now), in the codepath in create_window.cc that redirects the popup creation to the current window in this case. I'm not sure how best to do that though, as I'm not that familiar with blink code and there's a lot of cases here (it checks a number of things separately in order to print specific explanatory console messages for web developers' benefit).

Jochen, would you maybe be willing to help out with this? Do you think it's important to replicate that logic or would just checking kSandboxTopNavigation be enough?

something like this should work:  https://chromium-review.googlesource.com/#/c/chromium/src/+/1078748

do you have a test that is less involved than the thing in the buganizer entry?

Is navigating the top frame in a Webview more dangerous than that in a normal tab? If not, this seems low severity, since it's a bypass of the sandbox attribute which the top frame needs to opt in to.

torne: Can you please confirm this affects stable channel?

jochen: that change looks reasonable, thanks. I haven't written a test for this yet, I have just manually tested it by writing content with devtools. I suspect a layout test would be a reasonable way to test this, since there's already layout tests for the single-window case? (though we may also want a webview-layer test)

meacer: This affects all versions of WebView.

Navigating the top frame in WebView is often more dangerous than in a normal tab, because many use cases for WebView don't display a URL bar and so a top frame navigation is a phishing opportunity for malicious content. However, as you note, this is only a vulnerability in the case that the iframe is actually sandboxed (because otherwise it could do a top navigation anyway), and requires that the iframe is allowed to open popups (as otherwise this is blocked regardless).

The internal Android security bug (b/79170400) that this is copied from classified it as high severity due to being a bypass of a security feature, even if that security feature isn't enabled by default.

i thought layout tests no longer support single window setups?

The layout tests for the current single window behaviour still appear to exist and be run, as far as I can tell - I'm not too familiar with layout tests in general.

James is currently working on implementing a test for this so that we can verify the fix does what we want, and prevent regressions.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0446dd0fb5e2afe4142b26c1d777af091055b9da

commit 0446dd0fb5e2afe4142b26c1d777af091055b9da
Author: James Wallace-Lee <jamwalla@chromium.org>
Date: Tue Jun 19 00:46:07 2018

Don't allow sandboxed iframe to navigate top frame

This prevents sandboxed iframes that allow popups but not top
navigation from navigating the top frame.

The test fast/dom/Window/window-open-no-multiple-windows-from-sandbox
tests that a sandboxed iframe cannot navigate the top frame when
WebKitSupportsMultipleWindows is false.

Bug:  845983 
Change-Id: Ibe49275428bca1ceb3b5ca367d9309c2e087ea8a
Reviewed-on: https://chromium-review.googlesource.com/1098460
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: James Wallace-Lee <jamwalla@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568262}
[add] https://crrev.com/0446dd0fb5e2afe4142b26c1d777af091055b9da/third_party/WebKit/LayoutTests/fast/dom/Window/window-open-no-multiple-windows-from-sandbox-expected.txt
[add] https://crrev.com/0446dd0fb5e2afe4142b26c1d777af091055b9da/third_party/WebKit/LayoutTests/fast/dom/Window/window-open-no-multiple-windows-from-sandbox.html
[modify] https://crrev.com/0446dd0fb5e2afe4142b26c1d777af091055b9da/third_party/blink/renderer/core/page/create_window.cc

I'm going to follow up here as James is leaving the WebView team.

Security folks: do we want to backport this to M68, or is it low severity enough to just release it in 69?

Security Sheriff drive by comment: Can this issue be marked as fixed now that the patch has landed? Thanks.

This is a low severity issue that's mitigated enough that I think it doesn't warrant a merge.

Based on the severity of the corresponding Android bug, we would prefer a backport if possible.

It would be helpful if Chrome and Android security can agree about the severity here then :)

We're not backporting to M68; it's too late at this stage for an issue of this severity. It'll be released in M69.

FWIW, we discussed this on Chrome security chat last week and we maintain that it's a low severity bug from our point of view.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Please add QA manual verification steps if verifiable.Thanks