whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
7.96k stars 2.6k forks source link

Possible loophole in COOP together with sandbox="allow-popups" #5806

Closed zcorpan closed 4 years ago

zcorpan commented 4 years ago

While reviewing https://github.com/whatwg/html/pull/5760 yesterday, I noticed that The rules for choosing a browsing context step 8 says

If sandboxingFlagSet has the sandboxed auxiliary navigation browsing context flag set The user agent may offer the user one of:

Set chosen to the result of creating a new top-level browsing context and set windowType to "new and unrestricted".

and thought "does COOP not apply when using sandbox="allow-popups""?

Then I found in process a navigate fetch:

  1. If browsingContext is a top-level browsing context, then:
    1. Set responseCOOP to the result of obtaining a cross-origin opener policy given response and reservedEnvironment.
    2. If sandboxFlags is not empty and responseCOOP is not "unsafe-none", then set response to an appropriate network error and break. . Note: This results in a network error as one cannot simultaneously provide a clean slate to a response using cross-origin opener policy and sandbox the result of navigating to that response.

and thought "it will be a network error, so it's ok for window.open to return a WindowProxy".

But then I thought about the condition that results in a network error, and thought of a case that seems to me like it would still load a cross-origin resource in a popup, with the opener having a reference to the WindowProxy, despite the top-level using COOP: same-origin-allow-popups.

From what I can tell, "process a navigate fetch" doesn't block this (because responseCOOP is "unsafe-none"), and "The rules for choosing a browsing context" sets windowType to "new and unrestricted", thus window.open returns a WindowProxy.

Am I missing something?

@whatwg/cross-origin-isolation

zcorpan commented 4 years ago

I mean same-origin instead of same-origin-allow-popups

domenic commented 4 years ago

Note that allow-popups will unset the sandboxed auxiliary navigation browsing context flag. But I think that just makes this scenario occur with sandbox="allow-scripts"?

This section of "the rules for choosing a browsing context" has always seemed a little suspicious to me. Basically it's an escape hatch for user agents allowing popups to open anyway despite the sandbox="" attribute preventing them. Do any browsers actually do this? If they do, then maybe the normal rules shouldn't apply. But I kind of doubt it...

ArthurSonzogni commented 4 years ago

You describe a popup created from an iframe which is cross-origin with the main document. It is cross-origin, because sandbox was used without "allow-same-origin". As a result, window.open("https://b.com") do NOT return a WindowProxy.

Note: This is what Chrome shipped: (search for popup_coop)

zcorpan commented 4 years ago

@domenic

Note that allow-popups will unset the sandboxed auxiliary navigation browsing context flag.

Ah, yes. I was confused and got this backwards.

So this part of the spec is for "allow the user to open this popup that shouldn't open because of sandbox"

The way the spec is written it looks like it blocks the algorithm on the user choosing one or the other. But ignoring that, why does the spec allow the user to break out of the author-specified sandbox? That doesn't make any sense to me. As far as I can tell, none of Gecko, Chromium, or WebKit allow it. Here's a demo:

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/8343

In Chromium and WebKit, window.open returns null and devtools shows a message about a blocked popup due to lack of allow-popups. Gecko throws an InvalidAccessError exception. Per spec, if we follow the "don't ask the user" path in "The rules for choosing a browsing context", chosen is null and then "the window open steps" return null at step 11 (before opening a popup/navigating).

But I think that just makes this scenario occur with sandbox="allow-scripts"?

If the user allowed the popup, yes.

This section of "the rules for choosing a browsing context" has always seemed a little suspicious to me. Basically it's an escape hatch for user agents allowing popups to open anyway despite the sandbox="" attribute preventing them. Do any browsers actually do this? If they do, then maybe the normal rules shouldn't apply. But I kind of doubt it...

From my testing above, they don't seem to.

So this bug is essentially about sandbox only. Fixing the spec by not suggesting users can break out of the sandbox means there's no loophole for COOP either.

ParisMeuleman commented 4 years ago

If sandboxFlags is not empty and responseCOOP is not "unsafe-none", then set response to an appropriate network error and break. [I] thought "it will be a network error, so it's ok for window.open to return a WindowProxy". To my knowledge the goal of COOP interaction with sandboxing flags is that a popup with sandboxing flags (inherited from the opening frame) cannot open a document with a COOP header, as this could mean leaking opener's set attributes on a document that wants to not have relationship.

From what I can tell, "process a navigate fetch" doesn't block this (because responseCOOP is "unsafe-none"), and "The rules for choosing a browsing context" sets windowType to "new and unrestricted", thus window.open returns a WindowProxy. I reckon that in your original example the "COOP: unsafe-none" document should load (the one with the opener having COOP:Same-origin-allow-popups), similarly to what would happen if the opener document did not have COOP at all. In the redacted case, where the opener document has COOP:same-origin, the popups' document will trigger a browsing context group swap (navigating from a COOP:same-origin initial empty doc to a COOP:unsafe-none document). the window proxy will be closed on the opener side and the popup will have opener == null. I believe subsequent navigations to a page with a COOP value (other than none) will still trigger the network error, as the popup should still have the sandboxing flags.

The corresponding test is: https://github.com/web-platform-tests/wpt/blob/master/html/cross-origin-opener-policy/coop-sandbox.https.html

zcorpan commented 4 years ago

@ArthurSonzogni @ParisMeuleman I was interested in the implications of this step in "The rules for choosing a browsing context"

If sandboxingFlagSet has the sandboxed auxiliary navigation browsing context flag set

I now believe that this step is bogus in suggesting users can break out of the sandbox at all (by opening a popup that sandbox disallows). So COOP was a distraction, but would have been relevant if any browser implemented the "user override".

I've written a test at https://github.com/web-platform-tests/wpt/pull/24974 and I can submit a fix for the spec to remove the unimplemented fiction. (Edit: https://github.com/whatwg/html/pull/5817 )