Closed Bug 1557655 Opened 1 year ago Closed 1 year ago

Assertion failure: charcount <= std::numeric_limits<size_t>::max() / bitsPerChar, at js/src/vm/BigIntType.cpp:1417

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 + fixed
firefox69 + fixed
firefox70 --- verified

People

(Reporter: decoder, Assigned: wingo)

Details

(6 keywords, Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 155a7e2117e5 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

const MaxStringLength = 2**30 - 2;
var s = "\u0390".repeat(MaxStringLength);
var x = BigInt(-1);
x > s;

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  JS::BigInt::calculateMaximumDigitsRequired (cx=0xf6e1f800, radix=10 '\n', charcount=1073741822, result=0xffffbc58) at js/src/vm/BigIntType.cpp:1417
#1  0x568cfec6 in JS::BigInt::parseLiteralDigits<char16_t> (cx=<optimized out>, chars=..., radix=10, isNegative=false, haveParseError=0xffffbd93) at js/src/vm/BigIntType.cpp:1452
#2  0x568d0180 in JS::BigInt::parseLiteral<char16_t> (cx=0xf6e1f800, chars=..., haveParseError=0xffffbd93) at js/src/vm/BigIntType.cpp:1507
#3  0x568bc993 in ParseStringBigIntLiteral<char16_t> (haveParseError=0xffffbd93, range=..., cx=0xf6e1f800) at js/src/vm/BigIntType.cpp:3191
#4  js::StringToBigInt (cx=0xf6e1f800, str=...) at js/src/vm/BigIntType.cpp:3212
#5  0x568bd977 in JS::BigInt::lessThan (cx=<optimized out>, lhs=..., rhs=..., res=...) at js/src/vm/BigIntType.cpp:3082
#6  0x568bdc0f in JS::BigInt::lessThan (cx=<optimized out>, lhs=..., rhs=..., res=...) at js/src/vm/BigIntType.cpp:3114
#7  0x567d3cfe in js::LessThanImpl (cx=0xf6e1f800, lhs=..., rhs=..., res=...) at js/src/vm/Interpreter-inl.h:735
#8  0x567dbedc in js::GreaterThanOperation (res=<optimized out>, rhs=..., lhs=..., cx=<optimized out>) at js/src/vm/Interpreter-inl.h:820
#9  Interpret (cx=0xf6e1f800, state=...) at js/src/vm/Interpreter.cpp:2470
#10 0x567e75f3 in js::RunScript (cx=<optimized out>, state=...) at js/src/vm/Interpreter.cpp:425
[...]
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11406
eax	0x57d25934	1473403188
ebx	0x57d24ff4	1473400820
ecx	0xf7d90864	-136771484
edx	0x577cb710	1467791120
esi	0x3ffffffe	1073741822
edi	0x57d24ff4	1473400820
ebp	0xffffbc18	4294949912
esp	0xffffbc10	4294949904
eip	0x568b4164 <JS::BigInt::calculateMaximumDigitsRequired(JSContext*, unsigned char, unsigned int, unsigned int*)+308>
=> 0x568b4164 <JS::BigInt::calculateMaximumDigitsRequired(JSContext*, unsigned char, unsigned int, unsigned int*)+308>:	movl   $0x0,0x0
   0x568b416e <JS::BigInt::calculateMaximumDigitsRequired(JSContext*, unsigned char, unsigned int, unsigned int*)+318>:	ud2

This issue is 32-bit only and looking at the code, a violation of this check can lead to an integer overflow. Marking s-s based on that.

Flags: needinfo?(wingo)

Hi, I agree with the analysis. I am out of the office until Friday though, and will file a fix on Friday, if that works for y'all.

Assignee: nobody → wingo
Flags: needinfo?(wingo)

Also, thank you for the report!

(In reply to Andy Wingo [:wingo] from comment #1)

I am out of the office until Friday though, and will file a fix on Friday, if that works for y'all.

Thanks!

Priority: -- → P1

Comment on attachment 9072162 [details]
Bug 1557655 - Fix size_t/uint64_t mismatch for StringToBigInt on 32-bit

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Pretty easy I think. Only on 32-bit tho
  • 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?: FF68
  • If not all supported branches, which bug introduced the flaw?: https://bugzilla.mozilla.org/show_bug.cgi?id=js-bigint-ship
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Should apply directly
  • How likely is this patch to cause regressions; how much testing does it need?: Low-risk
Attachment #9072162 - Flags: sec-approval?

This bug should be kept open until the tests can land.

You are learning all the fun arcane aspects of security bug processes! Ordinarily we land the fix and that's enough for FIXED -- then either a new bug is filed for the tests, or the patch author just keeps it in the back of the mind to land the tests at a later date. I believe it mucks with the release people's processes to have a non-FIXED bug in their tracking. :-|

sec-approval+ for trunk. Please nominate it for beta as well so we can avoid shipping this flaw at all!

Attachment #9072162 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed

Comment on attachment 9072162 [details]
Bug 1557655 - Fix size_t/uint64_t mismatch for StringToBigInt on 32-bit

Beta/Release Uplift Approval Request

  • User impact if declined: arbitrary code execution on 32-bit platforms. there are tests but they have not landed yet
  • Is this code covered by automated tests?: No
  • 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): Fixes security bug, small obvious patch => low risk
  • String changes made/needed:
Attachment #9072162 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9072162 [details]
Bug 1557655 - Fix size_t/uint64_t mismatch for StringToBigInt on 32-bit

Fixes a BigInt crash. Approved for 68rc1.

Attachment #9072162 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][post-critsmash-triage]
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.