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

Restrict to secure contexts #259

Closed
annevk opened this issue Feb 17, 2020 · 15 comments · Fixed by #274
Closed

Restrict to secure contexts #259

annevk opened this issue Feb 17, 2020 · 15 comments · Fixed by #274
Labels
Milestone

Comments

@annevk
Copy link
Member

annevk commented Feb 17, 2020

Any reason this isn't restricted to secure contexts?

@koto
Copy link
Member

koto commented Feb 17, 2020 via email

@jonathanKingston
Copy link
Contributor

Some thoughts before I forget:

What's the advantage of restricting that?

  • Browsers agreed on restricting all new APIs unless it became an unreasonable implementation effort to do so.
    • This move was to help advance the web platform to HTTPS by default.
  • The rationale isn't too dissimilar to the arguments of removing Web Crypto. It's a new feature that is to improve the security of web applications. It's hard to expect security when the transport layer can't be trusted.

What's the advantage of restricting that?

  • An active network attack can manipulate the code such that the assurances of Trusted Types no longer hold.

@koto
Copy link
Member

koto commented Feb 18, 2020

Thanks for clarifying, Jonathan!

/cc @mikewest. I don't have a strong opinion here, but it would be surprising for authors if some directives of CSP applied, and some did not, depending on the page origin. An active network attacker can remove the assurances of most of the Web APIs, but here the effect of the attack would be that the web application can still have unconstrained DOM XSS, which doesn't give the attacker capabilities they did not already have.

If I understand correctly, secure contexts are intended to guard "sensitive APIs", not all new APIs (source), but I lack the bigger context here.

@koto koto added the spec label Mar 2, 2020
@otherdaniel
Copy link
Member

On Chromium side, we'll restrict TT to secure contexts.

(Obviously, it's much easier to extend an API's scope than to restrict it, so in case of doubt we should probably opt for restricting.)

@koto
Copy link
Member

koto commented May 19, 2020

Reopening, as this led to a nasty bypass due to secure context inheritance rules. In short, framing a document from a non-secure context disables DOM XSS protections despite that document being delivered over a secure transport. Letting the embedder disable security restrictions of APIs is quite undesirable.

Perhaps this can be gated on a weaker condition? Something like [SecureTransport] or [SecureContext=NoInheritance]? Protecting against XSS but only websites that prohibit framing themselves (or limit the schemes to secure ones) seems too limiting.

@koto koto reopened this May 19, 2020
@koto
Copy link
Member

koto commented May 19, 2020

// cc @mikewest

@annevk
Copy link
Member Author

annevk commented May 19, 2020

I think we should offer a way, perhaps through CORP, to indicate you only want to be embedded in secure contexts.

@koto
Copy link
Member

koto commented May 19, 2020

The issue is slightly different; Authors don't do anything wrong by letting themselves be embedded from http:, this should not make them more exposed to XSS.

@annevk
Copy link
Member Author

annevk commented May 19, 2020

I think that's debatable.

@arturjanc
Copy link

@annevk Does frame-ancestors https: not suffice to achieve the goal of making sure you're only embedded in a secure context?

@annevk
Copy link
Member Author

annevk commented May 19, 2020

@arturjanc it would break down when playing around on localhost. It wouldn't work for data: URLs. Not sure about blob: URLs. Depends on how that match is done.

@shhnjk
Copy link
Member

shhnjk commented May 26, 2020

I think #259 (comment) showed that restricting Trusted Types to Secure contexts provides a bypass. We should make deployment of Trusted Types as easy as possible, and adding more requirements for deployment isn't ideal.

So I think restricting Trusted Types to Secure contexts provides more risks than its advantages.

@koto
Copy link
Member

koto commented Mar 8, 2021

I removed the secure context restriction in #279 due to the bypass vector mentioned in #259 (comment). +1 that the platform should have a way for the embedees to opt-out from being framed by insecure contexts, but I think there's value in addressing XSS without gating that on SecureContext with the propagation rules. Or, more specifically, I think that, while restricting access to powerful APIs makes total sense, allowing a bypass of a security restriction via framing is very undesireable.

@koto koto closed this as completed Mar 8, 2021
@annevk
Copy link
Member Author

annevk commented Mar 8, 2021

Again, the bug here is it being possible to be framed in that way. I'd much rather we address the root cause than a symptom.

@koto
Copy link
Member

koto commented Mar 8, 2021

I think there should be a way to more accurately prohibit framing by insecure contexts, perhaps even making it the default at some point, or non-overridable setting later. But I don't think TT should be gated on that, unless all of CSP also is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants