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

strict-dynamic and SRI #653

Closed
annevk opened this issue Apr 11, 2024 · 3 comments · Fixed by #654
Closed

strict-dynamic and SRI #653

annevk opened this issue Apr 11, 2024 · 3 comments · Fixed by #654

Comments

@annevk
Copy link
Member

annevk commented Apr 11, 2024

I found #426, but found nothing documenting web-platform-tests/wpt#44769, despite there being two implementations. What am I missing?

@dveditz
Copy link
Member

dveditz commented Apr 11, 2024

hashes and nonces always work to whitelist a <script>—or at least they are intended to! Strict-dynamic doesn't change that. A non-normative explanation is in section 8, but the algorithm is in §6.7.1 https://www.w3.org/TR/CSP3/#script-checks

Hash is explicitly mentioned in $6.7.1.1 Script directives pre-request check. That section also says that we leave it up to SRI to enforce the integrity of the response.

But in the §6.7.1.2 post-request check hashes aren't mentioned at all. Is the note about SRI enforcing the response supposed to imply you skip §6.7.1.2 for integrity-checked hashes? If we don't skip it then I don't see how external hashes are ever allowed by §6.7.1.2, with or without 'strict-dynamic'

  1. we are not a nonce so we continue
  2. we are parser inserted so we continue (or we're not even using strict-dynamic)
  3. we don't match the host list so we return 'Blocked'
    --> we don't get to 'Allowed'

Yeah, this is broken. Main fetch step 19 calls the CSP post-response check, and then step 22 processes the integrity. Following the spec we will never get to step 22 which is obviously not the intent. Maybe Fetch was in a different order in the past?

It gets a little tricky here. We know the request passed the CSP request check if we got here, but we don't know why it passed. Just because the request has an integrity attribute doesn't mean that's why it was allowed to load: if it's not strict-dynamic it might have matched on a hostname instead. I think we need to insert a step between 1 and 2 and reproduce the full integrity expression check from 6.7.1.1. If it matches we can return "Allowed" and let Main Fetch step 22 do its thing. If it doesn't match then we check strict-dynamic and then hostnames.

@dveditz
Copy link
Member

dveditz commented Apr 11, 2024

Oh, another problem in §6.7.1.2 is the if strict-dynamic is in the policy we need to return an explicit Allowed or Blocked. We do NOT want to fall-through and let the hostnames be checked. hostnames should always be ignored if strict-dynamic is specified.

Because of the missing hash check a request with a perfectly valid hash value could fall-through here and then get blocked by not matching any of the hostnames that weren't even supposed to be checked.

@antosart
Copy link
Member

Makes sense to me. I gave it a try at fixing it with #654.

mikewest added a commit that referenced this issue Apr 12, 2024
Several "authoring considerations" sections should have been marked non-normative,
as noted in #653. This PR addresses that oversight.
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 a pull request may close this issue.

3 participants