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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Fuzzing Team]

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.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Fix an optimization in property assignment that is broken by Reflect.set

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.

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

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

Comment 5

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]:

Comment 6

[Jason Orendorff [:jorendorff]]

Confirmed -- the bug happens just the same in aurora 43, as expected.

Comment 7

[Jeff Walden [:Waldo]]

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?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

sec-critical + crash = tracking.

Comment 9

[Jason Orendorff [:jorendorff]]

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

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Fix an bug in property assignment, recently exposed by Reflect.set

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.

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

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

Comment 13

[Jason Orendorff [:jorendorff]]

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.

Comment 14

[Jason Orendorff [:jorendorff]]

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

Comment 15

[Jason Orendorff [:jorendorff]]

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.

Comment 16

[Jason Orendorff [:jorendorff]]

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

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Approving taking this late since it is a simple patch.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7c5bc09120

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88379b819e82

Comment 20

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/556adfdf68c3

Comment 22

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 23

[Jason Orendorff [:jorendorff]]

Tomcat also pushed this to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/88379b819e82
https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=88379b819e82

Comment 24

[Jason Orendorff [:jorendorff]]

And m-c:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7c5bc09120
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bc7c5bc09120

Comment 25

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3f60cefeb89abdf13aee861de376ac5a92528e
Bug 1206700 - Followup to add extra test assertion requested by jwalden in review.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc7c5bc09120

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb3f60cefeb8

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0aebad4cd674
https://hg.mozilla.org/releases/mozilla-beta/rev/cda83cc1aae0