Closed Bug 1834711 (CVE-2023-37202) Opened 1 year ago Closed 1 year ago

Assertion failure: IsBackgroundFinalizedWhenTenured(a) == IsBackgroundFinalizedWhenTenured(b), at /builds/worker/checkouts/gecko/js/src/vm/JSObject.cpp:1226

Categories

(Core :: JavaScript Engine, defect)

defect

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox-esr102 115+ fixed
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 + fixed
firefox116 + fixed

People

(Reporter: phambao1340, Assigned: jonco)

References

(Regression)

Details

(Keywords: regression, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main115+][adv-esr102.13+])

Attachments

(4 files)

Run following javascript code

const v3 = newGlobal();
v3.nukeAllCCWs();
const v13 = this.transplantableObject(v3);
const v27 = newGlobal({"newCompartment": true});

try {
    const t45 = v27.DataView;
    const v29 = new t45(v27);
} catch(e30) {
}

v27.firstGlobalInCompartment(v13.object);
v13.transplant(v3);

Stacktrace

#0  0x0000555556f4913d in JSObject::swap(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, js::AutoEnterOOMUnsafeRegion&) ()
#1  0x00005555572c903d in js::RemapDeadWrapper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) ()
#2  0x00005555572c8ba4 in js::RemapWrapper(JSContext*, JSObject*, JSObject*) ()
#3  0x00005555572c9770 in js::RemapAllWrappersForObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) ()
#4  0x0000555557270f25 in JS_TransplantObject(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) ()
#5  0x0000555556bd9808 in TransplantObject(JSContext*, unsigned int, JS::Value*) ()
#6  0x0000555556d0dff6 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#7  0x0000555556d0d786 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#8  0x0000555556d1bc1b in js::Interpret(JSContext*, js::RunState&) ()
#9  0x0000555556d0ccda in js::RunScript(JSContext*, js::RunState&) ()
#10 0x0000555556d1045b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) ()
#11 0x0000555556d109a0 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) ()
#12 0x0000555556e55120 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) ()
#13 0x0000555556e5535c in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) ()
#14 0x0000555556be2ae9 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) ()
#15 0x0000555556be20b0 in Process(JSContext*, char const*, bool, FileKind) ()
#16 0x0000555556b7d20d in Shell(JSContext*, js::cli::OptionParser*) ()
#17 0x0000555556b76fb0 in main ()
Flags: sec-bounty?
Group: firefox-core-security → javascript-core-security
Component: Security → JavaScript Engine
Product: Firefox → Core

This doesn't seem like the kind of thing content could do. Some of the actions could be caused to happen by opening and closing windows, but probably not with the control needed in this testcase.

Hey Jon,

This bug seems to be related to last year's work to transplant nursery objects (bug 1765338). It strikes me as potentially shell only, but may be worth a look.

Flags: needinfo?(jcoppeard)
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Keywords: regression
Regressed by: 1765338

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

The background finalized flag wasn't getting set in the second overload of
NewDeadProxyObject. The patch makes the first overload of this function more
generic so it can accept non-proxy arguments and uses it in all cases.

The testcase also results in dead object proxies being returned from rewrap()
in RemapDeadWrapper which previously cased an assertion. I added an early
return for this case - do you think that's OK?

Depends on D179576

Jon: is this a security problem for Firefox? Or is Matthew right this is shell (and chrome?) only?

Flags: needinfo?(jcoppeard)

(In reply to Daniel Veditz [:dveditz] from comment #6)
The testcase relies on shell test functionality to reproduce the problem.

I don't know whether this is possible in the browser. It would require transplanting an object after we have nuked all CCWs for the current compartment. I suspect that is not possible, but I don't know for sure.

Flags: needinfo?(jcoppeard)

Peter, can we run JS code that could do a transplant after we nuke a window in WindowDestroyedEvent? My vague recollection is that we are still running JS at that point, and we're only killing chrome to content references when we do that, so you could still have a reference to another global to transplant into or something? Thanks.

Flags: needinfo?(peterv)

Comment on attachment 9336811 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r?jandem

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Extremely 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 older supported branches are affected by this flaw?: Everything back to 101
  • 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?: This patch should apply but I haven't tested this.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikley. This cleans up setting flags for dead object proxy to make it more obviously correct.
  • Is Android affected?: Yes
Attachment #9336811 - Flags: sec-approval?

I've requested approval to land the patch, but I don't know what sec rating this should be. I'm not even sure this is something that could be triggered in the browser.

Comment on attachment 9336811 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r?jandem

Approved to land and request uplift

Attachment #9336811 - Flags: sec-approval? → sec-approval+

If you do need to uplift it here's a reminder for landing tests.

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15]

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

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch

It would be good to get confirmation from Peter, but I think this is at least in the ballpark of something that could really happen on a web page if you worked at it hard enough, so I'm going to mark it sec-high.

Keywords: sec-high

Please nominate this for Beta approval when you get a chance. It'll also need a rebased patch for ESR102.

Flags: needinfo?(jcoppeard)

Comment on attachment 9336811 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r?jandem

Beta/Release Uplift Approval Request

  • User impact if declined: Possible crash / security vulnerability.
  • 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): This change fixes the logic around creating dead objects proxies and the result is simpler and more obviously correct than before. This change has been on central for a week with no regressions found.
  • String changes made/needed:
  • Is Android affected?: Yes
Flags: needinfo?(jcoppeard)
Attachment #9336811 - Flags: approval-mozilla-beta?

The background finalized flag wasn't getting set in the second overload of
NewDeadProxyObject. The patch makes the first overload of this function more
generic so it can accept non-proxy arguments and uses it in all cases.

The testcase also results in dead object proxies being returned from rewrap()
in RemapDeadWrapper which previously cased an assertion. I added an early
return for this case - do you think that's OK?

Comment on attachment 9339088 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r=jandem (ESR102)

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug
  • User impact if declined: Possible crash / security vulnerability.
  • Fix Landed on Version: 116
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change fixes the logic around creating dead objects proxies and the result is simpler and more obviously correct than before. This change has been on central for a week with no regressions found.
Attachment #9339088 - Flags: approval-mozilla-esr102?

Comment on attachment 9336811 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r?jandem

Approved for 115.0b6.

Attachment #9336811 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9339088 [details]
Bug 1834711 - Set background finalized flag for dead object proxes created after nuking all CCWs r=jandem (ESR102)

Approved for 102.13esr.

Attachment #9339088 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15][adv-main115+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15][adv-main115+] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15][adv-main115+][adv-esr102.13+]
Flags: sec-bounty? → sec-bounty+

We'll go ahead and assume this is really triggerable from content and therefore sec-high for bug bounty purposes, but if we get proof of that (a POC that runs in a web page) we can raise the bounty amount we are awarding.

Flags: needinfo?(peterv)
Attached file advisory.txt
Alias: CVE-2023-37202
QA Whiteboard: [post-critsmash-triage]

2 months ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2023-08-15] .

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

Flags: needinfo?(jcoppeard)
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-08-15][adv-main115+][adv-esr102.13+] → [reporter-external] [client-bounty-form] [verif?][adv-main115+][adv-esr102.13+]
Flags: needinfo?(jcoppeard) → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: