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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Andy Wingo [: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.

Comment 2

[Andy Wingo [:wingo]]

Also, thank you for the report!

Comment 3

[Jan de Mooij [:jandem]]

(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!

Comment 4

[Andy Wingo [:wingo]]

Attachment: Bug 1557655 - Fix size_t/uint64_t mismatch for StringToBigInt on 32-bit

Comment 5

[Andy Wingo [:wingo]]

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

Comment 6

[Andy Wingo [:wingo]]

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

Comment 7

[Jeff Walden [:Waldo]]

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. :-|

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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

Comment 9

[Andy Wingo [:wingo]]

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:

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/3be455e67a2c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3be455e67a2c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12c9d3b85c14

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.