1953609 - UBSan undefined-behavior error: addition of unsigned offset overflowed or Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:585

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug stack

function f() {
  Error.captureStackTrace(f, f);
}
bindToAsyncStack(f, { stack: saveStack() })();

(gdb) bt
#0  0x00005555576c6d4d in MOZ_CrashSequence (aAddress=0x0, aLine=585) at /home/ubu32gx500/shell-cache/js-dbg-64-linux-x86_64-99f5fa3c8fdf/objdir-js/dist/include/mozilla/Assertions.h:267
#1  mozilla::Vector<js::SavedFrame::Lookup, 60ul, js::TempAllocPolicy>::operator[] (this=0x7fffffffbcd8, aIndex=<optimized out>) at /home/ubu32gx500/shell-cache/js-dbg-64-linux-x86_64-99f5fa3c8fdf/objdir-js/dist/include/mozilla/Vector.h:585
#2  JS::GCVector<js::SavedFrame::Lookup, 60ul, js::TempAllocPolicy>::operator[] (this=0x7fffffffbcd8, i=<optimized out>) at /home/ubu32gx500/shell-cache/js-dbg-64-linux-x86_64-99f5fa3c8fdf/objdir-js/dist/include/js/GCVector.h:71
#3  js::MutableWrappedPtrOperations<JS::GCVector<js::SavedFrame::Lookup, 60ul, js::TempAllocPolicy>, JS::Rooted<JS::GCVector<js::SavedFrame::Lookup, 60ul, js::TempAllocPolicy> > >::operator[] (this=0x7fffffffbcc0, aIndex=<optimized out>) at /home/ubu32gx500/shell-cache/js-dbg-64-linux-x86_64-99f5fa3c8fdf/objdir-js/dist/include/js/GCVector.h:258
#4  js::SavedStacks::insertFrames (this=0x7ffff6498e70, cx=0x7ffff6f3a200, frame=..., capture=..., startAtObj=startAtObj@entry=...) at /home/ubu32gx500/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1626
#5  0x00005555576c51de in js::SavedStacks::saveCurrentStack (this=<optimized out>, cx=<optimized out>, frame=..., capture=..., startAt=...) at /home/ubu32gx500/trees/mozilla-central/js/src/vm/SavedStacks.cpp:1339
/snip

This seems related to https://hg.mozilla.org/integration/autoland/rev/5fdd7a5d986dbf8742e762f09d8ae7475aea0455 which shipped Error.captureStackTrace and is likely related to bug 1886820 when run with additional runtime flag --setpref=experimental.error_capture_stack_trace=true.

Run with --fuzzing-safe --no-threads --no-baseline --no-ion, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 99f5fa3c8fdf.

Matt, is bug 1886820 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

In an opt ASan build I get:

/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/objdir-js/dist/include/mozilla/Vector.h:586:12: runtime error: addition of unsigned offset to 0x7fff6b354a58 overflowed to 0x7fff6b3549f0
    #0 0x5cf6210b7fc2  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x2f48fc2) (BuildId: 859a8572d95e75ae72d984ceb168074a)
    #1 0x5cf6210b5662  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x2f46662) (BuildId: 859a8572d95e75ae72d984ceb168074a)
    #2 0x5cf6214366d0  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x32c76d0) (BuildId: 859a8572d95e75ae72d984ceb168074a)
    #3 0x5cf620df1c00  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x2c82c00) (BuildId: 859a8572d95e75ae72d984ceb168074a)
    #4 0x5cf620b5b9b3  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x29ec9b3) (BuildId: 859a8572d95e75ae72d984ceb168074a)
    #5 0x5cf620b79f97  (/home/ubu32gx500/shell-cache/js-64-asan-linux-x86_64-99f5fa3c8fdf/js-64-asan-linux-x86_64-99f5fa3c8fdf+0x2a0af97) (BuildId: 859a8572d95e75ae72d984ceb168074a)
/snip

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Shell used in comment #1 was compiled with AR=ar sh ../configure --enable-fuzzing --without-sysroot --enable-address-sanitizer --disable-jemalloc --disable-stdcxx-compat --enable-undefined-sanitizer --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests.

(oops, mid-aired with Tyson, was in the process of editing)

Comment 3

[Andrew McCreight [:mccr8]]

Error.captureStackTrace was shipped in Firefox in bug 1950508 so I'm going to mark that as the regressor.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Without --setpref=experimental.error_capture_stack_trace=true:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5fdd7a5d986d
user:        Matthew Gaudet
date:        Mon Mar 03 15:48:35 2025 +0000
summary:     Bug 1950508 - Ship Error.captureStackTrace r=dminor,mccr8

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Error.captureStackTrace was shipped in Firefox in bug 1950508 so I'm going to mark that as the regressor.

Yes, I mentioned https://hg.mozilla.org/integration/autoland/rev/5fdd7a5d986dbf8742e762f09d8ae7475aea0455 in comment #0 as well which points to bug 1950508.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/autoland/rev/5fdd7a5d986dbf8742e762f09d8ae7475aea0455 also landed in m-c as https://hg.mozilla.org/mozilla-central/rev/5fdd7a5d986dbf8742e762f09d8ae7475aea0455

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

With --setpref=experimental.error_capture_stack_trace=true:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c27c123c7051
user:        Matthew Gaudet
date:        Thu Jan 16 15:31:28 2025 +0000
summary:     Bug 1886820 - Add experimental support for Error.captureStackTrace r=jandem

Comment 8

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

Initial look: This is Bug 1945318 in another guise. I have a patch which -works- but needs validation, but I'm past EOD today, so I'll come back to this tomorrow.

I'll leave ni? as a reminder.

Comment 9

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

Some notes:

1. Test case is constructed with a testing function (bindToAsyncStack), but the general problem here exists of how this algorithm handles re-parenting when the stack filtering introduced via Error.captureStackTrace means that you have to deal with the possibility that there is no stack entry to re-parent when it comes time to add the async stack.

   In Bug 1945318 I tried to fix this by ensuring you'd -seen- a frame already. However, this is not sufficient for this test case. In this case, you have seen desired frame and yet the stack chain vector is still empty at this point, because this is the -first- frame which has set seenStartAt, so we didn't emplace.

2. What does this get you. Well, initially an attacker can write a SavedFrame* at wherever the vectors data is -1. In the inline case this can cause other corruption because I believe that's mozilla::Vector::CapacityAndReserved::mCapacity. In the out-of-line case, this can write over the malloc header. However! I think in this case, you never use the out-of-line case.

So: A way in which I think you could use this for an attack:

1. figure out a way to trip this without bindToAsyncStack;
2. Ensure we have async stack reparenting enabled. I believe this is only enabled with devtools right now or a non-standard pref. (Should validate if this is part of the severity calculation)
3. construct a call such that we do the bad overwrite. This will almost certainly inflate the capacity of the saved frame vector such that we may now overwrite the stack.
4. Construct a stack with eval frames such that we continue to walk stack frames -- vasically if you can reliably fill unreachedEvalTargets
5. Use script line numbers to control parts of your stack clobbering
6. Profit

This doesn't seem easy, but a determined attacker might be able to use this.

---

One error I made when fixing Bug 1945318 is that I should have recognized the danger of getting this wrong again, and added a MOZ_RELEASE_ASSERT(stackChain.length() > 0) to just before the array reference.

I need to work on this a bit more to provide a more concrete fix here: My first attempt fails some tests. Unfortunately my brain is turning to mush right now, so hopefully I'll have a working patch tomorrow.

Comment 10

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

Working on trying to see how exploitable this is, and I think we actually are getting saved by another release assert.

So based on the exploit plan above, I wrote the following test case:

function f() {
  Error.captureStackTrace(f, f);
}
let k = bindToAsyncStack(f, { stack: saveStack() });

function eval_recur(depth, callee) {
  if (depth == 0) {
    callee()
  } else {
    eval(`eval_recur(${depth - 1}, callee)`)
  }
}

eval_recur(14, k);

Now it turns out, I didn't catch a subtlety when writing up my previous comment: unreachedEvalTargets is only populated from debugger frames; which actually rules out writing more frames in general content. Combined with what comes next, I think it effectively neuters this bug

Next, my test case, even in a debug build hits an exception. While I do clobber something on the stack, what I clobber ends up being jit::InlineFrameIterator ionInlineFrames_; which is part of the stack allocated FrameIter in SavedStacks::insertFrames. part of what gets clobbered is the tag for the contained State Variant stored in MachineState -- this is what saves us: We have a release assert on tag type during Variant destroy, and becasue the tag is overwritten with garbage, we crash.

The value we're writing is a GC allocated pointer. There's some ability to groom bits, but it seems likely it would be to get the correct variant tag

So I think ultimately this fails safe.

Comment 11

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

Attachment: Bug 1953609 - Replace reparenting check with simpler choice r?jandem

Comment 12

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

Attachment: WIP: Bug 1953609 - Add test case

Comment 13

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

Ok, given this is disabled in beta and release I'm going to land the patch.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eb62a932d7b
Replace reparenting check with simpler choice r=jandem

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0eb62a932d7b

Can we land the test too?

Comment 16

[Daniel Veditz [:dveditz]]

We had to go a long way to decide this was probably fail-safe, but the patch is clearly safer than what was there so we'll call this sec-low and award a small bounty.