730415 - JS OOM Testing: Invalid free in JSObject::makeDenseArraySlow

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following command crashes on mozilla-central revision 13b571bde26a:

js -A 380 -f js/src/jit-test/shell.js -f js/src/jit-test/tests/jaeger/bug709067.js


I'm getting a glibc abort here:

*** glibc detected *** debug64/js: free(): invalid pointer: 0x0000000000b361a0 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x78a8f)[0x7f3434a03a8f]
/lib/x86_64-linux-gnu/libc.so.6(cfree+0x73)[0x7f3434a078e3]

In Valgrind:

==6766== Invalid free() / delete / delete[]
==6766==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==6766==    by 0x403933: js_free (Utility.h:152)
==6766==    by 0x4118DD: js::Foreground::free_(void*) (Utility.h:566)
==6766==    by 0x41B7C5: JSRuntime::free_(void*) (jscntxt.h:640)
==6766==    by 0x41B84B: JSContext::free_(void*) (jscntxt.h:1126)
==6766==    by 0x451C07: JSObject::makeDenseArraySlow(JSContext*) (jsarray.cpp:1392)
==6766==    by 0x450CD6: array_setGeneric(JSContext*, JSObject*, jsid, JS::Value*, int) (jsarray.cpp:914)
==6766==    by 0x533C7D: JSObject::nonNativeSetProperty(JSContext*, jsid, JS::Value*, int) (jsobj.cpp:3124)
==6766==    by 0x44649F: JSObject::setGeneric(JSContext*, jsid, JS::Value*, int) (jsobjinlines.h:162)
==6766==    by 0x4F9C67: js::SetObjectElementOperation(JSContext*, JSObject*, jsid, JS::Value const&) (jsinterpinlines.h:829)
==6766==    by 0x506942: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2640)
==6766==    by 0x4FAFDE: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:454)
==6766==  Address 0xb361a0 is 0 bytes inside data symbol "_ZL17emptyObjectHeader"



At first glance it looks like some pointer arithmetic magic going wrong in jsobj.h (NULL pointer, likely from OOM is added to elements size to form a new pointer that is freed).

If the size is controllable (and I assume it will be somehow), then this is exploitable if the attacker can force OOM for the script in the right moment.

Comment 1

[Christian Holler (:decoder)]

Forgot that the shell.js is a file I created. Just omit the shell.js file and use -A 355, will also reproduce the bug.

Comment 2

[Christian Holler (:decoder)]

I wrote a patch for the JS shell that will output the trace when -A steps in (as in, when js_malloc and friends return NULL due to -A, then a backtrace will be emitted immediately). I hope this helps to track down the faulty code location that does not properly handle the OOM.

Here's the first trace for this test:

Forcing artificial memory allocation function failure:
#0 debug64-bt/js(+0x429f50) (js_malloc at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt/shell/../dist/include/js/Utility.h:162)
#1 debug64-bt/js(+0x42bc8d) (JSRuntime::malloc_(unsigned long, JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt/shell/../../jscntxt.h:606)
#2 debug64-bt/js(+0x42bd88) (JSContext::malloc_(unsigned long) at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt/shell/../../jscntxt.h:1099)
#3 debug64-bt/js(+0x474cb4) (js::ObjectElements* JSContext::new_<js::ObjectElements, int, int>(int, int) at /home/decoder/LangFuzz/mozilla-central/js/src/jscntxt.h:1129)
#4 debug64-bt/js(+0x462611) (JSObject::allocateSlowArrayElements(JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsarray.cpp:1311)
#5 debug64-bt/js(+0x4626af) (AddLengthProperty at /home/decoder/LangFuzz/mozilla-central/js/src/jsarray.cpp:1332)
#6 debug64-bt/js(+0x46290d) (JSObject::makeDenseArraySlow(JSContext*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsarray.cpp:1390)
#7 debug64-bt/js(+0x461a1d) (array_setGeneric at /home/decoder/LangFuzz/mozilla-central/js/src/jsarray.cpp:914)
#8 debug64-bt/js(+0x547af0) (JSObject::nonNativeSetProperty(JSContext*, jsid, JS::Value*, int) at /home/decoder/LangFuzz/mozilla-central/js/src/jsobj.cpp:3125)
#9 debug64-bt/js(+0x456ed4) (JSObject::setGeneric(JSContext*, jsid, JS::Value*, int) at /home/decoder/LangFuzz/mozilla-central/js/src/debug64-bt/../jsobjinlines.h:162)
#10 debug64-bt/js(+0x50d1a0) (js::SetObjectElementOperation(JSContext*, JSObject*, jsid, JS::Value const&) at /home/decoder/LangFuzz/mozilla-central/js/src/jsinterpinlines.h:829)
#11 debug64-bt/js(+0x519e7b) (js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) at /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:2640)
#12 debug64-bt/js(+0x50e517) (js::RunScript(JSContext*, JSScript*, js::StackFrame*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:455)
#13 debug64-bt/js(+0x50f058) (js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) at /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:657)


Not yet really beautiful I know, but I hope it's helpful :)

Comment 3

[Jesse Ruderman]

The crashing line of JSObject::makeDenseArraySlow is part of OOM recovery code that bhackett rewrote in bug 693221:

https://hg.mozilla.org/mozilla-central/rev/838464854ec6#l2.182

Comment 4

[Christian Holler (:decoder)]

I do have a fix for this that I'll post later when I'm home. not sure if it's correct of course.

Comment 5

[Christian Holler (:decoder)]

Attachment: Patch

I'm feeling lucky, so I created a patch that I think fixes the problem :D

Only call free when | elements | was actually set in | allocateSlowArrayElements |. If the ObjectElement allocation there fails and we return false, then elements remains unchanged and must not be freed.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 600734 [details] [diff] [review]
Patch

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

Oops, good catch.

Comment 7

[Christian Holler (:decoder)]

Attachment: Updated patch

Updated the patch to have proper commit message, carrying r+ from last review. Needs check-in.

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/7425d7896105

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7425d7896105

Comment 10

[Daniel Veditz [:dveditz]]

From the patch it looks like this is always going to try freeing emptyObjectHeader. Can an attacker influence what's there in a useful way? This should affect the ESR branch so we need to decide if this is a vulnerability that needs to be fixed there.

Comment 11

[Christian Holler (:decoder)]

(In reply to Daniel Veditz [:dveditz] from comment #10)
> From the patch it looks like this is always going to try freeing
> emptyObjectHeader.

Agreed. I'm opening this and I guess we won't need this for ESR.

Comment 12

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

Is this still correctly marked as an SG:Critical since we've opened it up?

Comment 13

[Christian Holler (:decoder)]

Nope :) Removed sg rating.

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Ubuntu 11.10 64bit

I built Spidermonkey for the latest firefox beta revision (5f412ea09aba) and ran the command from comment #1 (./js -A 355 -f ../jit-test/tests/jaeger/bug709067.js) and no  crash occured.

Marking verified for Firefox 13.

Comment 15

[Christian Holler (:decoder)]

Thanks for verifying this. However, for tests that use -A, simply running the command for verification on the new revision is not sufficient. The reason is that the -A parameter is the number of internal memory allocations done by the code before an out-of-memory condition is simulated. Thus, this value can easily change on a newer revision and the only way to verify the fix is to retest with a whole range (best, all), possible -A values.