Closed Bug 1064346 Opened 5 years ago Closed 5 years ago

Crash [@ IsInsideNursery] with use-after-free (likely OOM)

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 + verified
firefox34 + verified
firefox35 + verified
firefox-esr31 33+ verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: decoder, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main33+][adv-esr31.2+])

Crash Data

Attachments

(2 files)

Attached file Testcase for shell
The attached testcase crashes on mozilla-central revision 2255d7d187b2 (run with --no-threads --fuzzing-safe).
This looks like a memory-sensitive issue (likely OOM) and it only reproduced for me in an optimized build (--disable-debug --enable-optimize --enable-valgrind --enable-gczeal).

Backtrace:


Program received signal SIGSEGV, Segmentation fault.
IsInsideNursery (cell=0x4b4b4b4b4b4b4b4b) at ../../dist/include/js/HeapAPI.h:264
264         return location & ChunkLocationAnyNursery;
#0  IsInsideNursery (cell=0x4b4b4b4b4b4b4b4b) at ../../dist/include/js/HeapAPI.h:264
#1  MarkInternal<JSAtom> (trc=0x1650518, thingp=0x7ffff535f0b8) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:285
#2  0x000000000078016d in trace (trc=0x1650518, this=0x7ffff535f080) at /srv/repos/mozilla-central/js/src/jsfun.cpp:720
#3  fun_trace (trc=0x1650518, obj=0x7ffff535f080) at /srv/repos/mozilla-central/js/src/jsfun.cpp:757
#4  0x00000000004fe8e5 in js::GCMarker::processMarkStackTop (this=0x1650518, budget=...) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1748
#5  0x00000000004d6cad in js::GCMarker::drainMarkStack (this=0x1650518, budget=...) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1810
#6  0x000000000079c272 in drainMarkStack (phase=js::gcstats::PHASE_MARK, sliceBudget=..., this=0x164fc18) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4817
#7  js::gc::GCRuntime::incrementalCollectSlice (this=0x164fc18, budget=<optimized out>, reason=<optimized out>) at /srv/repos/mozilla-central/js/src/jsgc.cpp:5351
rax     0x4b4fffe8      5425512962856058856
rbx     0x1650518       23397656
rcx     0x20000 131072
rdx     0xa95db6        11099574
rsi     0xf535f0b8      140737307340984
rdi     0x4b4b4b4b      5425512962855750475
rbp     0x1650518       23397656
rsp     0xffffca00      140737488341504
r8      0xc011  49169
r9      0x21080 135296
r10     0x100   256
r11     0x0     8796093022208
r12     0xf5342158      140737307222360
r13     0xf5360088      140737307345032
r14     0x2     2
r15     0xffffcb40      140737488341824
rip     0x4c1d3c <MarkInternal<JSAtom>(JSTracer*, JSAtom**)+76>
=> 0x4c1d3c <MarkInternal<JSAtom>(JSTracer*, JSAtom**)+76>:     mov    (%rax),%eax


Looks dangerous to me, marking s-s. Ccing Jonco and sfink because it looks related to GGC.
Keywords: sec-high
Can you look at this, Terrence?  It looks like Jon is on PTO.
Flags: needinfo?(terrence)
Taking.
Assignee: nobody → terrence
Flags: needinfo?(terrence)
Blocks: 969478
No longer blocks: 969478
This is yet another path where JSFunction's quirky C-struct overlay on object slots is biting us.

In this case, we're failing a JSFunction allocation at setSingletonType after Object::create and before NewFunction. This leaves a JSObject with JSFunction::class's trace hook in the heap without having initialized it's memory in a way that the trace hook can understand it. In this case, shape->slotSpan() is hiding the slots and leaving them uninitialized; it really doesn't matter though. Whether the values we leave are uninitialized memory, poisoning, or even correct-for-an-object UndefinedValues, when JSFunction::trace looks at atom_, it's going to try to mark gibberish and crash.

We've already hacked up a bunch of paths at various places in the object and gc guts to manually check for JSFunction, so I think the right solution here is just to extend that and make JSObject::create aware that it needs to give JSFunctions C-style POD memory initialization.
Attachment #8487552 - Flags: review?(wmccloskey)
Attachment #8487552 - Flags: review?(wmccloskey) → review+
Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Pretty difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?
Nak.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to backport.

How likely is this patch to cause regressions; how much testing does it need?
Attachment #8487552 - Flags: sec-approval?
How likely is this to cause regressions?
(In reply to Al Billings [:abillings] from comment #6)
> How likely is this to cause regressions?

Very, very unlikely.
Whiteboard: [checkin on 9/23]
Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

sec-approval+ for checkin on 9/23 or later. We should have Aurora, Beta, and ESR31 patches for then too.
Attachment #8487552 - Flags: sec-approval? → sec-approval+
Terrence, can we have the uplift request? Thanks
Flags: needinfo?(terrence)
Keywords: checkin-needed
Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

Approval Request Comment
[Feature/regressing bug #]: None - it's an ancient problem.
[User impact if declined]: A potentially exploitable crash.
[Describe test coverage new/current, TBPL]: The fuzzer testcase is too large (and potentially sensitive?) to be useful as a test.
[Risks and why]: Low. This simply adds initialization for JSFunction.
[String/UUID change made/needed]: None.
Attachment #8487552 - Flags: approval-mozilla-release?
Attachment #8487552 - Flags: approval-mozilla-esr31?
Attachment #8487552 - Flags: approval-mozilla-beta?
Attachment #8487552 - Flags: approval-mozilla-aurora?
Flags: needinfo?(terrence)
Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

Giving approvals (except for release branch, which I cleared).
Attachment #8487552 - Flags: approval-mozilla-release?
Attachment #8487552 - Flags: approval-mozilla-esr31?
Attachment #8487552 - Flags: approval-mozilla-esr31+
Attachment #8487552 - Flags: approval-mozilla-beta?
Attachment #8487552 - Flags: approval-mozilla-beta+
Attachment #8487552 - Flags: approval-mozilla-aurora?
Attachment #8487552 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/88d55c033782
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reproduced the original issue using the information from comment #0 and comment #1 using revision 2255d7d187b2

fx 35 [revision: b85c260821ab] - Passed
fx 34 [revision: ee3cd82e3acb] - Passed
fx 33 [revision: d79568d581e6] - Passed
esr31 [revision: 8c431dcec0ff] - Passed
Whiteboard: [adv-main33+][adv-esr31.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.