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

VERIFIED FIXED in Firefox 33



4 years ago
3 years ago


(Reporter: decoder, Assigned: terrence)


(Blocks: 1 bug, 4 keywords)

crash, csectype-uaf, sec-high, testcase
Bug Flags:
in-testsuite ?
qe-verify +

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ verified, firefox34+ verified, firefox35+ verified, firefox-esr3133+ verified, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)


(Whiteboard: [adv-main33+][adv-esr31.2+], crash signature)


(2 attachments)



4 years ago
Created attachment 8485774 [details]
Testcase for shell

The attached testcase crashes on mozilla-central revision 2255d7d187b2 (run with --no-threads --fuzzing-safe).

Comment 1

4 years ago
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).


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.
status-firefox35: --- → affected
Keywords: sec-high
Keywords: csectype-uaf
Can you look at this, Terrence?  It looks like Jon is on PTO.
Flags: needinfo?(terrence)

Comment 3

4 years ago
Assignee: nobody → terrence
Flags: needinfo?(terrence)


4 years ago
Blocks: 969478


4 years ago
No longer blocks: 969478

Comment 4

4 years ago
Created attachment 8487552 [details] [diff] [review]

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 5

4 years ago
Comment on attachment 8487552 [details] [diff] [review]

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

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

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

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?
status-firefox32: --- → wontfix
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox-esr31: --- → affected
tracking-firefox33: --- → +
tracking-firefox34: --- → +
tracking-firefox35: --- → +
tracking-firefox-esr31: --- → 33+

Comment 7

4 years ago
(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]

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
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [checkin on 9/23]

Comment 11

4 years ago
Comment on attachment 8487552 [details] [diff] [review]

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]

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+
Last Resolved: 4 years ago
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox35: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox33: affected → fixed
status-firefox34: affected → fixed
status-firefox-esr31: affected → fixed
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
status-firefox33: fixed → verified
status-firefox34: fixed → verified
status-firefox35: fixed → verified
status-firefox-esr31: fixed → verified
Flags: qe-verify+
Whiteboard: [adv-main33+][adv-esr31.2+]


3 years ago
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.