Closed Bug 1254622 Opened 5 years ago Closed 5 years ago
Crash [@ js::Preliminary
Object Array::register New Object] with use-after-free
1.93 KB, text/plain
1.17 KB, patch
|Details | Diff | Splinter Review|
The attached testcase crashes on mozilla-central revision b6acf4d4fc20 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-extra-checks). Backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 js::PreliminaryObjectArray::registerNewObject (this=0xe5e5e5e5e5e5e5e5, res=res@entry=0x7fc8683691a0) at js/src/vm/TypeInference.cpp:3346 #1 0x0000000000b6c742 in js::TypeNewScript::registerNewObject (this=this@entry=0x7fc864993fd0, res=res@entry=0x7fc8683691a0) at js/src/vm/TypeInference.cpp:3605 #2 0x000000000094f95c in CreateThisForFunctionWithGroup (newKind=js::TenuredObject, group=..., cx=0x7fc866d07800) at js/src/jsobj.cpp:936 #3 js::CreateThisForFunctionWithProto (cx=cx@entry=0x7fc866d07800, callee=..., callee@entry=..., newTarget=..., newTarget@entry=..., proto=..., proto@entry=..., newKind=newKind@entry=js::GenericObject) at js/src/jsobj.cpp:975 #4 0x000000000094fea6 in js::CreateThisForFunction (cx=cx@entry=0x7fc866d07800, callee=callee@entry=..., newTarget=..., newTarget@entry=..., newKind=js::GenericObject) at js/src/jsobj.cpp:1019 #5 0x0000000000a9d1cd in js::RunState::maybeCreateThisForConstructor (this=this@entry=0x7fff728e8ad0, cx=cx@entry=0x7fc866d07800) at js/src/vm/Interpreter.cpp:356 #6 0x0000000000699657 in js::jit::CanEnter (cx=cx@entry=0x7fc866d07800, state=...) at js/src/jit/Ion.cpp:2506 #7 0x0000000000ab8869 in js::RunScript (cx=cx@entry=0x7fc866d07800, state=...) at js/src/vm/Interpreter.cpp:404 #8 0x0000000000ab8c8d in js::Invoke (cx=cx@entry=0x7fc866d07800, args=..., construct=construct@entry=js::CONSTRUCT) at js/src/vm/Interpreter.cpp:496 #9 0x0000000000ab9018 in InternalConstruct (cx=cx@entry=0x7fc866d07800, args=...) at js/src/vm/Interpreter.cpp:557 #10 0x0000000000ab9177 in js::Construct (cx=cx@entry=0x7fc866d07800, fval=..., fval@entry=..., args=..., newTarget=..., newTarget@entry=..., objp=..., objp@entry=...) at js/src/vm/Interpreter.cpp:604 #11 0x00000000006155d1 in js::jit::DoCallFallback (cx=0x7fc866d07800, frame=0x7fff728e90d8, stub_=<optimized out>, argc=<optimized out>, vp=0x7fff728e9028, res=...) at js/src/jit/BaselineIC.cpp:6117 [...] #22 0x0000000000000000 in ?? () (gdb) info reg rax 0x0 0 rbx 0x7fc866d07800 140498695124992 rcx 0x7fc8683b0760 140498718885728 rdx 0x1c2d080 29544576 rsi 0x7fc8683691a0 140498718593440 rdi 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rbp 0x7fff728e8740 0x7fff728e8740 rsp 0x7fff728e8740 0x7fff728e8740 r8 0x14 20 r9 0x14 20 r10 0xd 13 r11 0x3b 59 r12 0x2 2 r13 0x7fff728e87a0 140735115331488 r14 0x7fff728e8780 140735115331456 r15 0x7fc864993fd0 140498657951696 rip 0xb6c308 0xb6c308 <js::PreliminaryObjectArray::registerNewObject(JSObject*)+40> eflags 0x10246 [ PF ZF IF RF ] cs 0x33 51 ss 0x2b 43 ds 0x0 0 es 0x0 0 fs 0x0 0 gs 0x0 0 (gdb) x /i $pc => 0xb6c308 <js::PreliminaryObjectArray::registerNewObject(JSObject*)+40>: cmpq $0x0,(%rdi,%rax,8) Looks like use-after-free, marking s-s and sec-critical. Unfortunately the testcase is slightly intermittent. Bisection might not be accurate when done by JSBugMon. I should also note that I was not able to reproduce this bug under GDB directly *at all*. On the command line, it reproduced very reliably for me though.
Jan, any idea of who might want to look at this?
Tracking since this is sec-critical. Are 46 and 45 also affected?
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Excellent catch fuzzers! It took me a while to reproduce this but it was worth it. This is a UAF in CreateThisForFunctionWithGroup: NewObjectWithGroup can OOM and clear group->newScript(), so we shouldn't use |newScript| after that point.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8732227 - Flags: review?(bhackett1024)
Not sure exactly when this regressed. The code we're patching here is from 2014 or so, but the TI maybeClearNewScriptOnOOM stuff is probably a bit more recent. Anyway, this is sec-critical and the fix is simple, so we should just land this everywhere.
Can you request sec-approval? Thanks.
Yup, I'll request sec-approval after review.
Attachment #8732227 - Flags: review?(bhackett1024) → review+
Comment on attachment 8732227 [details] [diff] [review] Patch [Security approval request comment] > How easily could an exploit be constructed based on the patch? It's not easy, but certainly possible for a smart and determined attacker. > 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. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply or be very easy to backport. > How likely is this patch to cause regressions; how much testing does it need? Unlikely. The patch is very simple.
Attachment #8732227 - Flags: sec-approval?
sec-approval+ for trunk. We'll want this on affected branches as well. Please nominate patches for those.
Attachment #8732227 - Flags: sec-approval? → sec-approval+
See comment 11 (ni? to avoid dropping this)
Comment on attachment 8732227 [details] [diff] [review] Patch (Yup thanks, I usually wait until the patch is on m-c before uplifting.) Approval Request Comment [Feature/regressing bug #]: Not clear but an older bug. [User impact if declined]: Security bugs, crashes. [Describe test coverage new/current, TreeHerder]: Code is exercised on Treeherder. Patch fixes the reported crash. [Risks and why]: Low risk. Patch is very small and safe. [String/UUID change made/needed]: None.
Comment on attachment 8732227 [details] [diff] [review] Patch Crash fix, sec-critical, has test coverage on treeherder. Let's take this for aurora and for beta 6.
Comment on attachment 8732227 [details] [diff] [review] Patch sec critical, taking it Should be in 45.1.0 & 38.8.0
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main46+][adv-esr45.1+][adv-esr38.8+]
You need to log in before you can comment on or make changes to this bug.