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

[wasm] Set default for disableIntegrityCheck to true #102515

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kg
Copy link
Contributor

@kg kg commented May 21, 2024

From my profiling, in Firefox we spend ~11% of CPU time during startup in their network stack, making copies of data coming from network/cache and hashing it to perform the integrity check. By disabling SRI all of this CPU usage goes away. The cost of this integrity check will scale as files get larger, so it is more significant for big binaries i.e. AOT'd applications.

It's hard to measure the actual impact in terms of wall clock time but from my understanding of how all this works, our .wasm modules and assemblies can't begin being processed until the integrity check has finished, which means the whole asset has to be loaded over the network and hashed in a blocking fashion before something like streaming wasm compilation can start. (It's possible they are doing some of this work in parallel, but ultimately the integrity check has to block execution.)

I don't have any visibility into what impact this has on Chrome because their profiler doesn't expose internals like Firefox's.

To make this mergeable we would need to enforce a set of rules to determine when it is a valid default. From my past discussions with experts, if the following rules hold:

  • Both the HTML origin and the subresources must be control over the same entity
  • The subresources are hosted in a location with security controls no looser than the HTML origin's

It should be possible to safely disable SRI for assets served over HTTPS. Potential scenarios where SRI would matter, and my reasoning for why SRI isn't a meaningful improvement:

  • SRI hash verification protects against data corruption at rest and during transit
    • Other parts of the stack should already do this. Packets are checksummed at the transport level, and assets are typically served via gzip/brotli which will amplify corruption to the point that the file will catastrophically fail to load
    • This would in practice detect rare corruptions "earlier" and turn them into a specific type of error instead of a class of weird data corruption errors. I'm not sure this is worth the cost of potentially higher startup time and higher CPU usage.
    • How common are these kinds of file corruptions? I have in the past seen cloudflare serve up corrupted bytes from their cache, but other than that I've never seen it.
  • SRI hash verification protects against man-in-the-middle attacks
    • I view this as fully theoretical. Anyone who can MITM the individual subresources can mitm the html file or the config that contains the hashes.
    • SRI seems aimed at protecting against things like malicious CDNs, so we would want to make the default SRI-on for scenarios like that.
  • Protection against intentional modifications at rest
    • Like the MITM scenario, any attacker able to edit assets for an application at rest could edit the html or the config containing the hashes too.

So I think the rules we would enforce should be:

  • All assets must come from the same origin as the host html file and configs
  • All assets must be served over HTTPS

It might also make sense to enable SRI for the very first page load, since that one is already slow and "corruption on the server or over the network" is more likely than "corruption at rest in the browser cache". But cold start time is also more important for conversion rates, so it might be what matters to users.

@kg kg added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-meta-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant