Validate integrity metadata in module map - #12922
hiroshige-g wants to merge 8 commits into
Conversation
|
@whatwg/modules |
|
cc/ @mikewest for subresource integrity. |
|
Couldn't we ensure example 1 behaves as intended without nagative consequences on examples 2 & 3 by ensuring that any integrity attributes trigger an integrity check before execution, even if one wasn't done at loading time? |
It incurs memory cost in Chromium implementation, to keep raw bytes (which is currently discarded after load completion) of all loaded modules in the module map, even for pages that don't specify For the classic script and I explored intensively the implemenation options around SRI and preloads at that time, but it was already mostly 10 years ago. So things might have changed, but as far as very briefly codesearched/asked LLM, the performance cost is probably still valid (not sure though). |
|
Oh, OK. That makes sense. I guess the other alternative would be to calculate all the potential hashes for all preloaded modules before dropping their payload, which would have a runtime cost that impacts pages that don't need any of this. Have you looked into how compatible it would be to change the behavior to what this PR describes? |
|
@mozfreddyb for more SRI. Chromium's implementation aside, the proposal here seems reasonable as an initial step. It's better than the status quo, but I'm not sure that it's where we should stop. I'd like to understand what the actual cost of hashing resources would be; I'm pretty sure we can hide a good deal of the cost by shifting to a model that builds the hash as we're downloading a resource, and I'm also fairly confident that we end up hashing most resources for various purposes in any event (cache keys, etc). I started sketching that out for Unencoded-Digest enforcement in https://chromium-review.googlesource.com/c/chromium/src/+/7654216 a few months ago, but lost track of it and don't really have time to pick it up right now... But perhaps someone could be convinced to do so? |
Not yet. I can measure e.g. the percent of pages affected using
Perhaps theoretically yes, and there might be already existing (cryptographic or non-cryptographic) hases calculation around script loading across Blink and V8, a previous attempt for similar goal (in the context of (Personally I would be satisfied with the current proposal even if the breakage rate is relatively high as I'd consider it as an unfortunate but reasonable cost for enforcing SRIs, because anyway the guideline for the developer is clear and consistent: "always set the same integrity attributes across pages, or otherwise performance/functional breakage would occur", but SRI experts might have different opinions) |
Spec PR: whatwg/html#12922 Covers { correct | wrong | no } integrity for each of preceding and following module requests, typically `<link rel=modulepreload>` and `<script>`. Bug: 556858866, 502822515 Force-WPT-Export: true Change-Id: I67c09a53df62e970b26de52917ac50ee3122ece6
This is probably correct. A streaming hash update approach is theoretically better than our current implementation, but getting that benefit would be pretty expensive. It's not worth the investment if it's only covering edge cases of integrity mismatches, even though those edge cases are indeed surprising. If the metrics you're going to gather show that these cases don't crop up often enough to worry about, then it seems reasonable to accept them. If they crop up more often than expected, we might need to reevaluate. |
This CL records a UseCounter when enforcing integrity metadata matching in the module map (https://crrev.com/c/8380928) would newly reject module loading due to SRI. To check and record the integrity mismatches between module requests, this CL implements a part of Spec PR whatwg/html#12922 but doesn't actually reject the module loading (which will be done by https://crrev.com/c/8380928) - Record the integrity metadata associated with the request in `ModuleMap::Entry`. - Check each request's integrity metadata against the `Entry`'s integrity metadata. No behavior is changed except for the UseCounter. TAG=agy CONV=ef4b5b12-1823-4ab2-8e99-8d5f1fb59c84 Bug: 556858866, 502822515 Change-Id: I6ce3611d0216ff696d6f8bdc4b062506ea2e75c6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8408446 Reviewed-by: Yoav Weiss (@Shopify) <[email protected]> Commit-Queue: Hiroshige Hayashizaki <[email protected]> Cr-Commit-Position: refs/heads/main@{#1698252}
Summary
To apply subresource integrity for modulepreload and module scripts even when different integrity attributes are specified.
This PR makes module loading to fail when
integritymismatches with a previous successfully completed or in-flight module request to the same URL in the module map, even if the resource's contents match theintegrity's cryptographic hash.Therefore, this PR requires to set the same
integrityattribute value to all<link rel=modulepreload>and<script type=module>(or later requests can have nointegrityattribute).Sub changes:
match integrity metadatais created to unify the matching with<link rel=preload>cases.Behavior changes
Example 1 (primary intended behavior change):
Example 2 (unfortunate adverse effect):
modulepreloadandscript.Example 3 (unfortunate adverse effect):
modulepreloadandscript.Notes on failures due to integrity mismatch
Failing a script load that would otherwise succeed in the absence of a preceding
<link rel=modulepreload>with mismatching integrity metadata is unfortunate (Example 3).This might break existing pages by causing additional loading failures.
However, I expect trying to make such scripts to succeed would lead to double evaluation (of
foo.jsin the example above).For example, keying module maps also by integrity values (or other variants like https://crrev.com/c/8377861) would lead to double evaluation in Example 3.
I think this would likely be far more problematic and unexpected, given that mismatching integrity metadata is much more likely to be an unintentional mistake than an intent to evaluate the same script twice.
I haven't figure out anything that makes the (second) script element to load successfully in Example 2/3, without introducing performance cost (e.g. always calculating SHA256 etc. for all scripts even without integrity attributes) and without complicated semantics (e.g. evicting successful modulepreload results from module maps).
So overall I think the current proposed behavior is fine, aligning with
<link rel=preload>(which causes performance issues upon integrity attribute mismatches, not functional breakage, though).WDYT?
(See WHATWG Working Mode: Changes for more details.)
/links.html ( diff )
/urls-and-fetching.html ( diff )
/webappapis.html ( diff )