1810711 - (CVE-2023-25735) Assertion failure: IsObjectValueInCompartment(v, compartment()), at js/src/vm/NativeObject.h:1083

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Samuel Groß]

The following testcase triggers an assertion failure in debug builds of Spidermonkey built from current HEAD:

RegExp.sameZoneAs = RegExp;              
with (this.newGlobal(RegExp)) {          
    function f3(a4, a5) {                
        async function f6(a7, a8, a9) {                                                                                                                                                                                                                                                                                                                                 
            return a4;                                                                                                                                                                                                                                                                                                                                                  
        }                                
        const v11 = ("-9").replace;      
        try {                            
            v11();                       
        } catch(e13) {                   
            const o14 = {                                                                                                                                                                                                                                                                                                                                               
            };                                                                                                                                                                                                                                                                                                                                                          
            Object.defineProperty(o14, "getPrototypeOf", { enumerable: true, value: f6 });
            const v16 = new Proxy(e13, o14);
            v16.stack;                   
        }                                
        return f3;                       
    }                                    
    const v19 = new Promise(f3);         
}                                        
// CRASH INFO                                                                                                                                                                     
// ==========                                                                                                                                                                     
// TERMSIG: 11                           
// STDERR:                               
// Assertion failure: IsObjectValueInCompartment(v, compartment()), at /home/builder/firefox/js/src/vm/NativeObject.h:1083

Here is a part of the backtrace from gdb:

#0  0x0000555557525f2c in js::NativeObject::checkStoredValue (this=0x37c283201248, v=...) at js/src/vm/NativeObject.h:1083
#1  0x000055555769b65d in js::NativeObject::setSlot (this=0x37c283201248, slot=2, value=...) at js/src/vm/NativeObject.h:1091
#2  0x00005555576fba93 in js::EnvironmentObject::setAliasedBinding (this=0x37c283201248, slot=2, v=...) at js/src/vm/EnvironmentObject-inl.h:46
#3  0x00005555578cafd1 in js::EnvironmentObject::setAliasedBinding (this=0x37c283201248, bi=..., v=...) at js/src/vm/EnvironmentObject-inl.h:60
#4  0x00005555578a5971 in js::CallObject::create (cx=0x7ffff772d100, frame=...) at js/src/vm/EnvironmentObject.cpp:182
#5  0x00005555578b50d7 in js::InitFunctionEnvironmentObjects (cx=0x7ffff772d100, frame=...) at js/src/vm/EnvironmentObject.cpp:4001
#6  0x0000555557be104f in js::InterpreterFrame::initFunctionEnvironmentObjects (this=0x7ffff56fc1c8, cx=0x7ffff772d100) at js/src/vm/Stack.cpp:171
#7  0x0000555557be12ad in js::InterpreterFrame::prologue (this=0x7ffff56fc1c8, cx=0x7ffff772d100) at js/src/vm/Stack.cpp:189
#8  0x00005555576bd5ac in Interpret (cx=0x7ffff772d100, state=...) at js/src/vm/Interpreter.cpp:2131
#9  0x00005555576bd001 in js::RunScript (cx=0x7ffff772d100, state=...) at js/src/vm/Interpreter.cpp:431
#10 0x00005555576d4523 in js::InternalCallOrConstruct (cx=0x7ffff772d100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:579
#11 0x00005555576d4a99 in InternalCall (cx=0x7ffff772d100, args=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:614
#12 0x00005555576d4c49 in js::Call (cx=0x7ffff772d100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at js/src/vm/Interpreter.cpp:646
#13 0x0000555557ea4287 in js::ScriptedProxyHandler::getPrototype (this=0x5555596e1a50 <js::ScriptedProxyHandler::singleton>, cx=0x7ffff772d100, proxy=..., protop=...) at js/src/proxy/ScriptedProxyHandler.cpp:250
#14 0x0000555557ea00d2 in js::Proxy::getPrototype (cx=0x7ffff772d100, proxy=..., proto=...) at js/src/proxy/Proxy.cpp:297
#15 0x00005555578e0e1d in js::GetPrototype (cx=0x7ffff772d100, obj=..., protop=...) at js/src/vm/ObjectOperations-inl.h:50
#16 0x00005555578baed1 in FindErrorInstanceOrPrototype (cx=0x7ffff772d100, obj=..., result=...) at js/src/vm/ErrorObject.cpp:626
#17 0x00005555578ba890 in js::ErrorObject::getStack_impl (cx=0x7ffff772d100, args=...) at js/src/vm/ErrorObject.cpp:664
#18 0x00005555578ba740 in JS::CallNonGenericMethod<&(IsObject(JS::Handle<JS::Value>)), &js::ErrorObject::getStack_impl> (cx=0x7ffff772d100, args=...) at obj-debug/dist/include/js/CallNonGenericMethod.h:103
#19 0x00005555578ba6ac in js::ErrorObject::getStack (cx=0x7ffff772d100, argc=0, vp=0x7fffffff49a8) at js/src/vm/ErrorObject.cpp:656
#20 0x00005555576e604c in CallJSNative (cx=0x7ffff772d100, native=0x5555578ba660 <js::ErrorObject::getStack(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Getter, args=...) at js/src/vm/Interpreter.cpp:459
#21 0x00005555576d4313 in js::InternalCallOrConstruct (cx=0x7ffff772d100, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:547
#22 0x00005555576d4a99 in InternalCall (cx=0x7ffff772d100, args=..., reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:614
#23 0x00005555576d4c49 in js::Call (cx=0x7ffff772d100, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Getter) at js/src/vm/Interpreter.cpp:646
#24 0x00005555576d59d7 in js::CallGetter (cx=0x7ffff772d100, thisv=..., getter=..., rval=...) at js/src/vm/Interpreter.cpp:768
#25 0x0000555557a0f32a in CallGetter (cx=0x7ffff772d100, obj=..., receiver=..., id=..., prop=..., vp=...) at js/src/vm/NativeObject.cpp:2017

I'm not sure if this assertion failure has security implications, so I'm filing this as a security issue as a precaution.

Comment 1

[Nicolas B. Pierron [:nbp]]

I suspect RegEx are only part of this test case such that I can need-info Iain on our RegEx embedding such that he can discover a test case with all the greatest features of JavaScript.

However, looking at the stack trace, it sounds like this might be a miss-match between Proxies, with and possibly generators … So I guess Yulia might have a better understanding of the context.

(S2 as this is a bad compartment issue)

Comment 2

[Andrew McCreight [:mccr8]]

compartment mismatches can lead to UAFs, so I'll mark it sec-high. Feel free to clear the sec-high flag if this ends up being a shell only issue.

Comment 3

[Iain Ireland [:iain]]

Looking into this. It has to do with the interaction between error.stack and proxies. Reduced testcase:

var g = newGlobal({newCompartment: true});

try {
  undef()
} catch (err) {
  const handler = { "getPrototypeOf": (x) => () => x };
  const proxy = new g.Proxy(err, handler);
  proxy.stack
}

We call FindErrorInstanceOrPrototype with a CCW wrapping a scripted Proxy (allocated in g) wrapping a real error object (allocated in the main compartment). We unwrap the CCW, but since the target of the wrapper is a proxy, not an error, we try to get its prototype by calling GetPrototype on our proxy. (Somebody with a stronger understanding of the nuances of CCWs can correct me on this, but I think we've already gone wrong at this point.)

We retrieve the getPrototypeOf trap from the handler (both of which live in the main compartment). We call it with target (our unwrapped proxy from g) as the only argument. Because the argument to the lambda is captured in a closure, we store it in a CallObject. (In the original testcase, the trap captured its arguments because it was an async function.) This triggers an assertion, because we're storing an object from g's compartment in a CallObject belonging to the main compartment.

I am not confident that this is shell-only. The testcase only uses newGlobal to get access to the Proxy constructor from another compartment.

Refactoring the code so that we call GetPrototype on the original object instead of the unwrapped object fixes the bug, although I'm not confident that it's necessarily the best fix.

Comment 4

[Iain Ireland [:iain]]

Unrelated to the issue with cross-compartment wrappers, we could also consider doing something to see through scripted proxies in FindErrorInstanceOrPrototype.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1810711: Add testcase r=jandem!

Depends on D167105

Comment 7

[Iain Ireland [:iain]]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Close reading of the patch will indicate that we don't want to call GetPrototype on an unwrapped CCW. The patch doesn't point at a scripted proxy as the problematic wrapper target, but that's a pretty obvious thing to try out. I'm not sure how easy it is to exploit a cross-compartment pointer once you have one.
• 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 older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: It's relatively simple and targeted. Normal testing should be enough.
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Approved to land and request uplift

Comment 10

[Iain Ireland [:iain]]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Beta/Release Uplift Approval Request

• User impact if declined: Compartment mismatch potentially leading to UAF.
• 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): Simple targeted fix.
• 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: Sec-high
• User impact if declined: Compartment mismatch potentially leading to UAF.
• Fix Landed on Version: 111
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple targeted fix.

Comment 11

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

Refactor FindErrorInstanceOrPrototype r=jandem
https://hg.mozilla.org/integration/autoland/rev/4bb432829fb5d6814bea280cdf969efed5e616aa
https://hg.mozilla.org/mozilla-central/rev/4bb432829fb5

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Approved for 110 beta 6, thanks.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

We are not taking this one in the planned dot release.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9312745 [details]
Bug 1810711: Refactor FindErrorInstanceOrPrototype r=jandem!

Approved for 102.8esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/7ecf0a6e9e6e

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt

Comment 18

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

2 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2023-03-28] .

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8550deb25cd7