Closed Bug 1254622 Opened 5 years ago Closed 5 years ago

Crash [@ js::PreliminaryObjectArray::registerNewObject] with use-after-free


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 46+ fixed
firefox-esr45 46+ fixed


(Reporter: decoder, Assigned: jandem)


(5 keywords, Whiteboard: [jsbugmon:][adv-main46+][adv-esr45.1+][adv-esr38.8+])

Crash Data


(2 files)

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


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.
Attached file Testcase
Jan, any idea of who might want to look at this?
Flags: needinfo?(jdemooij)
Tracking since this is sec-critical. Are 46 and 45 also affected?
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Error: Failed to isolate test from comment
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, failed due to error: Error: Failed to isolate test from comment
Attached patch PatchSplinter Review
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
Flags: needinfo?(jdemooij)
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.
Flags: needinfo?(jdemooij)
Yup, I'll request sec-approval after review.
Attachment #8732227 - Flags: review?(bhackett1024) → review+
Comment on attachment 8732227 [details] [diff] [review]

[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?

> Which older supported branches are affected by this flaw?

> 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.
Flags: needinfo?(jdemooij)
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+
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
See comment 11 (ni? to avoid dropping this)
Flags: needinfo?(jdemooij)
Comment on attachment 8732227 [details] [diff] [review]

(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.
Flags: needinfo?(jdemooij)
Attachment #8732227 - Flags: approval-mozilla-esr45?
Attachment #8732227 - Flags: approval-mozilla-esr38?
Attachment #8732227 - Flags: approval-mozilla-beta?
Attachment #8732227 - Flags: approval-mozilla-aurora?
Group: javascript-core-security → core-security-release
Comment on attachment 8732227 [details] [diff] [review]

Crash fix, sec-critical, has test coverage on treeherder.
Let's take this for aurora and for beta 6.
Attachment #8732227 - Flags: approval-mozilla-beta?
Attachment #8732227 - Flags: approval-mozilla-beta+
Attachment #8732227 - Flags: approval-mozilla-aurora?
Attachment #8732227 - Flags: approval-mozilla-aurora+
Comment on attachment 8732227 [details] [diff] [review]

sec critical, taking it
Should be in 45.1.0 & 38.8.0
Attachment #8732227 - Flags: approval-mozilla-esr45?
Attachment #8732227 - Flags: approval-mozilla-esr45+
Attachment #8732227 - Flags: approval-mozilla-esr38?
Attachment #8732227 - Flags: approval-mozilla-esr38+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main46+][adv-esr45.1+][adv-esr38.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.