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

new_audit: csp-xss #12044

Merged
merged 69 commits into from
Mar 8, 2021
Merged

new_audit: csp-xss #12044

merged 69 commits into from
Mar 8, 2021

Conversation

adamraine
Copy link
Member

CSP evaluator audit 🎉 !
Design doc

Screen Shot 2021-02-02 at 6 54 45 PM

Notes on the first iteration:

  • CSP evaluator working on an npm module which will make importing the library less messy.
  • Translating UI strings for CSP evaluator findings requires us to copy the strings into our own UIStrings object. I'm interested in any feedback on the process to translate each finding.
  • Every finding will have its own row with the exception of syntax errors. Individual syntax errors will be subitems grouped by CSP. In most cases this means syntax errors will be grouped into one extra row.
  • Severity icons will be in a follow-up PR.

Closes #7159

@adamraine
Copy link
Member Author

We are thinking about adding an extra pass to detect if a CSP reuses a nonce between page loads. We could also just look at a the response headers without going through an entire page load, but that means we can't look at any nonces in CSP meta tags.

@patrickhulce would this have any major FR implications if we examined response headers without loading the page?

@patrickhulce
Copy link
Collaborator

@patrickhulce would this have any major FR implications if we examined response headers without loading the page?

Two things come to mind:

  1. Relying on a LH-initiated navigation to do this check means it can't be done in any other mode. I'd strongly advise finding another way to do this (repurposed fetcher possibly?).
  2. The implementation of using devtools logs from a non-default pass (which has never been done before) will look quite different and pose a problem for the current method of compat.

Aside from FR concerns, I have rather serious reservations about adding a new pass in general. Even for our extra passes that supported multiple major audits we've come to somewhat regret them. When new audits have required the use of multiple passes before, we've typically rejected them, and the reuse of nonces doesn't feel sufficiently exceptional to justify an entire new navigation. I'm a big advocate for finding a new way to make network requests that doesn't involve a navigation to solve this (and the many other use cases like http-redirect where this has come up 😁)

@connorjclark
Copy link
Collaborator

@patrickhulce I'm gonna try changing the redirect pass to not wait for page load, as discussed in that doc I shared. I'm hoping that the pass time is reduced enough such that keeping it around indefinitely could be reasonable.

repurposed fetcher possibly?

That's a good idea, we should try this first. We ought to be able to make the fetch via driver.Fetcher, which will return the HTML string, and the Network.* protocols can get us the response headers.

@adamraine
Copy link
Member Author

adamraine commented Mar 1, 2021

I have been experimenting with fetcher to gather a second set of response headers.

Ironically, the iframe injected by the fetcher is blocked by the frame-src and child-src directives of many CSPs (e.g. https://paulirish.com). Does the SourceMaps gatherer do anything to prevent this?

@adamraine
Copy link
Member Author

Looks like we are going to need a new gatherer which uses Network.loadNetworkResource instead of the current fetcher for nonce verification. I think we should revisit in a follow-up without blocking the release of this audit.

@@ -0,0 +1,216 @@
// @ts-nocheck
Copy link
Member

Choose a reason for hiding this comment

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

is there any movement on optimizing this file further? And anywhere tracking that for us? I fear this could remain an 80KiB blob in our bundle indefinitely (or until someone happens to open up a bundle viewer a year or two from now).

I'm not saying module$exports$google3$javascript$security$csp$csp_evaluator$csp.isHash=module$contents$google3$javascript$security$csp$csp_evaluator$csp_isHash doesn't gzip/brotli well, but Aa=f will still usually end up smaller :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to make the bundle smaller and usable by copying the source code to a new directory, compiling with tsc and bundling with js-bundle.

The problem with this is that the process isn't as simple as taking the binary from google3 and adding an export statement. There were some code modifications I had to make for it to work.

Copy link

Choose a reason for hiding this comment

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

Hey! We actually just finished publishing this to NPM so it should now be possible to add a dependency on the NPM package rather than vendoring this blob. See:

https://www.npmjs.com/package/csp_evaluator

And let me know if you run into any issues with using the package. I believe I set everything up correctly, but I'm not a JS expert. :)

Copy link
Member Author

@adamraine adamraine Mar 8, 2021

Choose a reason for hiding this comment

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

Looks like CSP Evaluator is on npm!
https://www.npmjs.com/package/csp_evaluator
EDIT: @ddworken got to it just before I did

We can switch to the module in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reduces devtools bundle size by 45 KB

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

Successfully merging this pull request may close these issues.

New audit: CSP policy evaluation
8 participants