Assertion failure: slotInRange(slot), at js/src/vm/NativeObject.h:779 or use-after-free [@ js::BarrieredBase<JS::Value>::pre]

RESOLVED FIXED in Firefox 42

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks 1 bug, 7 keywords)

Dependency tree / graph

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ fixed, firefox43+ fixed, firefox44+ fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

4 years ago
The following testcase crashes on mozilla-central revision ccd6b5f5e544 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --baseline-eager --ion-offthread-compile=off):

function testcase() {
  Reflect.set(testcase, "prop", 5, __proto__);
  new testcase(testcase);
}
testcase();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004e6a4d in js::NativeObject::setSlot (this=0x7ffff7e76940, slot=14, value=...) at js/src/vm/NativeObject.h:779
#0  0x00000000004e6a4d in js::NativeObject::setSlot (this=0x7ffff7e76940, slot=14, value=...) at js/src/vm/NativeObject.h:779
#1  0x00000000006e427c in setSlotWithType (overwriting=true, value=..., shape=0x7ffff7e779e8, cx=0x7ffff6907000, this=0x7ffff7e76940) at js/src/vm/NativeObject-inl.h:280
#2  NativeSetExistingDataProperty (cx=cx@entry=0x7ffff6907000, obj=obj@entry=..., shape=..., shape@entry=..., v=v@entry=..., result=..., receiver=...) at js/src/vm/NativeObject.cpp:2021
#3  0x00000000006fd04d in SetExistingProperty (result=..., shape=..., pobj=..., receiver=..., v=..., id=..., obj=..., cx=0x7ffff6907000) at js/src/vm/NativeObject.cpp:2247
#4  js::NativeSetProperty (cx=cx@entry=0x7ffff6907000, obj=..., id=..., value=..., receiver=..., qualified=qualified@entry=js::Qualified, result=...) at js/src/vm/NativeObject.cpp:2308
#5  0x00000000005580f3 in SetProperty (result=..., receiver=..., v=..., id=..., obj=..., cx=0x7ffff6907000) at js/src/vm/NativeObject.h:1436
#6  Reflect_set (cx=0x7ffff6907000, argc=4, vp=<optimized out>) at js/src/builtin/Reflect.cpp:344
#7  0x00007ffff7ff4ea8 in ?? ()
#8  0x00007ffff6a00040 in ?? ()
#9  0x00007fffffffb3f0 in ?? ()
#10 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff7e76940	140737352526144
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffb080	140737488334976
rsp	0x7fffffffafe0	140737488334816
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffada0	140737488334240
r11	0x7ffff6c27960	140737333328224
r12	0xe	14
r13	0x7fffffffb1e0	140737488335328
r14	0x7fffffffb1e0	140737488335328
r15	0x7ffff7e779e8	140737352530408
rip	0x4e6a4d <js::NativeObject::setSlot(unsigned int, JS::Value const&)+1837>
=> 0x4e6a4d <js::NativeObject::setSlot(unsigned int, JS::Value const&)+1837>:	movl   $0x30b,0x0
   0x4e6a58 <js::NativeObject::setSlot(unsigned int, JS::Value const&)+1848>:	callq  0x496780 <abort()>


Marking s-s until we confirmed that this cannot be exploited in the browser.

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

4 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7325fc8acaca
user:        Jason Orendorff
date:        Wed Nov 05 00:32:29 2014 -0600
summary:     Bug 987514, part 4 - Implement most of the standard Reflect methods. r=Waldo.

This iteration took 212.943 seconds to run.
Flags: needinfo?(jorendorff)
Assignee

Comment 2

4 years ago
SetExistingProperty() contains a fast path for the case when obj, pobj, and
receiver all refer to the same object. Ordinarily, these three objects are in
some chain of [[Prototype]] and/or [[ProxyTarget]] links, in the order
receiver --> obj --> pobj.  In that case, if receiver == pobj, then
receiver == obj == pobj. The existing code assumed this is always true, but
with Reflect.set() it is possible to arrange for receiver == pobj to be true
while obj is some other object.
Attachment #8668455 - Flags: review?(jwalden+bmo)
Assignee

Updated

4 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee

Comment 3

4 years ago
[Tracking Requested - why for this release]:

This crashing bug is definitely in beta 42 and probably in aurora 41 too (aurora contains the bad changeset; building now to check that the crash actually happens).

sec-crit because you can write off the end of an object. I'm guessing this means you can write 64 bits to fairly arbitrary locations both in the JS GC heap and in the malloc heap.
Flags: needinfo?(jorendorff)
Keywords: sec-critical
Assignee

Comment 4

4 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> This crashing bug is definitely in beta 42 and probably in aurora 41 too

Sorry, aurora 43 of course.
[Tracking Requested - why for this release]:
Assignee

Comment 6

4 years ago
Confirmed -- the bug happens just the same in aurora 43, as expected.
Comment on attachment 8668455 [details] [diff] [review]
Fix an optimization in property assignment that is broken by Reflect.set

Review of attachment 8668455 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/NativeObject.cpp
@@ +2227,5 @@
>          if (!shape->writable())
>              return result.fail(JSMSG_READ_ONLY);
>  
>          // steps 5.c-f.
> +        if (receiver.isObject() && obj == &receiver.toObject() && pobj == obj) {

So I think I agree we can do this.  But.  Lines 2218-2219 seem to also require this fix.  But.  I don't think it does, because it does SetDenseOrTypedArrayElement using *pobj*.  But that raises a question: can't we instead change line 2242 to pass in pobj, for equal safety/correctness and greater efficiency?
Attachment #8668455 - Flags: review?(jwalden+bmo)
sec-critical + crash = tracking.
Assignee

Comment 9

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> >          // steps 5.c-f.
> > +        if (receiver.isObject() && obj == &receiver.toObject() && pobj == obj) {
> 
> So I think I agree we can do this.  But.  Lines 2218-2219 seem to also
> require this fix.  But.  I don't think it does, because it does
> SetDenseOrTypedArrayElement using *pobj*.  But that raises a question: can't
> we instead change line 2242 to pass in pobj, for equal safety/correctness
> and greater efficiency?

It took a surprising amount of time to unpack this, but I think it's spot on. In several places, including this one, using obj when there's a pobj available is just a bug. Revising the patch now.
Assignee

Comment 10

4 years ago
SetExistingProperty() contains a fast path for the case when pobj and
receiver refer to the same object. Ordinarily, if that much is true,
then obj also refers to the same object, but with Reflect.set() it is
possible to arrange for receiver == pobj to be true while obj is some
other object.
Attachment #8669920 - Flags: review?(jwalden+bmo)
Assignee

Updated

4 years ago
Attachment #8668455 - Attachment is obsolete: true
Comment on attachment 8669920 [details] [diff] [review]
Fix an bug in property assignment, recently exposed by Reflect.set

Review of attachment 8669920 [details] [diff] [review]:
-----------------------------------------------------------------

"It took a surprising amount of time to unpack this"

Wouldn't surprise me in the slightest, especially given how long unpacking the original patch/bug took.

::: js/src/tests/ecma_6/Object/bug-1206700.js
@@ +3,5 @@
> +var y = {};
> +Reflect.set(y, "prop", 6, Object.prototype);
> +assertEq(x.hasOwnProperty("prop"), false);
> +assertEq(y.hasOwnProperty("prop"), false);
> +assertEq(Object.prototype.prop, 6);

Could hasOwnProperty this as well for extra diligence.
Attachment #8669920 - Flags: review?(jwalden+bmo) → review+
Jason, we are the end of the 42 beta cycle (beta 8 go to build is today). If we want to address this issue, this will have to land soon. Thanks
Flags: needinfo?(jorendorff)
Assignee

Comment 13

4 years ago
This hasn't landed because I haven't been able to get a clean Try run. The closest I've gotten is this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15dc77204145

Try has just been a disaster for two weeks now. Yesterday it wasn't working at all. But I should have been pushing harder on this.
Flags: needinfo?(jorendorff)
Assignee

Comment 14

4 years ago
Argh. My latest Try push looks good so far. But I forgot to scrub the bug number from the push, so the bad guys know can see the patch and know it's security-sensitive.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d7263884f73
Assignee

Comment 15

4 years ago
Comment on attachment 8669920 [details] [diff] [review]
Fix an bug in property assignment, recently exposed by Reflect.set

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

    Not super hard. You'd have to be an expert in SM internals and then
    it would take days. If I were the bad guys, I would pursue this one.

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?

    FF42 (currently beta for another few days) and later are affected.

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

    Bug 987514.

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

    Easy to create and low risk: it's a one-line patch.

How likely is this patch to cause regressions; how much testing does it need?

    This changes behavior in certain corner cases that should be super rare
    on the Web.

Approval Request Comment
[Feature/regressing bug #]: bug 987514
[User impact if declined]: Nothing unless we get zero-day'd, which is likely.
[Describe test coverage new/current, TreeHerder]: There's a test in the bug.
[Risks and why]: More risk than usual for a one-line patch, because it's a change to fundamentals of object behavior. But I think we have to take it.
[String/UUID change made/needed]: None.
Attachment #8669920 - Flags: sec-approval?
Attachment #8669920 - Flags: approval-mozilla-beta?
Attachment #8669920 - Flags: approval-mozilla-aurora?
Assignee

Comment 16

4 years ago
> [Describe test coverage new/current, TreeHerder]: There's a test in the bug.

It's not super clear to me what this is asking. How does a TreeHerder link help? Here's one anyway:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d7263884f73
Attachment #8669920 - Flags: sec-approval?
Attachment #8669920 - Flags: sec-approval+
Attachment #8669920 - Flags: approval-mozilla-beta?
Attachment #8669920 - Flags: approval-mozilla-beta+
Attachment #8669920 - Flags: approval-mozilla-aurora?
Attachment #8669920 - Flags: approval-mozilla-aurora+
Approving taking this late since it is a simple patch.
Keywords: checkin-needed
Al, this is indeed very late. We left the beta cycle, we are now building RC, meaning that if this patch causes any regression, we will have to spine a new RC.
Even it is a one liner, with the risk evaluation from Jason, I would have not taken it.
More coordination with release management would have been appreciated here.
Flags: needinfo?(abillings)
I understand. It *is* a one line change for a sec-critical we *haven't* shipped to customers yet. I want to never ship it.
Flags: needinfo?(abillings)
Blocks: 987514
Assignee

Comment 25

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3f60cefeb89abdf13aee861de376ac5a92528e
Bug 1206700 - Followup to add extra test assertion requested by jwalden in review.
https://hg.mozilla.org/mozilla-central/rev/bc7c5bc09120
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

4 years ago
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Group: core-security-release
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.