Closed Bug 1441006 Opened 2 years ago Closed 2 years ago

Assertion failure: clampedStart <= clampedEnd, at js/src/gc/Marking.cpp:2793


(Core :: JavaScript Engine, defect, P1, critical)




Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 + verified
firefox60 + verified


(Reporter: decoder, Assigned: jandem)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:update][adv-main59+])


(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision bfe62272d2a2 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager):

var length = 100000;
var array = new Array(length);
for (var i = 0; i < length; i++)
  array[x = 2147483647] = {};


received signal SIGSEGV, Segmentation fault.
0x0000000000e76802 in js::gc::StoreBuffer::SlotsEdge::trace (this=this@entry=0x7ffff5f07068, mover=...) at js/src/gc/Marking.cpp:2793
#0  0x0000000000e76802 in js::gc::StoreBuffer::SlotsEdge::trace (this=this@entry=0x7ffff5f07068, mover=...) at js/src/gc/Marking.cpp:2793
#1  0x0000000000eb4734 in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::SlotsEdge>::trace (this=this@entry=0x7ffff5f1d258, owner=owner@entry=0x7ffff5f1d1a8, mover=...) at js/src/gc/Marking.cpp:2760
#2  0x0000000000ec8471 in js::gc::StoreBuffer::traceSlots (mover=..., this=0x7ffff5f1d1a8) at js/src/gc/StoreBuffer.h:439
#3  js::Nursery::doCollection (this=this@entry=0x7ffff5f1cdd0, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, tenureCounts=...) at js/src/gc/Nursery.cpp:860
#4  0x0000000000ec8cce in js::Nursery::collect (this=0x7ffff5f1cdd0, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY) at js/src/gc/Nursery.cpp:722
#5  0x0000000000e5fa4a in js::gc::GCRuntime::minorGC (this=0x7ffff5f1a780, reason=JS::gcreason::OUT_OF_NURSERY, phase=<optimized out>) at js/src/gc/GC.cpp:7718
#6  0x0000000000e8ee3a in js::gc::GCRuntime::tryNewNurseryObject<(js::AllowGC)1> (this=this@entry=0x7ffff5f1a780, cx=cx@entry=0x7ffff5f16000, thingSize=thingSize@entry=32, nDynamicSlots=nDynamicSlots@entry=0, clasp=clasp@entry=0x1ff4ae0 <js::PlainObject::class_>) at js/src/gc/Allocator.cpp:93
#7  0x0000000000eb70c8 in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff5f16000, kind=kind@entry=js::gc::AllocKind::OBJECT0_BACKGROUND, nDynamicSlots=nDynamicSlots@entry=0, heap=heap@entry=js::gc::DefaultHeap, clasp=clasp@entry=0x1ff4ae0 <js::PlainObject::class_>) at js/src/gc/Allocator.cpp:55
#8  0x0000000000a21412 in js::NativeObject::create (cx=0x7ffff5f16000, kind=<optimized out>, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/NativeObject-inl.h:538
#9  0x0000000000b3fda4 in NewObject (cx=0x7ffff5f16000, group=..., kind=<optimized out>, newKind=js::GenericObject, initialShapeFlags=<optimized out>) at js/src/vm/JSObject.cpp:728
#10 0x0000000000b405c2 in js::NewObjectWithClassProtoCommon (cx=cx@entry=0x7ffff5f16000, clasp=0x1ff4ae0 <js::PlainObject::class_>, protoArg=..., protoArg@entry=..., allocKind=allocKind@entry=js::gc::AllocKind::OBJECT0_BACKGROUND, newKind=newKind@entry=js::GenericObject) at js/src/vm/JSObject.cpp:849
#11 0x000000000055ac62 in js::NewObjectWithClassProto (newKind=js::GenericObject, allocKind=js::gc::AllocKind::OBJECT0_BACKGROUND, proto=..., clasp=<optimized out>, cx=0x7ffff5f16000) at js/src/vm/JSObject-inl.h:677
#12 js::NewBuiltinClassInstance (newKind=js::GenericObject, allocKind=js::gc::AllocKind::OBJECT0_BACKGROUND, clasp=<optimized out>, cx=0x7ffff5f16000) at js/src/vm/JSObject-inl.h:714
#13 js::NewBuiltinClassInstance<js::PlainObject> (newKind=js::GenericObject, allocKind=js::gc::AllocKind::OBJECT0_BACKGROUND, cx=0x7ffff5f16000) at js/src/vm/JSObject-inl.h:736
#14 js::CopyInitializerObject (cx=0x7ffff5f16000, baseobj=..., newKind=js::GenericObject) at js/src/vm/NativeObject-inl.h:706
#15 0x000000000056defb in js::NewObjectOperationWithTemplate (cx=0x7ffff5f16000, templateObject=...) at js/src/vm/Interpreter.cpp:5014
#16 0x00001fe038e1e0a1 in ?? ()
#26 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff4c91120	140737300205856
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc300	140737488339712
rsp	0x7fffffffc2b0	140737488339632
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff5f07068	140737319563368
r13	0x7fffffffc400	140737488339968
r14	0x7ffff5f07060	140737319563360
r15	0x7ffff5f1d258	140737319653976
rip	0xe76802 <js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer&) const+562>
=> 0xe76802 <js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer&) const+562>:	movl   $0x0,0x0
   0xe7680d <js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer&) const+573>:	ud2

Marking s-s both because it is a GC and a range assertion.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
parent:      390713:c376bb034ca6
user:        Jan de Mooij
date:        Wed Nov 08 09:21:27 2017 +0100
summary:     Bug 1415119 - Support out-of-bounds indexes in PostWriteElementBarrier. r=jonco

This iteration took 312.688 seconds to run.
Flags: needinfo?(jdemooij)
Attached patch Patch (obsolete) — Splinter Review
The problem is that SlotsEdge stores start_ and count_ as (signed) int32_t.

In PostWriteElementBarrier we can add an element with start = INT32_MAX and count = 1 so we overflow in SlotsEdge::trace. I'm not sure if this is actually a security issue because dense elements can't grow that big anyway, but there's also SlotsEdge::overlaps to worry about.

This patch changes these fields to uint32_t. I think the only reason for signed integers here was so that SlotsEdge::overlaps becomes a bit simpler (it subtracts 1 from start_), but there we can just check start_ > 0.
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8953951 - Flags: review?(jcoppeard)
Comment on attachment 8953951 [details] [diff] [review]

Review of attachment 8953951 [details] [diff] [review]:

Ouch.  Thanks for fixing this.
Attachment #8953951 - Flags: review?(jcoppeard) → review+
Attached patch PatchSplinter Review
Smae patch, but also changes PostWriteElementBarrier to take the slow path if index >= NativeObject::MAX_DENSE_ELEMENTS_COUNT. I think this would also have fixed the bug, but we can do both.
Attachment #8953951 - Attachment is obsolete: true
Attachment #8953973 - Flags: review?(jcoppeard)
Not sure if this is really sec-high, but let's assume it is.
Attachment #8953973 - Flags: review?(jcoppeard) → review+
Comment on attachment 8953973 [details] [diff] [review]

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not sure. Might be possible with some work.

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

> If not all supported branches, which bug introduced the flaw?
Bug 1415119.

> 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 #8953973 - Flags: sec-approval?
Priority: -- → P1
Ritu, I'm not sure who in Release Management is on point for this release so I'm ni? you. We're late in the cycle so I wanted to make sure it was ok to take this.
Flags: needinfo?(rkothari)
Ryan has been approving 59 patches so NI him too.
Flags: needinfo?(ryanvm)
Liz and Ryan are handling 59.
Flags: needinfo?(rkothari) → needinfo?(lhenry)
I'm fine with this.
Flags: needinfo?(lhenry)
Comment on attachment 8953973 [details] [diff] [review]

Sec-approval+ for trunk. We'll want a Beta patch nominated ASAP as well so it can get into a beta.
Attachment #8953973 - Flags: sec-approval? → sec-approval+
Comment on attachment 8953973 [details] [diff] [review]

The patch that landed on inbound also applies to beta tip.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1415119.
[User impact if declined]: Potential security issues.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Pretty low risk.
[Why is the change risky/not risky?]: Lots of tests exercise this code.
[String changes made/needed]: None.
Attachment #8953973 - Flags: approval-mozilla-beta?
Comment on attachment 8953973 [details] [diff] [review]

Adding the m-r flag so we will consider this for the 59 RC.
Attachment #8953973 - Flags: approval-mozilla-release?
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8953973 [details] [diff] [review]

Fixes a sec-high and verified on trunk. Approved for Fx59rc1 and Fennec 59b15.
Flags: needinfo?(ryanvm)
Attachment #8953973 - Flags: approval-mozilla-release?
Attachment #8953973 - Flags: approval-mozilla-release+
Attachment #8953973 - Flags: approval-mozilla-beta?
Attachment #8953973 - Flags: approval-mozilla-beta+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main59+]
JSBugMon: This bug has been automatically verified fixed on Fx59
Group: javascript-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.