1823042 - (CVE-2023-29549) Assertion failure: cx->realm() == fun->realm(), at jit/VMFunctions.cpp:818

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 585fe519f14ca8f241370573a902fc6d53cf8ac6 the attached sample asserts in the js-shell when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe crash.js.
Bisecting the issue points to commit 4969ce47f2d294700b97907f9a890cac6d57f5d8 related to bug 1819558.

const v1 = this.newGlobal(this, this, this, this, this);
with (v1) {
    const v3 = ("p").__proto__;
    function f4(a5) {
        Reflect.construct(f4.bind(v1, v1), Reflect);
        return v3; 
    }   
    v3[Symbol.search] = f4; 
    function f11(a12, a13) {
        a12.search(v3);
        return this;
    }   
    v3[Symbol.toPrimitive] = f11;
    v3.trimEnd();
}

#0  0x00005555588482b8 in js::jit::CreateThisFromIC (cx=0x7ffff7438300, callee=..., newTarget=..., rval=...) at /js/src/jit/VMFunctions.cpp:817
#1  0x00002d4d9f43dd6f in ?? ()
#2  0x00007ffffffd63e0 in ?? ()
#3  0x00007ffffffd63b0 in ?? ()
#4  0xfff9800000000000 in ?? ()
#5  0x0000555559973e40 in js::jit::vmFunctions ()

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Sounds like an issue in the bind implementation, where we might not get the correct realm after bind.
Jan, can you have a look?

(setting s-s JS flag as far as I know, being able to change realm is one way to inject code where it is not supposed to belong)

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1819558

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1823042 - Switch to bound function target's realm before creating |this|. r?iain!

For non-constructing calls it's simpler and faster to switch after pushing the arguments
and loading the bound function's target, but for constructor calls this needs to happen
earlier.

Comment 4

[Andrew McCreight [:mccr8]]

sec-high I guess?

Comment 5

[Jan de Mooij [:jandem]]

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

sec-high I guess?

No, allocating an object in a different same-compartment realm isn't security sensitive. The only thing I can think of is this maybe affecting JS-implemented sandboxes like SES, so let's say sec-low.

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Jan de Mooij [:jandem]]

NI myself to request beta uplift.

Comment 8

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

Switch to bound function target's realm before creating |this|. r=iain
https://hg.mozilla.org/integration/autoland/rev/f45c94cb5b0da78f5f8f440028b555c043b1748a
https://hg.mozilla.org/mozilla-central/rev/f45c94cb5b0d

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9324412 [details]
Bug 1823042 - Switch to bound function target's realm before creating |this|. r?iain!

Beta/Release Uplift Approval Request

• User impact if declined: Broken websites.
• 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): Pretty simple and local fix.
• String changes made/needed:
• Is Android affected?: Yes

Comment 10

[Dianna Smith [:diannaS]]

Comment on attachment 9324412 [details]
Bug 1823042 - Switch to bound function target's realm before creating |this|. r?iain!

Approved for 112.0b7

Comment 11

[Dianna Smith [:diannaS]]

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

Comment 12

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 13

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons