1904644 - (CVE-2024-7521) Assertion failure: iter->done(), at js/src/jit/JitFrames.cpp:695

Status: RESOLVED FIXED

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

Description

[Nils Bars]

Attachment: bug.js

Steps to reproduce:

Checkout commit 9fcc11127fbfbdc88cbf37489dac90542e141c77 and invoke the js shell as follows:

js --fuzzing-safe <test-case>

Actual results:

Assertion failure: iter->done(), at js/src/jit/JitFrames.cpp:695

Comment 1

[Jan de Mooij [:jandem]]

We have a Wasm => JS JIT call and the JS code throws an exception. The Wasm code catches the exception but it looks like the exception handler code then doesn't expect that to happen (it's not just the assertion, the trampoline code calling the exception handler also seems incomplete). A bit surprising because I'd have expected tests, fuzzers, or websites to have hit this by now.

var throwExc = false;
var e = {m: {foreign() {
    if (throwExc) {
        throw new TypeError("hi");
    }
}}};
var bin = wasmTextToBinary(`(module(import "m" "foreign" (func $foreign))(func (export "f") try(call $foreign)end))`);
var mod = new WebAssembly.Module(bin);
var inst = new WebAssembly.Instance(mod, e);
for (var i = 0; i < 20; i++) {
    inst.exports.f();
}
throwExc = true;
inst.exports.f();

Comment 2

[Jan de Mooij [:jandem]]

Phabricator patches are not synced to bugzilla atm, so:

https://phabricator.services.mozilla.com/D214959
https://phabricator.services.mozilla.com/D214960

Comment 3

[Andrew McCreight [:mccr8]]

I'm not able to view that Phabricator link, so maybe the permissions are off somehow.

How bad of a security issue is this? I can't tell from the comments in this bug so far. Thanks.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1904644 - Share more exception handling code. r?rhunt!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1904644 - Add tests. r?rhunt!

Comment 6

[Andrew McCreight [:mccr8]]

I am able to view the Phabricator link now. It looks like phab-bot just updated it.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #3)

How bad of a security issue is this? I can't tell from the comments in this bug so far. Thanks.

I'll say sec-high because we jump to a Wasm catch handler with garbage in the instance register. A local non-debug build crashes like this:

   0x852ac69d101:       mov    0x48(%r14),%rcx
=> 0x852ac69d105:       testl  $0x1,(%rcx)

(rr) p/x $rcx
$1 = 0xe5e5e5e5e5e5e5e5

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9409677 [details]
Bug 1904644 - Share more exception handling code. r?rhunt!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's not super easy but probably also not very difficult.
• 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?: All
• 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?: Should apply or be easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Not very likely, but exception handling is complicated.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 9

[Daniel Veditz [:dveditz]]

It's too late to get this into Fx128, and it's a bad candidate to be a "ride-along" in the mid-cycle point release because we don't schedule a mid-cycle release for the ESR branch(es). We should wait until after the release to land this.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 9409677 [details]
Bug 1904644 - Share more exception handling code. r?rhunt!

sec-approval+ = dveditz
please request beta uplift, and esr after we get some nightly/beta confidence

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/821842bd3d34
Share more exception handling code. r=rhunt

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/821842bd3d34

Comment 13

[Donal Meehan [:dmeehan]]

:jandem could you add beta, ESR115, and ESR128 uplift requests on this when ready?

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 9409677 [details]
Bug 1904644 - Share more exception handling code. r?rhunt!

Beta/Release Uplift Approval Request

• User impact if declined: Security bugs, crashes.
• 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): Complicated code but changes are fairly straight-forward. The problematic case that's being fixed isn't used much in the wild at this point.
• String changes made/needed: N/A
• Is Android affected?: Yes

Comment 15

[Jan de Mooij [:jandem]]

I added the approval requests. This might not apply cleanly to the older branches - I can upload separate patches for these if it's an issue.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

It'll need a bit of rebasing for ESR115. The other branches graft cleanly.

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9409677 [details]
Bug 1904644 - Share more exception handling code. r?rhunt!

Approved for 129.0b7

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8df73ee5ebfc

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Bug 1904644 - Share more exception handling code. r=rhunt!, a=dmeehan (ESR115)

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

It'll need a bit of rebasing for ESR115. The other branches graft cleanly.

I posted a patch for ESR115. Fortunately it was just a single minor conflict. I also verified the tests fail on ESR115 and are fixed by this patch.

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9409677 [details]
Bug 1904644 - Share more exception handling code. r?rhunt!

Approved for 128.1esr.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/9d0bd5bd8933

Comment 23

[Donal Meehan [:dmeehan]]

Comment on attachment 9414107 [details]
Bug 1904644 - Share more exception handling code. r=rhunt!, a=dmeehan (ESR115)

Approved for 115.14esr

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/723cc10c7059

Comment 25

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 26

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

2 months ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2024-09-09] .

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20af2f42d18f
Add tests. r=rhunt

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/20af2f42d18f