621137 - StrictMode+JIT Crash/Assert: (uint32) index < arr->length

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following scripts were tested on mozilla-central trunk debug shell:

The attached testcase "min-assertIndexArrLengthCrash.js" will assert

Assertion failure: (uint32) index < arr->length, at jsscriptinlines.h:64

and furthermore crash with an invalid read (beyond array) and segmentation fault with an optimized build:

==15313== Invalid read of size 8
==15313==    at 0x6026E0: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x476CF6: js::Execute(JSContext*, JSObject*, JSScript*, JSStackFrame*, unsigned int, js::Value*) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x4118F5: JS_ExecuteScript (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x407850: Process(JSContext*, JSObject*, char*, int) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x4085FF: Shell(JSContext*, int, char**, char**) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x408904: main (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==  Address 0x805d6e030 is not stack'd, malloc'd or (recently) free'd
==15313== 
==15313== 
==15313== Process terminating with default action of signal 11 (SIGSEGV)
==15313==  Access not within mapped region at address 0x805D6E030
==15313==    at 0x6026E0: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x476CF6: js::Execute(JSContext*, JSObject*, JSScript*, JSStackFrame*, unsigned int, js::Value*) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x4118F5: JS_ExecuteScript (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x407850: Process(JSContext*, JSObject*, char*, int) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x4085FF: Shell(JSContext*, int, char**, char**) (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==    by 0x408904: main (in /scratch/holler/LangFuzz/sources/mozilla-central-hg/js.opt/src/shell/js)
==15313==  If you believe this happened as a result of a stack
==15313==  overflow in your program's main thread (unlikely but
==15313==  possible), you can try to increase the size of the
==15313==  main thread stack using the --main-stacksize= flag.
==15313==  The main thread stack size used in this run was 8388608.


The attached, slightly modified testcase "min-assertIndexArrLengthCrash.js" will assert instead with:

Assertion failure: !cx->fp()->hasImacropc() && (pc == cx->regs->pc || pc == cx->regs->pc + 1), at jstracer.cpp:5400

but I assume those to be related.

I also quickly verified both test cases on 32-bit, although the crash in optimized build with the first test case looks different here.


Locked as security bug due to possible exploitability (index length assertion and invalid read beyond array).

Comment 1

[Christian Holler (:decoder)]

Attachment: First testcase (crash/assert)

Comment 2

[Christian Holler (:decoder)]

Attachment: Second test case (related assertion?)

Comment 3

[Christian Holler (:decoder)]

Not sure if this is correct, but my bisect shows:

Changeset 42717:c96ba53e745f: bad
The first bad revision is:
changeset:   42717:c96ba53e745f
user:        Luke Wagner <[e-mail]>
date:        Wed Mar 03 18:10:13 2010 -0800
summary:     Bug 547851 - remove JSStackFrame::regs, JSStackFrame::callerFrame.sp (r=dvander)

Comment 4

[Nicholas Nethercote [inactive]]

This should definitely block!

Here's a smaller version of the first test:

options('strict');
options('tracejit');
function test() {
  var constants = ['E'];
  for (i = 0; i < 9 ; ++i) {
    ++Math[constants[0]];
  }
  eval({});
}
test();


Here's the stack trace:

#0  0xf7fdf430 in __kernel_vsyscall ()
#1  0xf7fae610 in raise (sig=6)
    at ../nptl/sysdeps/unix/sysv/linux/pt-raise.c:42
#2  0x081c2011 in JS_Assert (s=0x8321a98 "index < arr->length", 
    file=0x8321a0f "../../jsscript.h", ln=363) at ../jsutil.cpp:83
#3  0x0805852d in JSScript::getObject (this=0x8444950, index=4294867192)
    at ../../jsscript.h:363
#4  0x0830398c in js::Interpret (cx=0x840ecc0, entryFrame=0xf76c4030, 
    inlineCallCount=1, interpMode=JSINTERP_NORMAL) at ../jsinterp.cpp:5935
#5  0x081029f4 in js::RunScript (cx=0x840ecc0, script=0x84443b0, 
    fp=0xf76c4030) at ../jsinterp.cpp:657
#6  0x08103b3c in js::Execute (cx=0x840ecc0, chain=0xf7502028, 
    script=0x84443b0, prev=0x0, flags=0, result=0x0) at ../jsinterp.cpp:1001
#7  0x08074714 in JS_ExecuteScript (cx=0x840ecc0, obj=0xf7502028, 
    script=0x84443b0, rval=0x0) at ../jsapi.cpp:4936
#8  0x0804c59c in Process (cx=0x840ecc0, obj=0xf7502028, 
    filename=0xffffd2c6 "a.js", forceTTY=0) at ../../shell/js.cpp:453
#9  0x0804d565 in ProcessArgs (cx=0x840ecc0, obj=0xf7502028, argv=0xffffd0c8, 
    argc=1) at ../../shell/js.cpp:951
#10 0x08056f58 in Shell (cx=0x840ecc0, argc=1, argv=0xffffd0c8, 
    envp=0xffffd0d0) at ../../shell/js.cpp:5471
#11 0x08057133 in main (argc=1, argv=0xffffd0c8, envp=0xffffd0d0)
    at ../../shell/js.cpp:5579


The problem is that the |index| value passed to JSScript::getObject() has the value 4294867200 (in the run I'm debuggin right now, at least).  |index| is a size_t, and that value looks a lot like a smallish negative integer that was interpreted as unsigned.

jsinterp.cpp:5935 looks like this:

    LOAD_OBJECT(0, baseobj);

which expands to this:

    (baseobj = script->getObject(GET_FULL_INDEX(0)))

The |GET_FULL_INDEX(0)| is |index|, it expands to this:

    (atoms - script->atomMap.vector + GET_INDEX(regs.pc))

And in this case GET_INDEX(regs.pc) is zero.  The values of |atoms| and |script->atomMap.vector| are 0x83e2d90 and 0x8444990 respectively, so |index| ends up with the value -100096, which is 4294867200 when treated as unsigned.

So clearly either |atoms| should be larger than |script->atomMap.vector|, so one or both of them must have gone wrong somehow.

This code is in BEGIN_CASE(JSOP_NEWOBJECT), which was added by bug 606477.  CC'ing bhackett.

Comment 5

[Nicholas Nethercote [inactive]]

Here's a smaller version of the second test:

options('strict');
options('tracejit');
var constants = ['E'];
for (i = 0; i < 9; ++i) {
  ++Math[constants[0]];
}

The assertion fails because |cx->fp()->hasImacropc()| is true -- we're seemingly in the middle of an imacro at trace end.  I don't know if this has the same root cause as the first assertion failure;  if we find it doesn't have the same root cause, a new bug should be filed.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: fix

The problem here is that the PC gets corrupted, so that it points to the main script but the fp is marked as being in an imacro (and the atoms vector is messed up).  The NEWOBJECT code added by bug 606477 here is an innocent bystander, it just happens to be the first thing to try to use the atoms vector in the corrupt state.

The cause is that js_DecompileValueGenerator, which is called during an imacro when strict mode reports a warning on modifying a read-only element, saves/restores the imacro PC but does so incorrectly.

Comment 7

[Brian Hackett [Laid off!]]

Forgot: all four test cases work with this patch.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 499615 [details] [diff] [review]
fix

r=me if you add a jit-test containing the two short test cases.  Thanks for the quick fix!

Comment 9

[Brian Hackett [Laid off!]]

General question: how should things work when checking in fixes + tests for security bugs?  This code dates back to at least August, and while I highly doubt any 1.9.x versions are affected, all Fx4 beta versions will be.  The nature of this bug (no GC involved) would probably make it not too hard to craft an exploit based on a supplied testcase.

Comment 10

[Nicholas Nethercote [inactive]]

My impression is that we don't worry about such bugs if they're only present in beta releases, but I could be wrong.

Comment 11

[Christian Holler (:decoder)]

(In reply to comment #10)
> My impression is that we don't worry about such bugs if they're only present in
> beta releases, but I could be wrong.

Well, you pay for them, so it's not that you don't care ;) but Brian is right, reproducing this from tests will be easy. I guess the security team should give some input here as well.

Comment 12

[Jeff Walden [:Waldo]]

If it's only affected beta versions, either you don't care much because of this, or you push the fix without the test, then follow up a few weeks later (after people have upgraded, one hopes) with the test.  I tend to do the latter, but I don't think it's strictly necessary.  It's more a courtesy to beta and nightly users than anything else.

As for the commit message, make it something vaguely accurate but non-specific, hoping people skimming commit messages will not notice it (but if you're super-concerned about that, see below), or even omit it entirely if you can't.  Here, maybe "Bug 621137 - Slight tweak to how we decompile values.  r=sparky" would work.  If you're concerned, and the bug affects branches, maybe even file a "cover" bug, unhidden, post the patch there with nondescript comments and get review there, then push it referring to that bug.  Worst case, where it's just absolutely, completely blatantly obvious, either sneak it in another patch (I don't remember seeing this happen, but I imagine it's a tool we would use if necessary) or push it as close to point releases happening as possible.

Comment 13

[Brian Hackett [Laid off!]]

Pushed without testcases, due to the bug's easily exploitable nature.  This definitely needs the testcases pushed after the fix goes into a beta.

http://hg.mozilla.org/tracemonkey/rev/49eb06696448

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/41957753e5df
http://hg.mozilla.org/mozilla-central/rev/49eb06696448

Comment 16

[Daniel Veditz [:dveditz]]

If the regression range is accurate this should not affect the 1.9.x branches.

Comment 17

[Christian Holler (:decoder)]

Fixed for a long time and not affecting old branches, opening this.

Comment 18

[Christian Holler (:decoder)]

Old tracer bug, marking verified.

Comment 19

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719