New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 677022 link

Starred by 27 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , All , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 796363


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Resources with an `integrity` attribute cannot reuse preloaded resources

Project Member Reported by y...@yoav.ws, Dec 26 2016

Issue description

Chrome Version: M57
OS: All

What steps will reproduce the problem?
(1) https://lodash.com
(2) font-awesome.min.css is preloaded but not reused due to an `integrity` attribute
(3)

We need to implement `integrity` for link elements. Spec issue at https://github.com/w3c/webappsec-subresource-integrity/issues/26
 

Comment 1 by y...@yoav.ws, Dec 26 2016

Alternatively, we could also just implement SRI verification for memcached resources if they're about to be reused by an SRI capable element with integrity data, even if the original download did not include an integrity attribute.
Owner: y...@yoav.ws
Status: Assigned (was: Untriaged)
Yoav, letting you own this for now while the spec issues are sorted out.  Let me know if you aren't planning on implementing it and we can find another owner or make it generally available.

Comment 3 by y...@yoav.ws, Dec 29 2016

I can own it in the mean time. If the solution is adding an `integrity` attribute, I can easily implement. If it's just smarter SRI checks when taking resources out the memcache, I'm less familiar with that code (but might be able to give it a try)
Cc: y...@yoav.ws
 Issue 687618  has been merged into this issue.

Comment 5 by y...@yoav.ws, Feb 1 2017

csharrison - do you know who currently owns SRI? If so, can you CC them?
 Issue 692420  has been merged into this issue.
FYI the issue that was merged in concerns a "regular" css link tag, not a preload link tag.

Comment 8 by y...@yoav.ws, Feb 15 2017

Yeah, I agree that we probably need to tackle these two cases at the same time, as they share many characteristics.
Cc: mkwst@chromium.org kouhei@chromium.org
hkrutzer: Sorry I should have been more clear with the merge. I mostly merged these because of what Yoav mentioned in #8: I suspect the fix for this will be similar to preloads.

jww knew sri (I think) but is no longer working on Chromium. ccing mkwst for further triage. Also ccing kouhei who complicated stylesheet SRI with [1] :)

[1] https://codereview.chromium.org/2398733003/
Cc: tyoshino@chromium.org toyoshim@chromium.org
+1 to having link integrity attr.
We currently discard undecoded bytes aggressively, and it makes sense as undecoded bytes will not be used after CSS parse except for SRI/CSP digest computation.
If we have the CSS resource SRI-ed preload, we can simply cache the IntegrityMetadataSet at the time so MemoryCache can utilize the cached IntegrityMetadataSet.

tyoshino,toyoshim: FYI
Unlike `crossorigin` that affects the actual fetch `integrity` should only affect if resource can be used or not. It might be harder to implement, but it would be easier to use 'Link' headers if repeating `integrity` attribute wasn't required in preload `link` elements and 'in Link' headers.
Also - checksums are long are make `Link` headers hard to read ;-)

Comment 12 by y...@yoav.ws, Feb 22 2017

@kouhei - would an "integrity needed" signal (and corresponding hash function) be enough for us to calculate integrity for those preloaded resources, but matching calculated integrity when the resource is about to be reused?

Also, even if we do that resources in the MemoryCache may still be in flight when they're about to be used, so integrity hash might not be ready for comparison. We would need to hook this up with current SRI buffering mechanisms (which I'm not familiar with) 

Comment 13 by y...@yoav.ws, Mar 6 2017

 Issue 698240  has been merged into this issue.
 Issue 701943  has been merged into this issue.
#12 : It depends. If we knew which hash to compute, it would help.

Comment 16 by addyo@chromium.org, Mar 29 2017

Cc: addyo@chromium.org

Comment 17 by y...@yoav.ws, Apr 10 2017

Owner: ----
Status: Available (was: Assigned)
This issue seems to be with the overall SRI feature and its fetching (or lack of) from MemoryCache. I'll be happy to assist anyone interested in tackling it, but I don't think I'll have time in the near future to tackle it on my own. Marking as Available.
I've had to remove SRI entirely because this affects performance on mobile.
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Owner: vogelheim@chromium.org
Status: Assigned (was: Available)
I don't honestly know how tough this will be in the current infrastructure, but if you could take a look at it, vogelheim@, I'd appreciate it.
Status: Started (was: Assigned)
I disagree with the analysis in #17. From what I'm seeing:

- The preload does its thing.
- The 'real' resource request tries to match a corresponding preload in
  ResourceFetcher::MatchPreload.
- This calls Resource::MustRefetchDueToIntegrityMetadata, which returns true
  for the '.../font-awesome.min.css' resource.
- This causes the ResourceFetcher to abandon the preload and to re-request the resource.
- This also causes the console warning, which correctly diagnoses the mismatch.

This is working-as-designed, in that MatchPreload is meant to not match if the preload's integrity attribute is different from the resource requests'. 

The odd thing is, the preload link in <head/> even has the same integrity attribute, but apparently that gets lost along the way.

I see two ways of fixing this:

1, Make sure integrity=... is fully supported on <link>. Apparently the integrity attribute disappears somehow.

2, I'm guessing that the core reason for the MustRefetchDueToIntegrityMetadata doesn't actually apply to pre-fetches. If so, we could try to "adopt" a pre-fetch w/o integrity metadata, instead of abandoning it.


No idea what's best, or if I'm overlooking something... would be happy for any advice.

Thanks!

I think the only time we really care about integrity validation is when executing/applying/rendering/etc a resource. Prefetching a resource doesn't have security implications in and of itself (as least, none that SRI would be capable of addressing). That makes #2 somewhat appealing: we could prefetch a resource blindly in order to avoid blocking network requests later on, and then verify that the thing we're executing matches our expectations at the point where we're about to execute it.

Do we discard the bytes after prefetching? Or do we still have them lying around for integrity checks by the time we get to execution?
#20: Correction: The preload is in an HTTP Link:-header, without an integrity attribute. So that would change option 1 to, 'tell page authors to include integrity in the prelink header, and then maybe actually make it work'. I guess that also makes it somewhat less attractive.

#21: I don't know yet, but the assumption is that the bytes are still there. Will check.

Comment 23 by jww@chromium.org, Jul 6 2017

Drive-by comments:
(1) Integrity attributes were only supported on rel=stylesheet <link> tags, so I only implemented integrity attribute checking on StyleResource objects (and, separately, ScriptResource objects). Thus other resources created from <link> tags did not do anything with the integrity attribute. So, yes, you can tell authors to include integrity attributes in their preload, but you'd still need to add integrity attribute checking to the LinkResource object. 

(2) The issue is that at the preload site you don't generally know what type of resource is going to consume the preload, and you definitely don't know what integrity metadata that resource is going to have. As mentioned in comment #10, bytes are aggressively thrown away, so the implementation of SRI attempts to calculate integrity at fetch-time, otherwise it won't ever be able to see the bytes. In the case of rel=preload, there was no obvious way to figure out if the resource was going to be used later on with an integrity attribute, so I just skipped it at the time.

Looking now, it's tricky because at the point that the preload scanner checks for <link rel=preload> (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp?l=207), it doesn't know how it's going to be used later on, and in particular, if there's going to be an integrity attribute on it. The good news is that's only really problematic if it has an 'as' attribute, since then it aggressively tries to make a specific resource, which would throw away the bytes.

Comment 24 by y...@yoav.ws, Jul 7 2017

vogelheim@ - I don't disagree with your assessment from #20. Apologies if my comment at #17 wasn't clear/detailed enough. We also seen another issue with SRI which without digging too deep I thought was related: https://bugs.chromium.org/p/chromium/issues/detail?id=701943
Looking at it again, the issue there is lack of `integrity` support for link stylesheets in the HTMLPreloadScanner.

Option 2 seems best, as it would enable developers to avoid adding the same integrity info twice.

jww@ - Preload actually does know what type of resource it has (using the `as` attribute). How much overhead would it be to calculate SRI for all preloaded CSS/JS resources? If there are more than one potential hash, I'm guessing that can get complex. But maybe we can add some "needIntegrity" attribute with the hash name to link preload. It'd be better than forcing devs to add full integrity info, and we'd still be able to calculate the right hashes for these resources. 

 
#21 + #23-(2), aggressively discarding bytes:

ScriptResource only drops the bytes when the script is truly executed - which can't happen during a preload, which gives us plenty of time to 'adopt' the integrity attribute. I notice CSSStyleSheetResource does this in CheckNotify, which would often prevent this. I could delay dropping the bytes in CSSStyleSheetResource to a point in time that would could only happen during the 'real' execution, similar to ScriptResource. That doesn't seem hard to do, and wouldn't affect steady state memory consumption.

As an alternative, we could simply check whether the bytes are still there, and thus only partially fix this issue.


#24, "calculate SRI for all preloaded CSS/JS resources":

I don't think that's viable. There are several supported hash algorithms, and we'd have to compute each. Worse, we're looking at signature-based integrity (see link below), where this would be impossible, since you can't compute the signature without knowing the key.

(signature-based integrity link: https://github.com/w3c/webappsec-subresource-integrity/blob/master/signature-based-restrictions-explainer.markdown )

Comment 26 by y...@yoav.ws, Jul 7 2017

Didn't know about the signature based integrity check. Thanks!
I think the option of calculating the hash using the downloaded bytes is the best option, if possible to implement.

Comment 27 by jww@chromium.org, Jul 7 2017

Yes, what vogelheim said about calculating hashes. We thought about implementing that, but decided that it probably was prohibitively expensive, and even if it was viable in the short term, it would be taking on a fair amount of technical debt since allowed hash algorithms are expected to grow in the future (not to mention signatures as well).

yoav@, I didn't realize that "as" was required for rel=preload; that actually explains my confusion :-) Yes, that makes it more difficult since in our implementation, you have to know the integrity when the resource is loaded for stylesheets and scripts, since the bytes are thrown away later.

That means that the most viable path forward, it seems to me, is to have the preloader aggressively search for uses of the resource later, and if it finds one, to grab the integrity attribute off of that tag. You can probably stop once you've found one and not go any further.

This does mean that if you have two tags with different algorithm sets, you'll have to refetch on the second one, but that's already the case. It would be easy enough to have a metric for that, though, and see if it pops up in practice at all.
I have a bit of a hacky solution now, which follows the path laid out in #25; not jww's suggestion in #27.

Basically:
- This slightly refactors CSSStyleSheetResource & ScriptResource, to share integrity-related code, but mainly to do the byte->string conversion on demand (like ScriptResource already does), rather than always doing it in CheckNotify.
- For preloaded resources, the byte->string conversion doesn't happen. Hence, source bytes are still there.
- When matching preloads, if we find a preload without integrity attribute that still has the data, we'll simply copy over the integrity attribute. Preload matching (and integrity cheacking) would then proceed normally.

This works for unit tests, and also for the lodash.com case cited here.


This is an opportunistic approach, which might still fail if conditions are just right. But then, having the preloader search for the same resource has the same issue, so I don't feel too bad.


The code is a bit ugly, so I'll try to prettify it a bit more before sending it out. Let's hope loading owners will like it.
Cc: kinuko@chromium.org
My initial change clashed badly with ongoing refactoring in that general area. Since those changes have now landed, I re-wrote the patch. It's a good bit smaller now; and fixes the test cases; and also the lodash.com case given above.

CL is here, in case anyone wants to take a look: https://chromium-review.googlesource.com/598910

Let me summarize my understanding.

* Option 1: Link rel=preload requires integrity attr.
- PreloadScanner+LinkLoader to issue PreloadRequest w/ integrity set

Simple to implement. Requires user to properly set this up (should put console warning, probably)

* Option 2: Never discard raw bytes
PreloadScanner+LinkLoader issues PreloadRequest w/out the integrity set. However, since we never discard the raw bytes, we can compute the integrity hashes when we ResourceFetcher::MatchPreload.

* Option 3: Best effort matching. Discard the raw bytes, and hope that RF::MatchPreload is called before discarding raw bytes.

IIUC this is the vogelheim@ is moving forward: https://chromium-review.googlesource.com/598910
I have a preference to option 1, as this gives most predictable behavior.
but I can be convinced w/ any|combination of the proposal above.

#31 + #32: That matches my understanding as well. My understanding is that Option 2 would very likely be too expensive (in terms of memory usage), so it's really a decision between 1 and 3.

I lean towards #3, mainly because I think we're working around Chromium implementation artifacts here. That is, from the web platform's/specifications' perspective there doesn't seem to be any reason why a preload would require an integrity attribute. It merely says that the resource will likely be used, and doesn't have any obvious security function.

----

I guess one decision criteria would be what other browsers do. I'll look into that next week.

Another criteria might be where we think we're headed long term, whether we think we can at some point eliminate the "oops we threw away the bytes" problem, or whether we're stuck with that.
> from the web platform's/specifications' perspective there doesn't seem
> to be any reason why a preload would require an integrity attribute.

Does CSP's require-sri-for directive impact our preload requests at all?
Cc: yhirano@chromium.org
I prefer option 1 from predictability and implementation simplicity.

Is it possible to introduce a new integrity primitive for link rel=preload, saying "compute hash with this algorithm, I would give the exact hash value after"?
vogelheim@'s comments seem right to me. Option #1 boils down to asking developers to do extra work in order to overcome a quirk of Chrome's implementation. That doesn't sound reasonable, though I agree that it's simpler to implement.

At the moment, SRI is an execution enforcement mechanism. It's not clear to me that there's any value to developers in tacking it onto `preload` (and, as Eric notes, `require-sri-for` doesn't prevent preloads today, nor does it seem to me that it should).

Comment 37 by y...@yoav.ws, Oct 9 2017

vogelheim@ - regarding option #3, how often will we mismatch due to the raw bytes getting discarded before match happened?
#37: My understanding is that if you have one preload and one usage with integrity=..., then this should always work with the proposed fix. But you can construct funky situations where you have a preload and two usages on the page, one with and one without integrity.


------------------

My reasoning: Currently, this is a race. Whichever request - preload and proper load - finished first will do the character conversion (which implies discarding the raw bytes).

With the proposed fix, the preload request would never trigger the character conversion and would hence never discard the raw bytes. In other words, the preload request would no longer 'race' with the real request about doing the character conversion. Thus, with one preload and one proper request, the 'real' request would always find the unconverted, raw bytes.

If you have multiple requests for the same resource on the same page with inconsistent integrity attributes, it does become a race again.


Of course, that change of the mechanism - removing functionality from preload request - is also what makes the proposed fix somewhat risky & controversial.



General caveat, though: That part of the code is fairly subtle, so I might be overlooking something here after all.

I don't see this happening anymore on v63.0.3239.84 on e.g. https://scotthelme.co.uk
Yay, looks fixed to me as well on https://mail.one.com/ which exhibited the problem before and another one I tested locally. Chrome 63.0.3239.108
Blockedon: 796363

Comment 42 Deleted

As of August 2018, the original bug is still present on lodash.com in Chrome stable (67.0.3396.99) and Canary (70.0.3516.0).

The preload warning shows up consistently for preloaded CSS with integrity. For preloaded JavaScript with integrity, it still shows up *sometimes*, but I cannot reproduce it consistently.
Update: preloaded JavaScript files with integrity cause warnings and are downloaded twice consistently on a fresh tab opening the page, but gets handled properly when the page is reloaded in place.
Cc: domfarolino@gmail.com
Verifying that the above report displays the preload warning for preloaded CSS with subresource integrity.

vogelheim@, yoav@, domfaralino@: is this likely a remaining artifact of racing?
Screen Shot 2018-08-08 at 10.09.38 PM.png
772 KB View Download
Cc: rmcilroy@chromium.org leszeks@chromium.org

Sign in to add a comment