Closed Bug 1551128 Opened 2 years ago Closed 2 years ago

Crash [@ JS::BigInt::digit] or Assertion failure: idx < storage_.size(), at dist/include/mozilla/Span.h:679 with BigInt

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- fixed
firefox69 --- verified

People

(Reporter: gkw, Assigned: wingo)

References

(Regression)

Details

(7 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision b83d8a064f16 (build with AR=ar 'CC="clang -m32 -msse2 -mfpmath=sse"' 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh ./configure --target=i686-pc-linux --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

function g(f) {
    f(-Number.MIN_SAFE_INTEGER, BigInt(-1));
}
g(BigInt.asUintN);

Backtrace:

#0 mozilla::Span<unsigned int, 4294967295u>::operator[] (this=<optimized out>, idx=<optimized out>) at /home/ubuntu/shell-cache/js-dbg-32-linux-x86_64-b83d8a064f16/objdir-js/dist/include/mozilla/Span.h:679
#1 JS::BigInt::digit (this=<optimized out>, idx=<optimized out>) at js/src/vm/BigIntType.h:75
#2 JS::BigInt::truncateAndSubFromPowerOfTwo (cx=<optimized out>, x=..., bits=9007199254740991, resultNegative=<optimized out>) at js/src/vm/BigIntType.cpp:2260
#3 0x577729ae in JS::BigInt::asUintN (cx=0xf6b1c800, x=..., bits=9007199254740991) at js/src/vm/BigIntType.cpp:2309
#4 0x576aa3f5 in js::BigIntObject::asUintN (cx=0xf6b1c800, argc=2, vp=0xf66770d0) at js/src/builtin/BigInt.cpp:167
#5 0x57663302 in CallJSNative (cx=0xf6b1c800, native=0x576aa230 <js::BigIntObject::asUintN(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:443
/snip

For detailed crash information, see attachment.

This also crashes opt shell [@ mozilla::Span<unsigned int, 4294967295u>::operator[]] with [@ JS::BigInt::digit] on the stack. Not sure if this is entirely benign so setting s-s as a start.

Crash Signature: [@ JS::BigInt::digit]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/4b74d76e55a8
user: Andy Wingo
date: Wed Feb 06 13:41:56 2019 +0000
summary: Bug 1522436 - Enable BigInt compilation by default r=jandem,terpri,froydnj

Andy, guessing this is BigInt-related?

Flags: needinfo?(wingo)
Regressed by: 1522436
Assignee: nobody → wingo
Flags: needinfo?(wingo)

We can simplify the example to:

BigInt.asUintN(2**52, -1n)

Which tries to represent an all-bits-set bigint as a 2**52-bit value -- this is impossible as it's too much memory.

On x86-64, this example fails "normally" with:

js> BigInt.asUintN(2**52, -1n)
typein:7:8 RangeError: BigInt is too large to allocate
Stack:
  @typein:7:8

But I see that the failure is on 32-bit (x86). Surely an integer overflow error in some intermediate math :( Will investigate.

So, this bug comes from silently truncating a uint64_t bit-length value to a size_t. I have done a small audit over the code and I think this pattern only happens in the asUintN and asIntN APIs -- usually bit length is a size_t, and when a bit length comes from a bigint (and not from the user), it's in the range [0,MaxBitLength]. But these asUintN/asIntN APIs take a bit length directly, as an "index", meaning they are in [0,2**53-1].

This is perfectly valid in some cases, as e.g. BigInt.asUintN(2**53-1, 0n) -- i.e. the bigint 0n truncated to 2**53-1 bits -- is just zero. Truncating a positive bigint, whether to a signed or unsigned two's complement int, can only decrease the int's size. No vulnerability there.

However truncating a negative bigint (e.g. -1n) to a positive uintn can effectively fill a big bigint with a source of cheap 1 bits. That's the case that was mishandled and that's where this patch adds a fix. Note again that the mishandling was only on 32-bit systems.

A kind request to jwalden to review :)

Flags: needinfo?(jwalden)
Priority: -- → P1

FWIW here's a try build on linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5746a10c397aa457e52dc9adb28e30f799b293f Seems to fix the issue.

Keywords: checkin-needed

(We discussed this a bit on IRC. I pushed it without sec-approval because it only affects Nightly: BigInt is disabled in 67.)

https://hg.mozilla.org/integration/autoland/rev/1e82c40506077c6d5864a658b3585b0b2f2d8631

Flags: needinfo?(jwalden)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 786f094a30ae).
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update]
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.