1762520 - (CVE-2022-36315) SRI bypass: When <link rel=preload integrity="sha512-correcthash"> preloads a resource with the hash1, subresource requests with integrity="sha256-wronghash" also succeeds

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[hiroshige]

Steps to reproduce:

foo.js's SHA512 hash is correcthash512 and SHA256 hash is correcthash256, not wronghash256.
Run the following HTML:

<link rel="preload" as="script" href="foo.js" integrity="sha512-correcthash512">
<script src="foo.js" integrity="sha256-wronghash256"></script>

WPT version: Add the following sub test to the WPT /preload/subresource-integrity.html, inside the "if (supports_sri) {" block, and run the test:

  SRIPreloadTest(
    true,
    false,
    `Same-origin ${destination} with wrong digest does not reuse preload with correct and stronger digest.`,
    2,
    destination,
    same_origin_prefix + destination + ext + `?${token()}`,
    {integrity: sha512},
    {integrity: "sha256-deadbeefQ15RYHFvsYdWumweeFAw0hJDTFt9seErghA="}
  )

Actual results:

<link rel=preload> is successfully loaded, firing load event.
<script> is also successfully loaded, firing load event, even though its integrity attribute is wrong (that should be integrity="sha256-correcthash256" to be successful).

WPT: the added subtest fails because the load events of the main requests are fired:

FAIL Same-origin script with wrong digest does not reuse preload with correct and stronger digest. assert_unreached: Invalid subresource load succeeded. Reached unreachable code

I tested on Firefox 91.7.0esr on Linux.

So far I don't expect this is exploitable by attackers, but I'm filing this issue as an security issue just in case.
If an attacker can serve a malicious script as foo.js and inject <link rel=preload> with the SHA-512 of the malicious script, then SRI specified on <script> by the page author can be bypassed.
However, this needs injecting <link rel=preload> to the page, and if the attacker can inject <link>s, the attacker already can do more than the SRI bypassing.

I found this issue while reviewing https://github.com/whatwg/html/pull/7738, and this test case is stemming from:

<li><p>the user-agent has determined that <var>entry</var>'s <span data-x="preload integrity
metadata">integrity metadata</span>'s algorithm is more collision-resistant than
<var>integrityMetadata</var>'s algorithm <ref spec=SRI></p></li>

Expected results:

<link rel=preload> is successfully loaded, firing load event.
<script> fails, firing error event, because the integrity attribute doesn't match foo.js.

WPT: the added subtest should pass.

Chromium behaves like this.
I expect the WPT would fail on Safari, but due to another broader underlying issue.

Comment 1

[Anne (:annevk)]

This seems similar to not respecting CSP on the "actual fetch".

Comment 2

[Tom Schuster (MoCo)]

ScriptLoader::LookupPreloadRequest calls SRIMetadata::CanTrustBeDelegatedTo, which will always return true when the preload request used a strong hashing algorithm than the current request. Maybe it would be better to enforce that the preload request uses the same hash and algorithm? Added in bug 1623953.

Comment 3

[hiroshige]

Can we discuss this issue publicly (now or later when the impl is fixed), in order to fix the spec as well?
https://github.com/whatwg/html/pull/7738 merged this behavior into the spec (to reflect the current status anyway), and I'd like to amend the spec later.

Comment 4

[Anne (:annevk)]

Maybe don't actively call out the attack, but describe it as a robustness improvement? With that I think it should be okay to discuss this and improve the specification. Thanks for checking!

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:ckerschb, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #5)

Redirect a needinfo that is pending on an inactive user to the triage owner.

I am trying to ni? Honza again, maybe he has time to look at it eventually.

Comment 7

[hiroshige]

Thanks, I filed https://github.com/whatwg/html/issues/7973.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:freddy, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Frederik Braun [:freddy]]

(In reply to Tom Schuster (MoCo) from comment #2)

ScriptLoader::LookupPreloadRequest calls SRIMetadata::CanTrustBeDelegatedTo, which will always return true when the preload request used a strong hashing algorithm than the current request. Maybe it would be better to enforce that the preload request uses the same hash and algorithm? Added in bug 1623953.

Looks like Honza is unavailable. I've worked on the SRI spec and took a cursory look at SRIMetadata::CanTrustBeDelegatedTo.

I believe the issue is not comparing the algorithm strength. The code assumes that no metadata is needed because the latest request does not have integrity metadata here.
However, it should reuse the one from preload instead.

This seems the right place to fix. Though I haven't dug any further. Does that answer your question, Tom?

Comment 10

[Tom Schuster (MoCo)]

I believe the issue is not comparing the algorithm strength. The code assumes that no metadata is needed because the latest request does not have integrity metadata here.
However, it should reuse the one from preload instead.

I don't think that is true, at least not for the example from comment 0. The first (correct) hash is for the pre-load attribute and the second (wrong) hash is for the actual script/style load.

    {integrity: sha512},
    {integrity: "sha256-deadbeefQ15RYHFvsYdWumweeFAw0hJDTFt9seErghA="}

Comment 11

[Tom Schuster (MoCo)]

There is actually another test case that we should add. Firefox currently only tests if the subset of hashes in the current SRI metadata is contained in the other SRI. That means we will accept loads with a wrong hash as long as the preload also contains the wrong hash in addition to at least one correct hash.

SRIPreloadTest(
  true,
  false,
  `Same-origin ${destination} with wrong digest does not reuse preload with the wrong and a correct digest.`,
  2,
  destination,
  same_origin_prefix + destination + ext + `?${token()}`,
  {integrity: `${sha256} sha256-deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdead`},
  {integrity: "sha256-deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdead"}
)

Comment 12

[Tom Schuster (MoCo)]

Attachment: Bug 1762520 - Update preload SRI checking for HTML spec change. r?freddyb

Comment 13

[Tom Schuster (MoCo)]

https://hg.mozilla.org/integration/autoland/rev/cec84442f4c7ea410ba4660d4429f90b0d72f480

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cec84442f4c7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks like this grafts cleanly to Beta and ESR102. Did you want to nominate for uplift to both?

Comment 16

[Tom Schuster (MoCo)]

Comment on attachment 9284007 [details]
Bug 1762520 - Update preload SRI checking for HTML spec change. r?freddyb

Beta/Release Uplift Approval Request

• User impact if declined: It's possible for an attacker to bypass the sub-resource integrity attribute of a website. The actual security impact of this however is probably low.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The actual patch is quite simple and the feature itself is actually not widely used.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low risk patch security patch, early in the ESR cycle.
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:tschuster, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox103 to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9284007 [details]
Bug 1762520 - Update preload SRI checking for HTML spec change. r?freddyb

Approved for 103.0b9, thanks.

Comment 19

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b12599aeb21a

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9284007 [details]
Bug 1762520 - Update preload SRI checking for HTML spec change. r?freddyb

Approved for ESR102.1, thanks.

Comment 21

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/a2adccae05a4

Comment 22

[Frederik Braun [:freddy]]

Attachment: advisory.txt