Skip to content
This repository has been archived by the owner on May 5, 2022. It is now read-only.

Preload and SRI #127

Closed
yutakahirano opened this issue Aug 16, 2018 · 16 comments · Fixed by #137
Closed

Preload and SRI #127

yutakahirano opened this issue Aug 16, 2018 · 16 comments · Fixed by #137
Assignees
Labels
Milestone

Comments

@yutakahirano
Copy link
Contributor

yutakahirano commented Aug 16, 2018

We, Chrome, have some difficulty to support script preload with SRI. Here is the story:

  1. ScriptResource translates the received raw bytes into text data, and discards the raw bytes to save memory.
  2. A preloaded resource wants to processing the data before matching with the actual request.
    1. We think this is OK because that is not observable from JS, and we can discard the result when it's rejected by SRI.
  3. As the integrity value is provided by the actual request (<script integrity=...>), the above is not possible - the raw bytes needed to compute the hash are already discarded.

We spent months to solve the issue in our codebase, but we've given it up due to complexity the solution would bring.

I think, instead, we could ask web developers to specify the integrity value at the link element. For example,

<link rel="preload" as="script" href="/script.js">

doesn't match

<script src="/script.js"
        integrity="sha384-oqVuAfXRKap7fdgcCY5uykM6+R9GqQ8K/uxy9rx7HNQlGYl1kPzQho1wx4JwY8wC"></script>

while

<link rel="preload" as="script" href="/script.js"
      integrity="sha384-oqVuAfXRKap7fdgcCY5uykM6+R9GqQ8K/uxy9rx7HNQlGYl1kPzQho1wx4JwY8wC">

does.

What do you think about this idea?

@yutakahirano
Copy link
Contributor Author

@mikewest
Copy link
Member

As we discussed earlier in the week, I think it's reasonable to allow developers to specify an integrity attribute for preloaded resources. I don't think that absolves us (Chrome) of fixing our implementation's behavior (and we discussed options!), but it's reasonable advice to give developers in the short term, and should be straightforward to define (as we already define integrity on <link> to support stylesheets.

@yoavweiss
Copy link
Contributor

I think it's unfortunate to place extra complexity on users, but understand that codebase complexity may not be trivial as well.

Would it be sufficient to include a preload signal that indicates that SRI would be needed, and what hash function would be used? That way the SRI verification can only be done when the <script> is used, but calculating the hash can be done for the preload.

e.g. would <link rel="preload" as="script" href="/script.js" integrity="sha384"> be sufficient for the engine to know which hash to calculate before dropping the raw bytes?

@yutakahirano
Copy link
Contributor Author

@kinu

@yutakahirano
Copy link
Contributor Author

@yoavweiss Yes, that's enough. I will be happy with that option as well.

@kinu
Copy link

kinu commented Aug 16, 2018

(As we discussed internally) I'm very supportive of this idea, and the alternative suggested by @yoavweiss sounds good to me too, especially if it could make the developers' life easier.

@mikewest
Copy link
Member

If we're going to specify an integrity attribute, it seems reasonable to use it (and to discard the preloaded resource if it doesn't match). I'm not sure there's much value in hinting at a hashing function alone.

@yoavweiss
Copy link
Contributor

The hint will enable the engine to collect the right hash value of the resource without storing its bytes until it is matched with a script/style.
The reason I want to avoid forcing devs to include the hash itself in the preload link, is to avoid adding it twice, which is clumsy and potentially error-prone.

@igrigorik
Copy link
Member

+1 for supporting integrity enforcement on preload.

Related discussion: w3c/webappsec-subresource-integrity#26

Re, function vs value: are there use cases where value would enable enforcement where it's not possible today? E.g. if I preload a response that's later consumed via fetch(), it would be nice to defer to preload to do the validation, albeit there is the wrinkle here that if validation fails than preload can't simply drop the bytes but would instead need to return a failed response to fetch()?

@yoavweiss I understand your motivation, but I'm also a little hesitant of overloading and special casing "integrity" value here with hashes. If we go down this route, perhaps we should consider a different attribute name?

@toddreifsteck
Copy link
Member

Per discussion at TPAC 2018 on 10/25/2018:

  • As a first priority, specify the addition of full integrity to link rel preload
  • As a second priority, it makes sense to add integrity attribute (or a new attribute) that allows the hash type to be specified. This could allow perf optimizations to occur on servers.

@yoavweiss , can you please edit the spec or find an editor to make this update?

@yutakahirano
Copy link
Contributor Author

@domfarolino

@domfarolino
Copy link
Member

albeit there is the wrinkle here that if validation fails than preload can't simply drop the bytes but would instead need to return a failed response to fetch()?

Correct. Chrome's motivation is originally to just be able to obtain the hash digest of the preload resource before throwing the bytes away, but having developers specify the integrity also allows us to perform SRI validation on the preload itself. Fetch specifies that a preload that fails, the response becomes a network error.

It's important that this network error is stored in the preload cache and can be "matched" with, or else the developer can write:

<link rel=preload as=script href=a.js integrity=sha256-hash-does-not-match>
...
<script src=a.js></script>

..and expect the preload to do the SRI gatekeeping. If the preload failed SRI verification and we discard the response, an insecure script will fetched, likely against developer expectation. I'll start preparing some spec changes.

@domfarolino
Copy link
Member

Another concern that I think seems un-addressed is: How should preload + SRI work for resources that the SRI spec currently doesn't support? E.g., SRI spec only supports links + scripts, but rel=preload can load all kinds of things that SRI could previously not affect. It feels like there are two ways of dealing with it:

  1. Transitively extend the effect of SRI to all resource types supported by <link rel=preload>
    • A.k.a <link rel=preload as=audio integrity=bogus-integrity> would cause a later audio tag request to fail due to failed SRI verification, which previously wasn't possible
  2. Limit SRI's reach on rel=preload to only scripts and styles, maintaining the contract that SRI initially intended
    • I think this solution makes sense, however, it would require some logic in Fetch Standard once the preload cache is spec'ed to conditionally apply SRI verification on "supported destinations".

The spec can't really effected quite yet (until preload cache is ~finished I'd say), but knowing people's opinions on this would help with the Chrome implementation in the mean time :)

@kinu
Copy link

kinu commented Jun 12, 2019

Saying something like "UA must check the validity of the response if its integrity metadata is specified and UA supports the integrity check for the destination" and leaving other cases as optional (e.g. MAY check) sounds fine with me, while open to other options.

@desaianuj
Copy link

@yoavweiss @igrigorik @domfarolino are we going to follow up with allowing <link rel="preload" as="script"...> to just specify the SRI spec type using a new attribute name so we don't overload the existing one? I still like the point of not having redundant SRIs on the page and just specifying something like <link rel="preload" as="script" integrity-spec-type="sha512" ...> for it to match the script fetch later on in the page that has the full SRI specified.

@mushu999
Copy link

mushu999 commented Feb 1, 2022

Wondering if this was ever enabled in Chrome browsers? If anyone has a link to the Chrome developer thread discussing this I'd be interested in reading more.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants