Closed Bug 794025 Opened 7 years ago Closed 7 years ago

AddressSanitizer heap-use-after-free in [@ js::mjit::CallCompiler::generateNativeStub]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox15 --- wontfix
firefox16 + fixed
firefox17 + fixed
firefox18 + fixed
firefox-esr10 16+ fixed

People

(Reporter: decoder, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [asan][advisory-tracking+])

Attachments

(1 file)

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.
Assignee: general → sstangl
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.
Attachment #664659 - Flags: review?(dvander)
Attachment #664659 - Flags: review?(dvander) → review+
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.
Attachment #664659 - Flags: sec-approval?
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.
Attachment #664659 - Flags: sec-approval?
Attachment #664659 - Flags: sec-approval+
Attachment #664659 - Flags: approval-mozilla-esr10?
Attachment #664659 - Flags: approval-mozilla-beta?
Attachment #664659 - Flags: approval-mozilla-aurora?
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.
Attachment #664659 - Flags: approval-mozilla-esr10?
Attachment #664659 - Flags: approval-mozilla-esr10+
Attachment #664659 - Flags: approval-mozilla-beta?
Attachment #664659 - Flags: approval-mozilla-beta+
Attachment #664659 - Flags: approval-mozilla-aurora?
Attachment #664659 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f8daf97f682b
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [asan] → [asan][advisory-tracking+]
Group: core-security
Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51d9ab333b9
Flags: in-testsuite? → in-testsuite+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.