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

Security: Implement Automatic Integrity Checks #15

Open
harlan-zw opened this issue Apr 12, 2024 · 3 comments
Open

Security: Implement Automatic Integrity Checks #15

harlan-zw opened this issue Apr 12, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@harlan-zw
Copy link
Collaborator

To improve the security of using third-party scripts, we're able to compute the integrity of a script at build time and inject it within the <script> tag.

For example, we can do something like this:

useScript({ src: 'https://example.com/test.js' })

-->

// without bundling
useScript({ src: 'https://example.com/test.js', integrity: 'sha512-...' })
// with bundling
useScript({ src: '/....js', integrity: 'sha512-...' })

This would provide a window between builds that would block potential attackers from modifying the script with malicious code. If a script source has already been attacked when the integrity is computed it wouldn't do anything useful.

@vejja Would be great to have your input on this 🙏

@vejja
Copy link
Collaborator

vejja commented Apr 12, 2024

We thought about the same but decided not to go for it because of 2 reasons:

  1. The attack window potential that you are mentioning. Especially for sites that use ISR or other automated scheduled deploy processes. You would then 'integrity-vet' a compromised script, although you are right to note that it is the same as not including the integrity hash at all.
  2. The potential risk of full site shutdown when the legitimate script provider modify their source. In that case, integrity hash would block the new release until the application owner does a full re-build of the application.

It is a pain really for external scripts that can change without notice (GA, CF, etc.). I think the way to think about it is:

  • If a script provider has a versioning system with publicly available integrity hashes, use a versioned release and include the integrity hash manually;
  • If not, do not include the integrity hash at all.

For self-hosted scripts though it is different, and we do inject integrity hashes for scripts that are generated as part of the build process - based on the assumption that the local script cannot be compromised at the time of build.

My personal opinion is that I'm fine with it as long as it is an option.
I would suggest to set it false as default, and personally I would not use it. The risk of bringing down the application is too high: GA is well-known to change regularly without notice for instance.

@harlan-zw
Copy link
Collaborator Author

harlan-zw commented Apr 12, 2024

Interesting, thanks for your input!

I agree that we shouldn't be adding integrity checks for any remote script that isn't versioned (and not bundled). For example useScriptNpm, this has a version option, it would be safe to generate the integrity for this.

When bundling scripts and serving them with the app, in my mind, it should be safe enough.

For example, we can download Google Analytics at build time and serve it for the duration of that release. I think the chances of Google breaking old script APIs is low due to the nature of browser caching, even if they do useScript will already handle it gracefully without breaking the rest of the app.

Here's an example:

useScript('https://static.cloudflareinsights.com/beacon.min.js', { assetStrategy: 'bundle' })

We download this script and serve it under a hash, meaning it gets transformed to something like this:

useScript({
  src: '/__script/<hash>.js',
  integrity: '...'
})

The only issue from this will arise if Cloudflare modifies the API the script talks to, this is still a risk but a low one and the consequences are minimal. If the user is opting it then I see no issue.

@vejja
Copy link
Collaborator

vejja commented Apr 12, 2024

Ah got it, you are proposing this as part of { assetStrategy: 'bundle' } only. I was more afraid of the implications of automatically computing integrity hashes of remote scripts at build time.

In that case fully agree that it's fine. And also agree that serious providers always update their APIs with backwards-compatibility in mind. Never heard of GA breaking old scripts.

The only remaining issue is when you bundle a compromised version of the remote script. Setting in stone a snapshot of that script prevents the user from benefiting from future security patches. But this has nothing to do with integrity - unfortunately that's a consequence of bundling.

@harlan-zw harlan-zw added the enhancement New feature or request label Apr 21, 2024
@harlan-zw harlan-zw changed the title Implement Automatic Integrity Checks Security: Implement Automatic Integrity Checks Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants