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

RESOLVED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: decoder, Assigned: sstangl)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla18
x86_64
Linux
csectype-uaf, sec-critical, testcase, valgrind
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 wontfix, firefox16+ fixed, firefox17+ fixed, firefox18+ fixed, firefox-esr1016+ fixed)

Details

(Whiteboard: [asan][advisory-tracking+])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: general → sstangl
(Assignee)

Comment 1

5 years ago
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.
Attachment #664659 - Flags: review?(dvander)
Attachment #664659 - Flags: review?(dvander) → review+
(Assignee)

Comment 2

5 years ago
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?
status-firefox-esr10: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → +
tracking-firefox18: --- → +
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?
tracking-firefox-esr10: ? → 16+
tracking-firefox16: ? → +

Comment 4

5 years ago
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+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f8daf97f682b
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/89026001a0a9
http://hg.mozilla.org/releases/mozilla-beta/rev/0b6fd624e847
http://hg.mozilla.org/releases/mozilla-aurora/rev/66f30e40a741
status-firefox-esr10: affected → fixed
status-firefox15: --- → wontfix
status-firefox16: affected → fixed
status-firefox17: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/f8daf97f682b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox18: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [asan] → [asan][advisory-tracking+]
Keywords: verifyme
Group: core-security
(Reporter)

Comment 8

4 years ago
Test added:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51d9ab333b9
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a51d9ab333b9
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.