Closed Bug 1821959 (CVE-2023-29536) Opened 9 months ago Closed 8 months ago

MOZ_DIAGNOSTIC_ASSERT in mozjemalloc from background thread free()

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox-esr102 112+ fixed
firefox111 --- wontfix
firefox112 + fixed
firefox113 + fixed

People

(Reporter: phambao1340, Assigned: tcampbell)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+])

Attachments

(5 files)

Bug 1821959 - Use unshiftedElementsHeader when transplanting objects. r?jandem!
48 bytes, text/x-phabricator-request
tjr
: sec-approval+
Details | Review
Bug 1821959 - Add test case. r?jandem!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1821959 - Use unshiftedElementsHeader when transplanting objects. r?jandem!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1821959 - Use unshiftedElementsHeader when transplanting objects. r?jandem!
48 bytes, text/x-phabricator-request
Details | Review
advisory.txt
246 bytes, text/plain
Details

When run this following javascript code, gecko fail at mozjemalloc

const v1 = this.transplantableObject();
const v2 = v1.object;
for (let v3 = 0; v3 < 100; v3++) {
    const v5 = String.fromCharCode();
    const v8 = [v5];
    Reflect.apply(([v5,v5,v5,v5,v5]).unshift, v2, v8);
}
v1.transplant(newGlobal());

DEBUG


/*
Assertion failure: diff == regind * size, at /home/s/gecko-dev/memory/build/mozjemalloc.cpp:2515  

In file: /home/s/gecko-dev/memory/build/mozjemalloc.cpp 
   2510 
   2511   MOZ_ASSERT(diff <=
   2512              (static_cast<unsigned>(bin->mRunSizePages) << gPageSize2Pow));
   2513   regind = diff / bin->mSizeDivisor;
   2514 
 ► 2515   MOZ_DIAGNOSTIC_ASSERT(diff == regind * size);
   2516   MOZ_DIAGNOSTIC_ASSERT(regind < bin->mRunNumRegions);
   2517 
   2518   elm = regind >> (LOG2(sizeof(int)) + 3);
   2519   if (elm < run->mRegionsMinElement) {
   2520     run->mRegionsMinElement = elm;
──────────────────────────────────────────────────────────────────────────────────────────────────────────[ STACK ]──────────────────────────────────────────────────────────────────────────────────────────────────────────
00:0000│ rsp 0x7ffff6bff9b0 —▸ 0x7ffff7900d80 ◂— 0x947d3d24
01:0008│     0x7ffff6bff9b8 —▸ 0x7ffff6e07001 ◂— 0x880000001f384adf
02:0010│     0x7ffff6bff9c0 —▸ 0x7ffff7900d80 ◂— 0x947d3d24
03:0018│     0x7ffff6bff9c8 —▸ 0x7ffff6e00000 —▸ 0x7ffff7900d80 ◂— 0x947d3d24
04:0020│     0x7ffff6bff9d0 —▸ 0x7ffff7900da0 ◂— 0x2
05:0028│     0x7ffff6bff9d8 —▸ 0x7ffff6e0a858 ◂— 0xe5e5e5e5e5e5e5e5
06:0030│ rbp 0x7ffff6bff9e0 —▸ 0x7ffff6bffa20 —▸ 0x7ffff6bffb50 —▸ 0x7ffff6bffba0 —▸ 0x7ffff6bffc20 ◂— ...
07:0038│     0x7ffff6bff9e8 —▸ 0x555556dab580 (arena_dalloc(void*, unsigned long, arena_t*)+176) ◂— jmp 0x555556dab5c6
────────────────────────────────────────────────────────────────────────────────────────────────────────[ BACKTRACE ]────────────────────────────────────────────────────────────────────────────────────────────────────────
 ► f 0   0x555556db066d arena_t::DallocSmall(arena_chunk_t*, void*, arena_chunk_map_t*)+1021
   f 1   0x555556db066d arena_t::DallocSmall(arena_chunk_t*, void*, arena_chunk_map_t*)+1021
   f 2   0x555556dab580 arena_dalloc(void*, unsigned long, arena_t*)+176
   f 3   0x55555777669d js::gc::GCRuntime::freeFromBackgroundThread(js::AutoLockHelperThreadState&)+781
   f 4   0x55555777669d js::gc::GCRuntime::freeFromBackgroundThread(js::AutoLockHelperThreadState&)+781
   f 5   0x55555777669d js::gc::GCRuntime::freeFromBackgroundThread(js::AutoLockHelperThreadState&)+781
   f 6   0x5555577043e6 js::GCParallelTask::runTask(JS::GCContext*, js::AutoLockHelperThreadState&)+118
   f 7   0x5555577046ac js::GCParallelTask::runHelperThreadTask(js::AutoLockHelperThreadState&)+172
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
pwndbg> bt
#0  arena_run_reg_dalloc (run=<optimized out>, bin=<optimized out>, ptr=<optimized out>, size=<optimized out>) at /home/s/gecko-dev/memory/build/mozjemalloc.cpp:2515
#1  arena_t::DallocSmall (this=this@entry=0x7ffff7900d80, aChunk=aChunk@entry=0x7ffff6e00000, aPtr=aPtr@entry=0x7ffff6e0a858, aMapElm=<optimized out>) at /home/s/gecko-dev/memory/build/mozjemalloc.cpp:3652
#2  0x0000555556dab580 in arena_dalloc (aPtr=0x7ffff6e0a858, aOffset=<optimized out>, aArena=<optimized out>) at /home/s/gecko-dev/memory/build/mozjemalloc.cpp:3743
#3  0x000055555777669d in js_free (p=0x7ffff7c4ea60 <_IO_stdfile_2_lock>) at /home/s/gecko-dev/obj-fuzzbuild/dist/include/js/Utility.h:414
#4  JS::GCContext::freeUntracked (p=0x7ffff7c4ea60 <_IO_stdfile_2_lock>, this=<optimized out>) at /home/s/gecko-dev/js/src/gc/GCContext.h:117
#5  js::gc::GCRuntime::freeFromBackgroundThread (this=0x7ffff6e23728, lock=...) at /home/s/gecko-dev/js/src/gc/Sweeping.cpp:477
#6  0x00005555577043e6 in js::GCParallelTask::runTask (this=this@entry=0x7ffff6e250b8, gcx=gcx@entry=0x7ffff6bffbb0, lock=...) at /home/s/gecko-dev/js/src/gc/GCParallelTask.cpp:209
#7  0x00005555577046ac in js::GCParallelTask::runHelperThreadTask (this=0x7ffff6e250b8, lock=...) at /home/s/gecko-dev/js/src/gc/GCParallelTask.cpp:193
#8  0x0000555556fe8967 in js::GlobalHelperThreadState::runTaskLocked (this=this@entry=0x7ffff6e0f000, task=0x7ffff7c4ea60 <_IO_stdfile_2_lock>, locked=...) at /home/s/gecko-dev/js/src/vm/HelperThreads.cpp:2767
#9  0x0000555556fe8710 in js::GlobalHelperThreadState::runOneTask (this=0x7ffff6e0f000, lock=...) at /home/s/gecko-dev/js/src/vm/HelperThreads.cpp:2736
#10 0x0000555556ffa9a2 in js::HelperThread::threadLoop (this=this@entry=0x7ffff6e1c0a0, pool=pool@entry=0x7ffff6e17200) at /home/s/gecko-dev/js/src/vm/InternalThreadPool.cpp:282
#11 0x0000555556ffa75c in js::HelperThread::ThreadMain (pool=0x7ffff6e17200, helper=0x7ffff6e1c0a0) at /home/s/gecko-dev/js/src/vm/InternalThreadPool.cpp:225
#12 0x000055555700cee8 in js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::callMain<0ul, 1ul> (this=0x7ffff6e195f0) at /home/s/gecko-dev/js/s
rc/threading/Thread.h:220
#13 js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::Start (aPack=0x7ffff6e195f0) at /home/s/gecko-dev/js/src/threading/Thread.h:209
#14 0x00007ffff7ac7b43 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#15 0x00007ffff7b59a00 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

*/
Flags: sec-bounty?
Group: firefox-core-security → javascript-core-security
Component: Security → JavaScript Engine
Product: Firefox → Core
Summary: [Security] Gecko fail on mozjemalloc → MOZ_DIAGNOSTIC_ASSERT in mozjemalloc from background thread free()

Whether this is a security bug or not depends on whether the transplantableObject() mechanism in the jsshell is the source of the problem, or merely exposing a problem that could be accessible from web JavaScript.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(tcampbell)

This is a nice find. I'm investigating what the consequences in full browser are, but it looks like a real bug.

Simplified test case:

const v1 = this.transplantableObject();
const v2 = v1.object;
Array.prototype.push.call(v2, 0);
Array.prototype.push.call(v2, 0);
Array.prototype.shift.call(v2);
v1.transplant(newGlobal());

There is definitely a defect in the swap code with us forgetting to use unshiftedElementsHeader to get the base of the allocation. https://searchfox.org/mozilla-central/rev/f078cd02746b29652c134b144f0629d47e378166/js/src/vm/JSObject.cpp#1048,1102

Assignee: nobody → tcampbell
Flags: needinfo?(tcampbell)
Blocks: sm-security
Severity: -- → S3
Priority: -- → P1

I can reproduce this in web content. We end up calling free on an address in the middle of user-controlled data, so this probably is exploitable in content process. This is likely an old bug and certainly reproduces in ESR-102.

status-firefox111: --- → affected
status-firefox112: --- → affected
status-firefox113: --- → affected
status-firefox-esr102: --- → affected
Keywords: sec-high

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:tcampbell, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Flags: needinfo?(tcampbell)

The bug is marked as tracked for firefox112 (beta) and tracked for firefox113 (nightly). However, the bug still has low severity.

:sdetar, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Flags: needinfo?(sdetar)
Severity: S3 → S2
Flags: needinfo?(tcampbell)
Flags: needinfo?(sdetar)

Comment on attachment 9324220 [details]
Bug 1821959 - Use unshiftedElementsHeader when transplanting objects. r?jandem!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: A careful reader would notice we are freeing a pointer inside a buffer instead of the pointer to start of buffer which highlights the bug. Building the scenario to hit this in the browser is not obvious, but with some care could be constructed by a non-jit-expert. Finally, exploiting an allocator with a bad free is a tricky but not uncommon attack vector. The content sandbox still protects us though.
  • 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?: This rebases onto ESR, Release, Beta channels cleanly.
  • How likely is this patch to cause regressions; how much testing does it need?: Regression risk is low. We were using what is clearly the wrong API and have replaced it with the correct API that is already heavily used in similar cases.
  • Is Android affected?: Yes
Attachment #9324220 - Flags: sec-approval?

Comment on attachment 9324220 [details]
Bug 1821959 - Use unshiftedElementsHeader when transplanting objects. r?jandem!

Approved to request uplift and land.

Attachment #9324220 - Flags: sec-approval? → sec-approval+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23]

https://hg.mozilla.org/mozilla-central/rev/56605de82f79

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 8 months ago
status-firefox113: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

The patch landed in nightly and beta is affected.
:tcampbell, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(tcampbell)

Manually confirmed that crash/bug is resolved on today's Nightly. Moz-regression also correctly identifies my patch as the point it is fixed. I don't see any new crashes involving these function signatures either.

Flags: needinfo?(tcampbell)

Uplift Approval Request

  • String changes made/needed: No
  • Is Android affected?: yes
  • Code covered by automated testing: yes
  • Explanation of risk level: Specific code branches are rarely hit. Replace incorrect helper function call with the correct one that is already used in well tested code.
  • Risk associated with taking this patch: low
  • User impact if declined: sec-high bug that can crash and maybe control a content process
  • Fix verified in Nightly: yes
  • Steps to reproduce for manual QE testing: n/a
  • Needs manual QE test: no

Uplift Approval Request

  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: n/a
  • Fix verified in Nightly: yes
  • User impact if declined: sec-high bug may crash or control a content process
  • Risk associated with taking this patch: low
  • Explanation of risk level: Specific code branches are rarely hit. Replace incorrect helper function call with the correct one that is already used in well tested code.
  • Code covered by automated testing: yes
  • Is Android affected?: yes
  • String changes made/needed: no

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Flags: qe-verify-
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage]
Flags: sec-bounty? → sec-bounty+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage][adv-main112+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage][adv-main112+] → [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage][adv-main112+][adv-esr102.10+]
Attached file advisory.txt —
Alias: CVE-2023-29536

Minor correction to the advisory. This doesn't really have anything to do with the JITs. Could just title it "Invalid free from JavaScript code" or something to that effect.

a month ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2023-05-23] .

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

Flags: needinfo?(tcampbell)
Whiteboard: [reporter-external] [client-bounty-form] [verif?][reminder-test 2023-05-23][post-critsmash-triage][adv-main112+][adv-esr102.10+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main112+][adv-esr102.10+]

Testcase pushed. Thanks, BugBot.

Flags: needinfo?(tcampbell)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.