1883542 - (CVE-2024-3852) Assertion failure: GetThisObject(thisObj) == thisObj

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Logan Stratton]

Attachment: POC to trigger assertion wihtin debug builds

Steps to reproduce:

The attached sample is a minimized sample from fuzzing an older version of the spider monkey engine. This sample causes the assertion error to be triggered within a debug build of the Spidermonkey engine. This was confirmed to still be trigger-able on a new build as well as build from over a year ago. Apologies for not directly narrowing down to see when this was added. The below code block identifies the output when running the sample.

Assertion failure: GetThisObject(thisObj) == thisObj, at ./js/src/vm/Interpreter.cpp:636
#01: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da3be0]
#02: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1db47c5]
#03: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da211f]
#04: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da56fc]
#05: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x25367c5]
#06: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x25384af]
#07: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x253c712]
#08: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x2552d7b]
#09: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1dcba05]
#10: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da2bab]
#11: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da3d3e]
#12: js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x24b71ce]
#13: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x2491920]
#14: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x24a5fe7]
#15: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x1da3135]
#16: ???[./obj-x86_64-pc-linux-gnu/dist/bin/js +0x29a252c]
#17: ??? (???:???)
zsh: segmentation fault ./obj-x86_64-pc-linux-gnu/dist/bin/js ~/POC_assertion.js

Actual results:

This crash is believed to be caused during the Baseline Interpreter Compilation phase. Excuse the target component as we were unsure if this direct component is under the JIT sub category.

This was confirmed during testing as including --no-blinterp would stop the incorrect optimization to occur. Unfortunately we were not able to further narrow down the bug from this.

We believe this to be directly related to the debugging framework as noted by the use of the debug functionality within the POC, as such we do not believe the security impact is major, but will hold off further any further assumptions.

The second POC demonstrates utilizing this bug to gain an incorrect JIT range analysis through an incorrect assumption made from the result of the exec.

Comment 1

[Logan Stratton]

Attachment: POC_JIT.js

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Bryan could you take a look?

Comment 3

[Daniel Veditz [:dveditz]]

Looks like Jan added this assertion in bug 1765773.

The shell doesn't have inner/outer windows so maybe this is something funky with the testcase or a testing function in js shell?

Comment 4

[Iain Ireland [:iain]]

I poked at this a bit. Here's a slightly cleaned-up version:

function peekAtThis(){
  const g = newGlobal({sameZoneAs: this});
  const dbg = g.Debugger(this);
  const frame = dbg.getNewestFrame().older;
  frame.eval("this.newString()")
}

Object.defineProperty(this, "getter", {get: foo});

let i = 20;
function foo() {
  eval("");
  peekAtThis();
  if (i--) {
    getter++;
  }
}
foo();

We attach a getter to the global object that calls foo. Inside foo, we use an empty eval (a with works just as well) so that getter++ uses BindName and GetBoundName. foo calls itself recursively, so we loop until we tier up. It doesn't seem to matter whether it's blinterp or baseline; the key is that we start attaching ICs.

Initially, we always call the getter with the window proxy as this. After a couple of iterations in blinterp/baseline to attach ICs, we suddenly start passing in the bare global. I think we may have already gone wrong at this point, but the only way I've found to actually trigger a crash involves using the debugger to read the this value out of the frame, and calling a function on it. (newString is arbitrary; I picked it because it is faster than newGlobal.)

Out of time for today; will dig deeper tomorrow.

Comment 5

[Jan de Mooij [:jandem]]

I'll forward this to Iain because he has been looking into this already.

Comment 6

[Iain Ireland [:iain]]

Turns out it's definitely possible to trigger this without the debugger.

Object.defineProperty(this, "getter", {get: foo});

var output;
let i = 20;
function foo() {
  eval("");
  output = this;
  if (i--) {
    getter++;
  }
}
foo();
output.newString();

It seems like there's a disconnect between BindName and GetBoundName when it comes to getters and this. If BindName doesn't find a binding before the global, then it will return the bare global object (not the WindowProxy). I assume that this is generally okay because the environment we push here isn't directly exposed to script. This also seems to be what BindGName does in the same circumstances. However, this doesn't seem to play nicely with our IC attach logic for GetBoundName. Our WindowProxy-specific code generally seems to expect that tryAttach code will see a WindowProxy and maybe unwrap it. In this case, the global object isn't a proxy, so we don't trigger any of the WindowProxy-specific code. Instead, we treat it like a normal NativeObject and attach a normal getter.

One potential fix I considered was adding a check in IsCacheableGetPropCall to avoid attaching getters on the global, only to realize that we used to have that check, but removed it in bug 1805199. I'm pretty sure that change caused this regression. In another patch on that bug, we handled this case for setters by hardcoding the receiver if we need a WindowProxy. The same approach might work here.

Security-wise, I'm not entirely clear what the implications are of giving access to the global directly instead of through the window proxy, but I don't think it's very good.

Comment 7

[Jan de Mooij [:jandem]]

JSOp::GetBoundName is a pretty uncommon op (emitted for inc/dec operations with unknown environment chains IIRC?) that we already check for in some places (IsCacheableNoProperty), so going with the simplest fix will probably be fine.

Maybe GetBoundName should have its own CacheIR generator at some point to avoid these footguns.

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1883542: Simplify GetBoundName ICs r=jandem

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1883542: Add test and assertion r=jandem

I ran a one-line patch containing only the assertion through ./mach try auto and everything passed.

Comment 10

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

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

Comment 11

[Iain Ireland [:iain]]

Comment on attachment 9390310 [details]
Bug 1883542: Simplify GetBoundName ICs r=jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Fairly difficult. This patch doesn't say anything about WindowProxy.
• 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?: No
• If not, how different, hard to create, and risky will they be?: It looks like the patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It disables ICs in a rare corner case.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 12

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

The bug is marked as tracked for firefox125 (nightly). We have limited time to fix this, the soft freeze is in a day. However, the bug still has low priority and has low severity.

:sdetar, could you please increase the priority and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 13

[Steven DeTar [:sdetar]]

Iain, do you have any thoughts on comment 12?

Comment 14

[Iain Ireland [:iain]]

I bumped up the priority and severity. I've marked it S1 under the assumption that leaking the global is a security problem. If that's not true then we could decrease the severity.

Backing out the regressor, which landed a year ago, is not a reasonable option.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

S1 generally means chemspill severity. I'm going to assume that's not the case and set this to S2 instead.
https://wiki.mozilla.org/BMO/UserGuide/BugFields#bug_severity

Comment 16

[Tom Ritter [:tjr]]

Comment on attachment 9390310 [details]
Bug 1883542: Simplify GetBoundName ICs r=jandem

sec-approvals were paused for a few days after merge, thanks for the patience. approved to land and uplift

Comment 17

[Iain Ireland [:iain]]

Attachment: Bug 1883542: Simplify GetBoundName ICs r=jandem

Original Revision: https://phabricator.services.mozilla.com/D204130

Comment 18

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Explanation of risk level: Disables ICs in a rare corner case
• Code covered by automated testing: yes
• Needs manual QE test: no
• Is Android affected?: yes
• Steps to reproduce for manual QE testing: None
• Fix verified in Nightly: yes
• User impact if declined: Global object exposed directly (instead of WindowProxy)
• Risk associated with taking this patch: Low

Comment 19

[Iain Ireland [:iain]]

Attachment: Bug 1883542: Simplify GetBoundName ICs r=jandem

Original Revision: https://phabricator.services.mozilla.com/D204130

Comment 20

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: Global object exposed directly (instead of WindowProxy)
• Risk associated with taking this patch: Low
• String changes made/needed: None
• Code covered by automated testing: yes
• Explanation of risk level: Disables ICs in a rare corner case
• Is Android affected?: yes
• Needs manual QE test: no
• Fix verified in Nightly: yes
• Steps to reproduce for manual QE testing: None

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7037aea3d72
Simplify GetBoundName ICs r=jandem

Comment 22

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

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

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/0da9fe75c111

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/316e40fc4d59

Comment 25

[Logan Stratton]

As the status of this bug has been set to resolved. Could credit for this find be attributed to Andrew Kramer and Logan Stratton.
Thanks

Comment 26

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt

Comment 27

[Logan Stratton]

Looking at the advisory.txt recently attached could Andrew Kramer be added as well to the attribution? Also would this fall under enough criteria to be considered for a CVE designation?

Comment 28

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

2 months ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-05-28] .

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0984863a044f
Add test and assertion r=jandem

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/0984863a044f

Comment 31

[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