1155474 - Crash [@ js::Shape::search] with heap-buffer-overflow

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

{
  for (var i = 0; i < 100; i++)
    arr += arr.foo;
  let arr = 1;
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005e5cb7 in js::Shape::search (cx=cx@entry=0x7ffff698d0f0, start=0x7ffff69223f8, id=..., pentry=pentry@entry=0x7fffffffcca0, adding=adding@entry=false) at js/src/vm/Shape-inl.h:44
#0  0x00000000005e5cb7 in js::Shape::search (cx=cx@entry=0x7ffff698d0f0, start=0x7ffff69223f8, id=..., pentry=pentry@entry=0x7fffffffcca0, adding=adding@entry=false) at js/src/vm/Shape-inl.h:44
#1  0x00000000005c32b8 in lookup (id=..., cx=0x7ffff698d0f0, this=<optimized out>) at js/src/vm/NativeObject.cpp:224
#2  LookupOwnPropertyInline<(js::AllowGC)1> (donep=<synthetic pointer>, propp=..., id=..., obj=..., cx=0x7ffff698d0f0) at js/src/vm/NativeObject-inl.h:460
#3  NativeGetPropertyInline<(js::AllowGC)1> (vp=..., nameLookup=NotNameLookup, id=..., receiver=..., obj=..., cx=0x7ffff698d0f0) at js/src/vm/NativeObject.cpp:1831
#4  js::NativeGetProperty (cx=0x7ffff698d0f0, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:1875
#5  0x00000000008a6855 in GetProperty (vp=..., id=..., receiver=..., obj=..., cx=0x7ffff698d0f0, cx@entry=0x7fffffffcec0) at js/src/vm/NativeObject.h:1417
#6  MaybeCallMethod (cx=cx@entry=0x7ffff698d0f0, obj=obj@entry=..., id=..., id@entry=..., vp=vp@entry=...) at js/src/jsobj.cpp:3315
#7  0x00000000008ac7a4 in JS::OrdinaryToPrimitive (cx=0x7ffff698d0f0, obj=..., hint=JSTYPE_VOID, vp=...) at js/src/jsobj.cpp:3377
#8  0x000000000055d027 in ToPrimitive (vp=..., cx=0x7ffff698d0f0) at js/src/jsobjinlines.h:545
#9  AddOperation (res=..., rhs=..., lhs=..., cx=0x7ffff698d0f0) at js/src/vm/Interpreter.cpp:1537
#10 js::AddValues (cx=cx@entry=0x7ffff698d0f0, lhs=..., lhs@entry=..., rhs=..., rhs@entry=..., res=..., res@entry=...) at js/src/vm/Interpreter.cpp:4421
#11 0x00000000006b7954 in js::jit::DoBinaryArithFallback (cx=0x7ffff698d0f0, frame=0x7fffffffd118, stub_=0x7ffff69a0720, lhs=..., rhs=..., ret=...) at js/src/jit/BaselineIC.cpp:2549
#12 0x00007ffff7feebc4 in ?? ()
#13 0x00007ffff7fe8640 in ?? ()
#14 0x00007fffffffd0b8 in ?? ()
#15 0x00007ffff7fe8640 in ?? ()
#16 0xfff9000000000000 in ?? ()
#17 0x000000000171aba0 in js::jit::DoConcatStringsInfo ()
#18 0x00007ffff4d51d60 in ?? ()
[...]
#28 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69223f8	140737330160632
rcx	0x7fffffffcca0	140737488342176
rdx	0x0	0
rsi	0x7ffff4d1cc10	140737300778000
rdi	0x7ffff698d0f0	140737330598128
rbp	0x7ffff4d1cc10	140737300778000
rsp	0x7fffffffcbe0	140737488341984
r8	0x0	0
r9	0x7ffff4d5e128	140737301045544
r10	0x7fffffffd0e0	140737488343264
r11	0xfff9000000000000	-1970324836974592
r12	0x7ffff698d108	140737330598152
r13	0x7fffffffcc60	140737488342112
r14	0x7fffffffcd90	140737488342416
r15	0x7ffff698d0f0	140737330598128
rip	0x5e5cb7 <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+167>
=> 0x5e5cb7 <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+167>:	mov    0x20(%rax),%rdi
   0x5e5cbb <js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool)+171>:	callq  0x5bfed0 <js::ShapeTable::search(jsid, bool)>


ASan trace:

=================================================================
==12534==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60800003a68d at pc 0xa94398 bp 0x7fff97efdb70 sp 0x7fff97efdb68
READ of size 1 at 0x60800003a68d thread T0
    #0 0xa94397 in js::Shape::inDictionary() const js/src/vm/Shape.h:786
    #1 0xa94397 in js::Shape::search(js::ExclusiveContext*, js::Shape*, jsid, js::ShapeTable::Entry**, bool) js/src/vm/Shape-inl.h:43
    #2 0x9fe521 in js::NativeObject::lookup(js::ExclusiveContext*, jsid) js/src/vm/NativeObject.cpp:224:12
    #3 0x9fe521 in bool js::LookupOwnPropertyInline<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType,
 js::MaybeRooted<js::Shape*, (js::AllowGC)1>::MutableHandleType, bool*) js/src/vm/NativeObject-inl.h:460
    #4 0x9fe521 in _ZL23NativeGetPropertyInlineILN2js7AllowGCE1EEbP9JSContextNS0_11MaybeRootedIPNS0_12NativeObjectEXT_EE10HandleTypeENS4_IP8JSObjectXT_EE10HandleTypeENS4_I4jsidXT_EE10HandleTypeE12IsNameLoo
kupNS4_IN2JS5ValueEXT_EE17MutableHandleTypeE js/src/vm/NativeObject.cpp:1831
    #5 0x9fe521 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 
js/src/vm/NativeObject.cpp:1875
    #6 0x151a2f0 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/vm/Nat
iveObject.h:1417
    #7 0x151a2f0 in MaybeCallMethod(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3315
    #8 0x1518424 in JS::OrdinaryToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3377
    #9 0x15179a1 in js::ToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) js/src/jsobj.cpp:3196
    #10 0x946317 in js::ToPrimitive(JSContext*, JS::MutableHandle<JS::Value>) js/src/jsobjinlines.h:545
    #11 0x946317 in AddOperation(JSContext*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:1537
    #12 0x946317 in js::AddValues(JSContext*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:4421
    #13 0xd607a1 in js::jit::DoBinaryArithFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICBinaryArith_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:2549

0x60800003a68d is located 19 bytes to the left of 96-byte region [0x60800003a6a0,0x60800003a700)
allocated by thread T0 here:
    #0 0x4abd47 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7acce4 in js_malloc(unsigned long) js/src/opt64asan/js/src/../../dist/include/js/Utility.h:119
    #2 0x7acce4 in _ZL13js_pod_mallocIhEPT_m js/src/opt64asan/js/src/../../dist/include/js/Utility.h:274
    #3 0x7acce4 in unsigned char* js::MallocProvider<js::ExclusiveContext>::pod_malloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:62
    #4 0x7acce4 in js::ScriptSource* js::MallocProvider<js::ExclusiveContext>::new_<js::ScriptSource>() js/src/vm/MallocProvider.h:161
    #5 0x7acce4 in js::frontend::CreateScriptSourceObject(js::ExclusiveContext*, JS::ReadOnlyCompileOptions const&) js/src/frontend/BytecodeCompiler.cpp:181
    #6 0x7ad99e in js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Handle<js::StaticEvalObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, unsigned int, js::SourceCompressionTask*) js/src/frontend/BytecodeCompiler.cpp:246
    #7 0x1402e3b in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3792
    #8 0x14033f2 in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3802
    #9 0x14033f2 in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, char const*, unsigned long, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3817 
    #10 0x140364c in JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, _IO_FILE*, JS::MutableHandle<JSScript*>) js/src/jsapi.cpp:3828

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/vm/Shape.h:786 js::Shape::inDictionary() const


Marking s-s and sec-critical because ASan shows this crash as a heap-buffer-overflow:

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "bad" changeset has the timestamp "20150416202025" and the hash "1a51b749299f".
The "good" changeset has the timestamp "20150416203330" and the hash "66eee8b402fd".

Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1a51b749299f&tochange=66eee8b402fd

Comment 2

[Christian Holler (:decoder)]

Needinfo from shu based on comment 1.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Christian Holler (:decoder) from comment #2)
> Needinfo from shu based on comment 1.

Am I reading autobisect right in that it thought it was fixed by one of those patches?

Comment 4

[Shu-yu Guo [:shu]]

I have no idea from the stacks if either of my patches were proper fixes for this bug.

Comment 5

[Christian Holler (:decoder)]

That's indeed a good question, a fix bisection was not requested. gkw, can you maybe look manually why autobisect produced the wrong result here? Thanks!

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Christian Holler (:decoder) from comment #5)
> That's indeed a good question, a fix bisection was not requested. gkw, can
> you maybe look manually why autobisect produced the wrong result here?
> Thanks!

I ran autoBisect on downloaded binaries here on my computer and it shows latest binaries to still be showing the problem. Not sure why that happened for JSBugMon.

Next, I ran it on compiled binaries and it shows:

Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/03242a11d044
user:        Shu-yu Guo
date:        Mon Sep 15 16:30:45 2014 -0700
summary:     Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem)

changeset:   https://hg.mozilla.org/mozilla-central/rev/8114e77c253e
user:        Shu-yu Guo
date:        Mon Sep 15 16:30:46 2014 -0700
summary:     Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb)

changeset:   https://hg.mozilla.org/mozilla-central/rev/31714af41f2c
user:        Shu-yu Guo
date:        Mon Sep 15 16:30:46 2014 -0700
summary:     Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem)

Shu-yu, is bug 1001090 a likely regressor?

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Consider the input to MThrowUninitializedLexical implicitly used.

What's going on here is Ion compiled a lexical check as
MThrowUninitializedLexical, since it saw that the input is for sure not
initialized. MThrowUninitializedLexical has no inputs, so then Ion optimized
out the JS_UNINITIALIZED_LEXICAL magic in favor of a JS_OPTIMIZED_OUT magic.
When we bailed out, the BaselineFrame now has a wrong magic value for an
uninitialized lexical.

Comment 8

[Shu-yu Guo [:shu]]

I think this will need backport to everything.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8596145 [details] [diff] [review]
Consider the input to MThrowUninitializedLexical implicitly used.

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

Stealing upon request.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81ed40faade8

This landed without sec-approval (and a test)? Please request sec-approval and Aurora/Release uplift approval ASAP.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/81ed40faade8
> 
> This landed without sec-approval (and a test)? Please request sec-approval
> and Aurora/Release uplift approval ASAP.

Argh sorry, I don't think I've ever remembered to request sec-approval in my life.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8596145 [details] [diff] [review]
Consider the input to MThrowUninitializedLexical implicitly used.

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

Fairly difficult as it requires a situation where the script executes in Ion before executing in Baseline/Interpreter. This generally isn't possible without special prefs/flags.

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

The test case itself is an exploit, only with --ion-eager. In normal content code it is not an exploit.

Which older supported branches are affected by this flaw?

All the way back to release.

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?

Same patch can be backported.

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

Unlikely to cause regressions.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8596145 [details] [diff] [review]
Consider the input to MThrowUninitializedLexical implicitly used.

Approval Request Comment
[Feature/regressing bug #]: 1001090
[User impact if declined]: Rare crashes when using lexicals in versioned JS (unavailable to unversioned content JS)
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I.
[String/UUID change made/needed]: None

Comment 14

[Sylvestre Ledru [:Sylvestre]]

> [Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I.
The risk is the potential impact on the Firefox product itself. Not the security risk.
Could you evaluate it? Thanks

Comment 15

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

Comment on attachment 8596145 [details] [diff] [review]
Consider the input to MThrowUninitializedLexical implicitly used.

sec-approval+ for trunk.

Comment 16

[Shu-yu Guo [:shu]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> > [Risks and why]: Medium. An exploit could potentially be constructed from the patch. I don't see how right now, but there are cleverer people than I.
> The risk is the potential impact on the Firefox product itself. Not the
> security risk.
> Could you evaluate it? Thanks

Sorry, what does impact on the product itself mean? Like could it exhibit buggy behavior? Web-breaking changes? That kind of thing?

Comment 17

[Sylvestre Ledru [:Sylvestre]]

(In reply to Shu-yu Guo [:shu] from comment #16)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #14)

> Sorry, what does impact on the product itself mean? Like could it exhibit
> buggy behavior? Web-breaking changes? That kind of thing?
Yes.
For example: "high risk, this could break every javascript-based application"

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8596145 [details] [diff] [review]
Consider the input to MThrowUninitializedLexical implicitly used.

Taking it as this is low impact and it should not land in the RC anyway.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bc69f217d41
https://hg.mozilla.org/releases/mozilla-release/rev/daaa2c27b89f
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1063eb65195a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1063eb65195a

Correction:
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d4135355e5bd

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff65ba4cd38a

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx38
JSBugMon: This bug has been automatically verified fixed on Fx39
JSBugMon: This bug has been automatically verified fixed on Fx40