Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should opener be taken into consideration when determining if a context is secure? #42

Closed
jakearchibald opened this issue Jul 25, 2016 · 20 comments
Assignees
Milestone

Comments

@jakearchibald
Copy link

@jakearchibald jakearchibald commented Jul 25, 2016

Ok, I know this is going to sound like heresy, but I'm thinking a window opened by a non-secure context via window.open or <a target="_blank"> should still be considered a secure context.

I agree that an HTTPS iframe embedded in an HTTP page should not be considered secure, since that gives you an invisible and automatic way of creating a channel between the two, but window.open and <a target="_blank"> are visible, and both require a user interaction.

Imagine http://example.com contains:

<iframe src="https://secure-proxy.example.com/proxy">
<a href="https://secure-proxy.example.com/worker" target="_blank" rel="noopener">Click me</a>

The iframe is non-secure, but the opened tab is secure. The secure opened tab can perform secure actions, and send it to the non-secure iframe via any origin storage, BroadcastChannel, or in a hacky way SharedWorker (since its security is decided by the document which creates it).

If we drop the parts of the spec that take opener into account when deciding if the context is secure users get a more consistent experience when following links, as their seemingly top-level contexts are consistently able to access powerful features.

The alternative, to combat the above, is to add isSecureContext to the origin identifier. Meaning otherwise secure contexts opened via target="_blank" are a different origin to contexts opened to the same URL via target="_blank" rel="noopener". But that exacerbates the weird user experience.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Jul 25, 2016

Additionally, imagine https://example.com

<a href="http://example.com/worker" target="_blank">Click me</a>

If the link above is clicked, http://example.com now has a WindowProxy reference to https://example.com, but https://example.com remains a secure context unless the state of a context's security is mutable.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Jul 25, 2016

As @mikewest says, .opener is a misfeature. We could look at how much of the web would break if it were disabled when crossing schemes.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Jul 28, 2016

It'd be great to see numbers for "window.opener used from an HTTPS page to an HTTP page" - could we deprecate in this case?

@mikewest mikewest self-assigned this Jul 29, 2016
@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Jul 29, 2016

Arg, sorry I missed this when you filed it. Thanks for the ping.

So. Heresy or not, I'm also not thrilled with the opener restrictions, but I'm not sure that throwing them out completely is the right thing to do. I think I agree that in some cases it ends up being more confusing than helpful to taint a popup based on the way it was opened.

The general idea behind the opener restriction is to close down communication channels between secure origins and non-secure origins in order to prevent feature use in one context from leaking into another. As you note (and as described in https://w3c.github.io/webappsec-secure-contexts/#isolation), a number of communication side-channels exist for same-origin content that we haven't addressed via this restriction. Those side channels are hampered by the fact that no direct DOM access exists between the contexts, but they exist as long as we don't use the context's "secure" status to influence the origin (which I'm very reluctant to consider).

That said, the same side- and direct-channels exist for <iframe> elements. You've suggested that you're ok with those restrictions because you recognize the risk, and further that <iframe> is invisible and doesn't require a user gesture. I'm not sure that either of those limitations actually makes a significant difference. Getting a click to work with is trivial, and visibility only matters insofar as users would consider it odd that a window was opened. I have great faith in the ability of folks who want to work around the restrictions we're putting in place to build reasonable-feeling UI that looks like it's doing one thing, while actually exfiltrating data. More involved, yes, impossible, hardly.

Still, I'm totally sympathetic to the notion that we shouldn't punish innocent bystander sites to which a user legitimately wants to navigate. Since direct DOM access worries me much more than side-channel access, perhaps we could take the opener context's origin and content into consideration more than we do today. A pretty ugly strawman would be something like "If there's a non-secure view onto the openee's origin (or an origin which has opted into same-originness via document.domain) in the opener's frame tree, the openee is non-secure. Otherwise evaluate it as a standalone."? I'm not sure that actually helps though, and it's even more complex and confusing than the current setup.

I'd suggest that we can pursue a three-pronged approach here:

  1. Give pages the ability to protect themselves from the leakage opener implies, which we're poking at in w3c/webappsec#517 and w3c/webappsec#139.
  2. Reexamine the default behavior of opener. I think that turning it off when transitioning schemes is appealing, but I know that a number of federated identity providers poke window.opener in order to complete a sign-in process (as an example, all of the federated options on https://discourse.wicg.io/ rely on popups). I'm reluctant to break those for non-secure origins; I'm all for getting folks over to HTTPS, but unilaterally breaking folks' authentication mechanisms seems like quite a stick to wield.
  3. Evaluate weakening the opener restriction in Secure Contexts, as discussed above.

(/cc @annevk @bifurcation @hillbrad @jwatt)

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Jul 29, 2016

1, in particular w3c/webappsec#517 seems good (basically a signal that you want to become a new top-level browsing context rather than auxiliary, also useful if you want independent process benefits and such).

@bifurcation can be pinged through @rlbmoz.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Jul 29, 2016

@travisleithead

This comment has been minimized.

Copy link
Member

@travisleithead travisleithead commented Jul 29, 2016

I tend to agree with the ordered approach above. The more often a window can be legitimately disowned (via cross-origin navigation, or one of the opt-in options mentioned) the better, and allows for more opportunities for strong isolation in the UA (e.g., process isolation hinted at by Anne above). If these options help to re-set the "inherited" (in)secure context flag for that new window, then, that's just further incentive for site authors to apply it.

So, I don't have issue with the way the secure context flag is extended to opener windows today.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Jul 29, 2016

The difference between a window & an iframe is the origin that's displayed in the URL bar. It seems fine to limit iframes because the user can be unaware that the framed origin is executing code.

In regards to postmessaging, could this have been tackled as part of MIX? As in, signal a drop in security if a mixed communication happens, and allow CSP to prevent mixed communication?

w3c/webappsec#517 will solve this for geolocation & such, but it won't solve it for service worker. We need to know if the site's SW can handle the navigation fetch, and at that point we don't know if the site is going to drop opener.

We may drop opener automatically for SW-controlled scopes, although I've been reluctant to introduce changes in page behaviour resulting from an empty service worker. It could be opt-in on the activate event.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Jul 29, 2016

I think that turning it off when transitioning schemes is appealing, but I know that a number of federated identity providers poke window.opener in order to complete a sign-in process

Do they generally use window.open or target="_blank" too. Might be easier to deprecate opener on the latter when it's cross-scheme.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Jul 29, 2016

w3c/webappsec#517 will solve this for geolocation & such, but it won't solve it for service worker.

Origin policy?

We may drop opener automatically for SW-controlled scopes

I would be pretty happy with this resolution.

@jakearchibald

This comment has been minimized.

Copy link
Author

@jakearchibald jakearchibald commented Aug 3, 2016

An origin policy would work here also. But I think we'll auto-disown opener if opener is a non-secure context. Happy for this to close if you are.

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Aug 30, 2016

Sure. I'll leave the bit as "at risk" in the spec just in case we can come up with something better, but happy to close this for the moment.

@mikewest mikewest closed this Aug 30, 2016
@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Aug 30, 2016

Actually, let's leave this open, just for visibility.

@mikewest mikewest reopened this Aug 30, 2016
@dveditz

This comment has been minimized.

Copy link

@dveditz dveditz commented Jan 11, 2017

This is causing us angst. We've been using the "is a secure context" state to warn about entering passwords on insecure pages. As a result we have confused users wondering why some of their https:// pages are warning them that submitting their password is insecure. It looks secure when they look at the url bar. Very hard to convince them that "the page might pass information insecurely via its opener" makes the page insecure. The site might be storing passwords in plain text in a world-readable file once they are securely transmitted.

Of course we could decide passwords are a special case and determine secureness differently, but if we're having trouble explaining this insecure-secure-site case to users maybe developers aren't going to get it either. If anything I'd rather just nullify opener by default if we think this is a significant case we need to stop, and make sites specify explicit rel=insecureopener to get it back (and then treat the spawned context as insecure).

@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Jan 11, 2017

@dveditz: We don't tie mixed content checks to the context being "secure", only to it being delivered over a secure transport. The password check you're talking about in Firefox seems much more similar to a mixed content check than to a "should this API be available at all" check. shrug

@jwatt

This comment has been minimized.

Copy link
Contributor

@jwatt jwatt commented Mar 13, 2017

For the record, as of Firefox 52 we added a privileged-code-only isSecureContextIfOpenerIgnored property to compliment isSecureContext, and the code that checks for passwords before allowing form submission uses the former. Offhand I'm not even sure why we care to check the security of anything other than the page containing the form to be honest, but I guess the more we can get away with locking things down and pushing the Web to HTTPS the better.

@mnoorenberghe

This comment has been minimized.

Copy link

@mnoorenberghe mnoorenberghe commented Mar 13, 2017

As it is now I think everything in Gecko should use isSecureContextIfOpenerIgnored instead of isSecureContext for the reasons explained before that there is currently nothing web developers can do to avoid an insecure opener from third-party sites in browsers. We just used isSecureContextIfOpenerIgnored for geolocation in bug 1072859 too since we noticed that's basically what Chrome and Safari did. From what I could tell Chrome doesn't implement the at-risk opener part of the spec in their secure context method. It seems like we need to remove the opener check from the spec due to it breaking websites until we have an deployed solution (e.g. the CSP ones) in browsers allowing authors to prevent the issue.

hubot pushed a commit to WebKit/webkit that referenced this issue Jun 10, 2017
https://bugs.webkit.org/show_bug.cgi?id=158121
<rdar://problem/26012994>

Reviewed by Alex Christensen.

Part 2

Implements the Secure Contexts spec., <https://w3c.github.io/webappsec-secure-contexts/> (Editor's
Draft, 17 November 2016) except for the allow-secure-context sandbox flag and restrictions on window.opener
as the former is at risk of being dropped from the specification and the latter is being discussed in
<w3c/webappsec-secure-contexts#42>. We are not making use of the Secure
Contexts functionality at the moment. We will make use of it in a subsequent commit.

* dom/Document.cpp:
(WebCore::Document::isSecureContext): Added,
* dom/Document.h:
* dom/ScriptExecutionContext.h:
(WebCore::ScriptExecutionContext::isSecureContext): Deleted; moved to class SecurityContext.
* dom/SecurityContext.h:
* page/DOMWindow.cpp:
(WebCore::DOMWindow::isSecureContext): Added.
* page/DOMWindow.h:
* page/SecurityOrigin.cpp:
(WebCore::isLoopbackIPAddress): Convenience function to determine whether the host portion
of the specified URL is a valid loopback address.
(WebCore::shouldTreatAsPotentionallyTrustworthy): Implements the "Is origin potentially trustworthy?"
algorithm from the Secure Context specification.
(WebCore::SecurityOrigin::SecurityOrigin): Compute whether this origin is potentially trustworthy.
Also, use C++ brace initialization syntax in member initialization list.
* page/SecurityOrigin.h:
(WebCore::SecurityOrigin::isPotentionallyTrustworthy): Added.
* page/WindowOrWorkerGlobalScope.idl: Expose attribute isSecureContext. Fix style nit; remove
period from a comment that is not meant to be a complete sentence.
* workers/WorkerGlobalScope.cpp:
(WebCore::WorkerGlobalScope::isSecureContext): Added.
* workers/WorkerGlobalScope.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218028 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@mikewest mikewest closed this in 98f2c26 Oct 18, 2017
@mikewest

This comment has been minimized.

Copy link
Member

@mikewest mikewest commented Oct 18, 2017

I screwed this up in Chrome, sorry for both the angst and the churn. I've dropped the section from the spec, which brings it into line with everyone's implementations. My apologies. :(

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Oct 20, 2017

As @jonathanKingston pointed out and also illustrated by the comment from @jwatt above, Firefox does actually implement this. We have https://bugzilla.mozilla.org/show_bug.cgi?id=1410364 to align with the changed standard. It would be good if there were tests.

@bzbarsky

This comment has been minimized.

Copy link

@bzbarsky bzbarsky commented Nov 8, 2017

We have tests; @jwatt added them when we implemented in Firefox. See http://w3c-test.org/secure-contexts/

It looks like @mikewest already updated those tests in web-platform-tests/wpt@dd83f89

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 22, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

--HG--
extra : rebase_source : 3d7fa73976571f357e84e369093aecfc10c5872e
extra : amend_source : ca86714f357b653577f3186b6312bfa00f1f45b9
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Nov 26, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Nov 28, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 29, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this issue Nov 29, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Nov 30, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Dec 5, 2017
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 681fece780aef771f1c6b56cddf4e52c8c800cb2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 6784a27f54ff76952569067bab6e34c443e48c84
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 681fece780aef771f1c6b56cddf4e52c8c800cb2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 6784a27f54ff76952569067bab6e34c443e48c84
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 681fece780aef771f1c6b56cddf4e52c8c800cb2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
… r=bz, r=dveditz

Per w3c/webappsec-secure-contexts#42, the
section considering the window opener when calculating secure context is
to be dropped. Firefox already uses "isSecureContextIfOpenerIgnored" in
most places as this is the actual behavior we want. This patch aligns
with the upcoming spec changes by ignoring the window opener. We also no
longer have to keep information about whether our opener was secure as
that no longer factors in our calculations.

UltraBlame original commit: 6784a27f54ff76952569067bab6e34c443e48c84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.