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

[CSP] Incorrect script-src handling #4323

Open
lobsterkatie opened this issue Dec 2, 2024 · 0 comments
Open

[CSP] Incorrect script-src handling #4323

lobsterkatie opened this issue Dec 2, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@lobsterkatie
Copy link
Member

lobsterkatie commented Dec 2, 2024

In messing around with our CSP-handling code and looking at a bunch of example CSP events in the process of teaching the grouphash metadata helpers how to gather data on security reports, I discovered that we're interpreting the unsafe-eval and unsafe-inline keywords backwards.

This is from our logic which turns an incoming CSP report into a Sentry event:

if self.violated_directive.contains("'unsafe-inline'") {
"Blocked unsafe inline 'script'".to_string()
} else if self.violated_directive.contains("'unsafe-eval'") {
"Blocked unsafe eval() 'script'".to_string()

But according to MDN (here, capitalization mine):

If a page has a CSP header and ‘unsafe-eval’ ISN’T specified with the script-src directive, the following methods are blocked and won’t have any effect:

  • eval()
    etc.

And it’s the same idea for unsafe-inline. So IOW, having one of those in your script-src directive ALLOWS eval and/or inline scripts, not blocks them.

According to the spec, inline script violations have blocked-uri: inline and eval violations have blocked-uri: eval, so it seems like that's what we should be looking for instead. (See point 4 here.)

UPDATE: Added getsentry/sentry#81531 to track making sure the sentry side of things is updated once this is fixed.

@lobsterkatie lobsterkatie added the bug Something isn't working label Dec 2, 2024
lobsterkatie added a commit to getsentry/sentry that referenced this issue Dec 3, 2024
…inputs (#81539)

This adds two new grouping test inputs, to show how we handle CSP reports with `unsafe-eval` and `unsafe-inline` violations, which are cases we don't currently cover with our grouping tests.

Note that there are two bugs which currently affect how we handle such inputs:

- We don't currently recognize such reports for grouping metadata purposes (this is very soon to be fixed), which is why for now they're being recorded in the metadata snapshots as being grouped by an unknown method. 

- We handle such reports backwards at the ingest level, marking events which aren't either `unsafe-eval`- or `unsafe-inline`-type violations as one or the other. (Unclear how soon this will be fixed, as it went undetected for five years and no one has yet complained about it.) For now, the input data includes the necessary values to trigger the relevant handlers, but each also includes a `TODO` so that it can  be updated once the underlying bug is fixed. (See getsentry/relay#4323 and #81531.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant