nuxt / scripts

Plug-and-play script optimization for Nuxt applications. (Public Preview)
https://scripts.nuxt.com
MIT License
134 stars 9 forks source link

Security: Implement Automatic Integrity Checks #15

Open harlan-zw opened 2 months ago

harlan-zw commented 2 months ago

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 :pray:

vejja commented 2 months ago

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:

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 commented 2 months ago

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 commented 2 months ago

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.