2010050 - Memory64 bounds check with spectre-mitigations off is incorrect on 32-bit machines

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Ryan Hunt [:rhunt]]

The bounds check we use with 64-bit wasm memories, on 32-bit machines, with spectre-mitigations=off, is incorrect and can be bypassed.

With spectre-mitigations off, the logic gets messed up and we skip the bounds check if the high 32-bits of the 64-bit index are non-zero [1] [2]. The index is then truncated back to 32-bits and used to index into the wasm memory. The mapped size of the memory can be less than 32-bits, leading to an out of bounds access.

Unfortunately we had tests for this, but they were only run with spectre-mitigations=on. The bug to change the default in the JS shell uncovered this (Bug 2009237). specre-mitigations=off is the default in web-content processes.

I'm going to investigate more if there are any mitigating circumstances, and write up a reduced test case. I also need to get a regression range.

[1] https://searchfox.org/firefox-main/rev/6ece603789f6751c37c48b23f39dbbb16b290592/js/src/jit/x86/MacroAssembler-x86.cpp#1993
[2] https://searchfox.org/firefox-main/rev/6ece603789f6751c37c48b23f39dbbb16b290592/js/src/jit/CodeGenerator.cpp#21557-21570

Comment 1

[Ryan Hunt [:rhunt]]

It looks like the bad code goes all the way back to the initial release of wasm-memory64 in Fx134 (I've linked to the bug). The other necessary prerequisite is spectre-mitigations being disabled in web content processes, which goes back to Fx119 if I got it right.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: (secure)

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: (secure)

Comment 4

[Ryan Hunt [:rhunt]]

The second phabricator revision contains a minimal test case. I was able to use it to read-write memory outside the allocated wasm memory.

The only real mitigating factor is that this requires a 32-bit browser build, which is less common now. This does require Ion to compile the function, but I think it's trivial to trigger a tier-up to get to Ion by just running the code in a loop.

Comment 5

[Ryan Hunt [:rhunt]]

I'm unsure if we should try to hide bug 2009237. Someone theoretically could examine the push failures and see something suspicious with bounds check failures (which is what I did to find this). But it might be too late for that, as it's already on try/autoland.

Comment 6

[Ryan Hunt [:rhunt]]

There's not a great way to obscure the fix here. The attached patch is pretty obvious. We could maybe remove the OOL trapping code for these bounds checks and then the fix would be a bit further away, and not point at the 32-bit architectures.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Could we remotely disable 64-bit wasm memories on 32bit via pref ahead of landing?

Comment 8

[Ryan Hunt [:rhunt]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)

Could we remotely disable 64-bit wasm memories on 32bit via pref ahead of landing?

We removed the pref for memory64 in Fx141 (https://bugzilla.mozilla.org/show_bug.cgi?id=1967231). So we could do that for ESR, but not release or beta.

It would also be a breaking change that could have web compat impacts. I don't have an estimate for if they'd be bad or not. It could be fine, or it might not.

Comment 9

[Ryan Hunt [:rhunt]]

Obviously we should have caught this by running our tests in more configurations, or in code review. But I'm also curious why existing fuzzers didn't catch it. The fuzzers do run with spectre-mitigations=off.

One unfortunate part of this bug is that if the wasm load/store triggers an access violation when reading OOBs, the wasm signal handler kicks in and thinks that it must be an expected wasm trap and converts it into a wasm trap exception. If there is no access violation, the load/store succeeds. The reason is that wasm signal handler currently just validates the faulting PC is in wasm code at a wasm load/store instruction. It does not check if the effective address of the load/store is in expected wasm memory.

So we maybe never caught this in fuzzers because the wasm signal handler prevented bad test cases from crashing. Which unfortunately would also make exploits using this bug more reliable.

I think it's possible to harden the signal handler to check the effective address, and only convert the OS exception into a wasm trap if it's within the reserved guard regions. I'll look into that and file a separate security bug for it.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ryan Hunt [:rhunt] from comment #8)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)

Could we remotely disable 64-bit wasm memories on 32bit via pref ahead of landing?

We removed the pref for memory64 in Fx141 (https://bugzilla.mozilla.org/show_bug.cgi?id=1967231). So we could do that for ESR, but not release or beta.

Another option might be to reenable spectre mitigation on 32bit (perhaps via pref) ahead of shipping the bounds checking fix.

Comment 11

[Ryan Hunt [:rhunt]]

Filed bug 2010315 for hardening the signal handler.

Comment 12

[Ryan Hunt [:rhunt]]

Another option might be to reenable spectre mitigation on 32bit (perhaps via pref) ahead of shipping the bounds checking fix.

Yes, that would work.

Comment 13

[Ryan Hunt [:rhunt]]

Comment on attachment 9537294 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly easily. The changes are only in 32-bit code in the 64-bit wasm bounds check. It wouldn't be too challenging to audit the 5 callers and find the vulnerable callers in Ion.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: beta, release, esr140 are affected. Release status flags are correct.
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this code is pretty well tested and the change is straightforward. (we had tests for it that just didn't run with spectre-mitigations=off).
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 14

[Ryan Hunt [:rhunt]]

To elaborate on the 'android affected' flag, android still requires spectre mitigations always [1] [2] and so it should not be vulnerable.

[1] https://searchfox.org/firefox-main/rev/8e6b6cb1dd0fdd9838e2359219e2b8d3b84490b2/dom/ipc/ContentChild.cpp#2740-2748
[2] https://searchfox.org/firefox-main/rev/8e6b6cb1dd0fdd9838e2359219e2b8d3b84490b2/modules/libpref/init/StaticPrefList.yaml#2048

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 9537294 [details]
(secure)

sec-approval+, a=dveditz
please hold off on landing the test until April 6 or later (after the 149 release).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/04e2701b51d4
https://hg.mozilla.org/integration/autoland/rev/d1f5a5383948
r=iain.

Comment 17

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

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

Comment 18

[Julian Seward [:jseward]]

Comment on attachment 9537294 [details]
(secure)

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Potential attacker-controllable OOB memory reads and writes when running 32-bit builds of Fx, in web content processes (because in those, sceptre mitigations are disabled). 64-bit targets are unaffected.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Note re tests: A test case was developed as part of the fix, but has not yet landed, per normal protocols, hence "No" re automated-test-q above.

Note re Android: android is not affected because it has sceptre mitigations always enabled. See comment 14.

• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It affects only wasm (not JS), and only on 32-bit targets. Patch is simple.
• String changes made/needed: none
• Is Android affected?: No

Comment 19

[BugBot [:suhaib / :marco]]

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 20

[Julian Seward [:jseward]]

The fix (D278853) applies cleanly to beta but not to esr140.
I'm working on an esr140 equivalent.

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9537294 [details]
(secure)

Setting the beta uplift flag for Comment 18

Comment 22

[Donal Meehan [:dmeehan]]

Comment on attachment 9537294 [details]
(secure)

Approved for 148.0b6

Comment 23

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/9038a7b7db39
https://hg.mozilla.org/releases/mozilla-beta/rev/6054b1b4eb8a

Comment 24

[Julian Seward [:jseward]]

Attachment: (secure)

Comment 25

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #20)

The fix (D278853) applies cleanly to beta but not to esr140.
I'm working on an esr140 equivalent.

The esr140 fix, D280551, is now attached. (w/ apologies if this
isn't the correct procedure for providing an esr-specific patch).

Comment 26

[Donal Meehan [:dmeehan]]

:jseward could you complete the uplift assessment on the esr140 patch? https://lando.moz.tools/D280551/
https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift#Submit_the_Uplift_Request_Form

Comment 27

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Potential attacker-controllable OOB memory reads and writes when running 32-bit builds of Fx, in web content processes (because in those, sceptre mitigations are disabled). 64-bit targets are unaffected.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Note re tests: A test case was developed as part of the fix, but has not yet landed, per normal protocols, hence "No" re automated-testing question above.

Note re Android: android is not affected because it has sceptre mitigations always enabled. See comment 14.

• Risk associated with taking this patch: low
• Explanation of risk level: It affects only wasm (not JS), and only on 32-bit targets. Patch is simple.
• String changes made/needed: none
• Is Android affected?: no

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/daaa46f53225

Comment 29

[BugBot [:suhaib / :marco]]

2 months ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2026-04-06] .

rhunt, please refer to the original comment to better understand the reason for the reminder.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/28863f16e634
https://hg.mozilla.org/integration/autoland/rev/9b6dc984087f
add test. r=iain

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/9b6dc984087f