Last Comment Bug 794025 - AddressSanitizer heap-use-after-free in [@ js::mjit::CallCompiler::generateNativeStub]
: AddressSanitizer heap-use-after-free in [@ js::mjit::CallCompiler::generateNa...
Status: RESOLVED FIXED
[asan][advisory-tracking+]
: csectype-uaf, sec-critical, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Sean Stangl [:sstangl]
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2012-09-25 04:02 PDT by Christian Holler (:decoder)
Modified: 2014-01-10 10:41 PST (History)
11 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
16+
fixed


Attachments
Properly detect IC purging. (2.14 KB, patch)
2012-09-25 14:53 PDT, Sean Stangl [:sstangl]
dvander: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
dveditz: sec‑approval+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-09-25 04:02:32 PDT
The following testcase triggers an ASan error on mozilla-central revision e327e66a027d (no options required):


function getterFunction(v) { return "getter"; }
Object.defineProperty(Array.prototype, 1,{ 
  get: getterFunction, 
});
var N = (10000);
repeat_str("try { f(); } finally {\n", N),
repeat_str("}", ("" ));
function repeat_str(str, repeat_count) {
  var arr = new Array(--repeat_count);
  while (repeat_count != 0)
    arr[--repeat_count] = str;
  return str.concat.apply(str, arr);
}


ASan trace:

==24366== ERROR: AddressSanitizer heap-use-after-free on address 0x7f19004f036c at pc 0xc1a8fb bp 0x7fff512c8450 sp 0x7fff512c8448
READ of size 8 at 0x7f19004f036c thread T0
    #0 0xc1a8fa in js::mjit::CallCompiler::generateNativeStub() js/src/methodjit/MonoIC.cpp:1082
    #1 0xc191a2 in js::mjit::ic::NativeCall(js::VMFrame&, js::mjit::ic::CallICInfo*) js/src/methodjit/MonoIC.cpp:1308
    #2 0xa6da22 in throwpoline_exit js/src/methodjit/MethodJIT.cpp:0
    #3 0xa6e249 in js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) js/src/methodjit/MethodJIT.cpp:1045
    #4 0xa6ed7b in JSContext::fp() const js/src/methodjit/MethodJIT.cpp:1103
    #5 0x5d9873 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:1487
    #6 0x5bbb03 in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324
    #7 0x5e8fbf in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:509
    #8 0x5e9519 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:546
    #9 0x46d6f3 in JS_ExecuteScript js/src/jsapi.cpp:5632
    #10 0x41f5b4 in Process(JSContext*, JSObject*, char const*, bool) js/src/shell/js.cpp:439
    #11 0x41d3ed in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:4700
    #12 0x41e309 in main js/src/shell/js.cpp:4944
    #13 0x7f1906bafcdc in __libc_start_main ??:0
0x7f19004f036c is located 748 bytes inside of 1028-byte region [0x7f19004f0080,0x7f19004f0484)
freed by thread T0 here:
    #0 0xf56840 in __interceptor_free ??:0
    #1 0xa72f57 in js_free ./dist/include/js/Utility.h:170
previously allocated by thread T0 here:
    #0 0xf56971 in __interceptor_calloc ??:0
    #1 0xaa2ed9 in js_calloc ./dist/include/js/Utility.h:159
    #2 0xa79e92 in js::mjit::Compiler::performCompilation() js/src/methodjit/Compiler.cpp:564
    #3 0xa78cb5 in js::mjit::Compiler::compile() js/src/methodjit/Compiler.cpp:140
    #4 0x5d8876 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:1480
    #5 0x5bbb03 in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324
    #6 0x5e8fbf in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:509
    #7 0x5e9519 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:546
    #8 0x46d6f3 in JS_ExecuteScript js/src/jsapi.cpp:5632
    #9 0x41f5b4 in Process(JSContext*, JSObject*, char const*, bool) js/src/shell/js.cpp:439
    #10 0x41d3ed in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:4700
    #11 0x41e309 in main js/src/shell/js.cpp:4944
    #12 0x7f1906bafcdc in __libc_start_main ??:0


The issue is also visible in Valgrind which might be easier for debugging purposes. Assuming sec-critical because it seems that we're using a JITChunk (compiled part of a script) after it has been free'd.
Comment 1 Sean Stangl [:sstangl] 2012-09-25 14:53:41 PDT
Created attachment 664659 [details] [diff] [review]
Properly detect IC purging.

ic::SplatApplyArgs() can cause a GC, but the RecompilationMonitor was only instantiated after that function was called, so the GC was undetected and we referenced an already-freed IC.
Comment 2 Sean Stangl [:sstangl] 2012-09-25 15:10:42 PDT
Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Very easily.

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?

All

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

Patch is the same for all branches.

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

Can't cause regressions. No testing is needed.
Comment 3 Daniel Veditz [:dveditz] 2012-09-26 12:34:35 PDT
Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

sec-approval+ to land this bug now if you can get it landed on all branches (esp Beta) by Friday Sept 28.

Still need separate drivers approval for those branches AFAIK (since part of asking them is so they aren't surprised by landings). The patch looks like it would apply fine back to ESR-10.
Comment 4 Alex Keybl [:akeybl] 2012-09-26 14:40:35 PDT
Comment on attachment 664659 [details] [diff] [review]
Properly detect IC purging.

[Triage Comment]
Feel free to land this on Aurora/Beta/ESR10 as soon as you see green on m-i/m-c. As Dan notes, please do not land later than Friday.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-09-26 18:45:27 PDT
https://hg.mozilla.org/mozilla-central/rev/f8daf97f682b
Comment 8 Christian Holler (:decoder) 2013-03-12 20:09:00 PDT
Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51d9ab333b9
Comment 9 Ed Morley [:emorley] 2013-03-13 05:35:57 PDT
https://hg.mozilla.org/mozilla-central/rev/a51d9ab333b9
Comment 10 Tracy Walker [:tracy] 2014-01-10 10:41:45 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.