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

Rewrote CSP & EcmaScript integration #170

Merged
merged 5 commits into from
Jul 9, 2019
Merged

Conversation

mikesamuel
Copy link
Collaborator

This reworks how new Function(source) and eval(source) are
checked against a CSP and Trusted Types policy.

It trusts TrustedScript when the relevant CSP policies and TrustedType
configurations agree on whether to enforce and the TrustedType
configuration places limits on policy creation.

It also changes the previous language to use calleeRealm instead of
callerRealm for consistency with other sinks.

  let f = new self.top.Function(source);

In this case, the callerRealm's Window is self and the
calleeRealm's Window is self.top.
The Trusted Types portion of this algorithm uses calleeRealm
for consistency with other sinks.

  // Assigning a string to another Realm's DOM sink uses that
  // Realm's default policy.
  self.top.body.innerHTML = 'Hello, World!';
  // Using another Realm's builtin Function constructor should
  // analogously use that
  // Realm's default policy.
  new self.top.Function('alert(1)')()

It also makes recent versions of bikeshed run without warnings.

Fixes #143
Issue #144

@mikesamuel
Copy link
Collaborator Author

@mikewest This is what we discussed via email.

@otherdaniel @koto Fyi, this has a normative change to the realm used.

spec/index.bs Outdated
1. If |ttConfig|'s {{TrustedTypeConfiguration/domSinks}} is
{{StringAtSinkDisposition|"reject"}} then
set |isTTEnforced| to `true`.
1. Let |isCSPEnforced| = `false`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name might be a bit misleading, IIUC this is not checking the policy disposition, but whether any of the CSPs (enforcing, or report-only) cover script-src without 'unsafe-eval'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um. isCSPEnforced should mean

∃ p. !isReportOnly(p) and 'unsafe-eval' not in p

If not, that's a spec bug.
What do you think of the name given that goal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is fine (there's just no check for report only here, I think).

spec/index.bs Outdated
<a>ASCII case-insensitive</a> match for
the string "<a grammar>'unsafe-eval'</a>", then
set |isCSPEnforced| to `true`.
1. If |isTTStrict| is `true` and |isCSPEnforced| is equal to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you calculate & check for isCSPEnforced here? Can't eval(TrustedScript) be exempt from CSP restrictions if isTTStrict and isTTEnforced under script-restricting CSP, and always otherwise?

IIUC, that would happen if you don't do any CSP-related calculations in this algorithm. EnsureCSPDoesNotBlockStringCompilation will enforce any CSP restrictions if this algorithm returned false, which may simply do nothing if there are no CSPs set.

What usecase does the condition in this step cover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking for consistency between enforcement, because I want testing with TT & CSP in report-only mode to only report violations during normal use that need to be fixed to avoid violation reports with enforcement on during normal use.

So I worry that if I don't check for consistency between TT and CSP that an engineer migrating would see CSP report-only violation reports that would be suppressed by the is-exempt check were TT and CSP both in enforce mode.

If that sounds like a good goal, maybe this deserves a note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this assumes you're swapping the CSP enforcement together with TT enforcement? For example, if you have enforcing TT and report-only CSP, this will generate violations that you have no way of fixing, which would disappear when CSP is enforced.

In general, during rollouts it would be rare to have the disposition of both CSP and TT to be changed simultaneously.

This reworks how `new Function(source)` and `eval(source)` are
checked against a CSP and Trusted Types policy.

It trusts TrustedScript when the relevant CSP policies and TrustedType
configurations agree on whether to enforce and the TrustedType
configuration places limits on policy creation.

It also changes the previous language to use *calleeRealm* instead of
*callerRealm* for consistency with other sinks.

> ```js
>   let f = new self.top.Function(source);
> ```
> In this case, the *callerRealm*'s Window is `self` and the
> *calleeRealm*'s Window is `self.top`.
> The Trusted Types portion of this algorithm uses *calleeRealm*
> for consistency with other sinks.
> ```js
>   // Assigning a string to another Realm's DOM sink uses that
>   // Realm's default policy.
>   self.top.body.innerHTML = 'Hello, World!';
>   // Using another Realm's builtin Function constructor should
>   // analogously use that
>   // Realm's default policy.
>   new self.top.Function('alert(1)')()
> ```

It also makes recent versions of `bikeshed` run without warnings.

Fixes w3c#143
Issue w3c#144
@mikesamuel
Copy link
Collaborator Author

I rebased and rewrote to recognize script-src: 'trusted-script'.

@mikesamuel
Copy link
Collaborator Author

Ack. Just noticed that I need to rewrite IsSourceExempt. It should simplify a lot since there's no need to distinguish between strong and weak TT configurations.

spec/index.bs Outdated Show resolved Hide resolved
@mikesamuel mikesamuel merged commit 69dc1a1 into w3c:master Jul 9, 2019
@mikesamuel mikesamuel deleted the csp3 branch July 9, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify interaction between unsafe-eval and TrustedScript.
2 participants