AddressSanitizer: heap-use-after-free [@ js::detail::HashTableEntry<js::ReadBarriered<js::GlobalObject*> const>::isLive] with READ of size 4 with Debugger

VERIFIED FIXED in Firefox 48

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla49
x86_64
Linux
crash, csectype-uaf, regression, sec-high, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48+ verified, firefox49+ verified, firefox-esr45 unaffected)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
The following testcase crashes on mozilla-central revision ae7413abfa4d (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --baseline-eager):

try {
var g = newGlobal();
var dbg = new Debugger(g);
function test(type, provocation) {
    dbg.onEnterFrame = function handleFirstFrame(f) {
        dbg.onEnterFrame = function handleSecondFrame(f) {
            dbg.onEnterFrame = function handleThirdFrame(f) {
            };
        };
    };
}
test('global', 'evaluate(\'debugger; throw \\\'mud\\\';\');');
} catch(exc2) {}
try {
dbg.addDebuggee(g);
g.eval("" + function f() {
  function g() { }
});
var g = newGlobal();
g.evaluate("function f(x) { return x + 1; }");
schedulegc(10);
var gw = dbg.addDebuggee(g);
var s = dbg.findScripts();
} catch(exc217) {}



Backtrace:

==3515==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000030150 at pc 0x1387c85 bp 0x7fffd97fb670 sp 0x7fffd97fb668
READ of size 4 at 0x615000030150 thread T0
    #0 0x1387c84 in js::detail::HashTableEntry<js::ReadBarriered<js::GlobalObject*> const>::isLive() const js/src/opt64asan/dist/include/js/HashTable.h:789
    #1 0x1387c84 in js::HashSet<js::ReadBarriered<js::GlobalObject*>, js::MovableCellHasher<js::ReadBarriered<js::GlobalObject*> >, js::RuntimeAllocPolicy>::all() const js/src/opt64asan/dist/include/js/HashTable.h:977
    #2 0x1387c84 in js::Debugger::ScriptQuery::matchAllDebuggeeGlobals() js/src/vm/Debugger.cpp:4129
    #3 0x12f36e3 in js::Debugger::ScriptQuery::omittedQuery() js/src/vm/Debugger.cpp:3968
    #4 0x12f36e3 in js::Debugger::findScripts(JSContext*, unsigned int, JS::Value*) js/src/vm/Debugger.cpp:4282
    #5 0x14cd4d9 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #6 0x14cd4d9 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:480
    #7 0x84cdef in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:6116

0x615000030150 is located 336 bytes inside of 512-byte region [0x615000030000,0x615000030200)
freed by thread T0 here:
    #0 0x4b438f in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x1370db7 in js_free(void*) js/src/opt64asan/dist/include/js/Utility.h:265
    #2 0x1370db7 in js::RuntimeAllocPolicy::free_(void*) js/src/vm/Runtime.h:1933
    #3 0x1370db7 in js::detail::HashTable<js::ReadBarriered<js::GlobalObject*> const, js::HashSet<js::ReadBarriered<js::GlobalObject*>, js::MovableCellHasher<js::ReadBarriered<js::GlobalObject*> >, js::RuntimeAllocPolicy>::SetOps, js::RuntimeAllocPolicy>::changeTableSize(int, js::detail::HashTable<js::ReadBarriered<js::GlobalObject*> const, js::HashSet<js::ReadBarriered<js::GlobalObject*>, js::MovableCellHasher<js::ReadBarriered<js::GlobalObject*> >, js::RuntimeAllocPolicy>::SetOps, js::RuntimeAllocPolicy>::FailureBehavior) js/src/opt64asan/dist/include/js/HashTable.h:1445
    #4 0x12e42cc in js::Debugger::removeDebuggeeGlobal(js::FreeOp*, js::GlobalObject*, js::detail::HashTable<js::ReadBarriered<js::GlobalObject*> const, js::HashSet<js::ReadBarriered<js::GlobalObject*>, js::MovableCellHasher<js::ReadBarriered<js::GlobalObject*> >, js::RuntimeAllocPolicy>::SetOps, js::RuntimeAllocPolicy>::Enum*) js/src/vm/Debugger.cpp:3774
    #5 0x12e4cd0 in js::Debugger::detachAllDebuggersFromGlobal(js::FreeOp*, js::GlobalObject*) js/src/vm/Debugger.cpp:2874
    #6 0x1045a2e in JSCompartment::sweepGlobalObject(js::FreeOp*) js/src/jscompartment.cpp:686
    #7 0x10e4364 in js::gc::GCRuntime::beginSweepingZoneGroup() js/src/jsgc.cpp:5245
    #8 0x10e75c3 in js::gc::GCRuntime::beginSweepPhase(bool) js/src/jsgc.cpp:5428
    #9 0x10ee836 in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6151
    #10 0x10f0522 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6388
    #11 0x10f196b in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6496
    #12 0x10f8c54 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6556
    #13 0x10f8c54 in js::gc::GCRuntime::runDebugGC() js/src/jsgc.cpp:7039
    #14 0x18a4508 in js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) js/src/gc/Allocator.cpp:28
    #15 0x18b7e46 in JSContext::runtime() const js/src/gc/Allocator.cpp:55
    #16 0x18b7e46 in JSScript* js::Allocate<JSScript, (js::AllowGC)1>(js::ExclusiveContext*) js/src/gc/Allocator.cpp:213
    #17 0x11c80d3 in JSScript::Create(js::ExclusiveContext*, JS::Handle<JSObject*>, bool, JS::ReadOnlyCompileOptions const&, JS::Handle<JSObject*>, unsigned int, unsigned int) js/src/jsscript.cpp:2804
    #18 0x180d49f in js::frontend::CompileLazyFunction(JSContext*, JS::Handle<js::LazyScript*>, char16_t const*, unsigned long) js/src/frontend/BytecodeCompiler.cpp:832
    #19 0x10ab934 in JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, JS::Handle<JSFunction*>) js/src/jsfun.cpp:1435
    #20 0x1048b20 in JSFunction::getOrCreateScript(JSContext*) js/src/jsfun.h:413
    #21 0x1048b20 in JSCompartment::needsDelazificationForDebugger() const js/src/jscompartment.cpp:1033
    #22 0x1048b20 in JSCompartment::ensureDelazifyScriptsForDebugger(JSContext*) js/src/jscompartment.cpp:1047
    #23 0x1387ea9 in js::Debugger::ScriptQuery::addCompartment(JSCompartment*) js/src/vm/Debugger.cpp:4106
    #24 0x1387bfe in js::Debugger::ScriptQuery::matchAllDebuggeeGlobals() js/src/vm/Debugger.cpp:4130
    #25 0x12f36e3 in js::Debugger::ScriptQuery::omittedQuery() js/src/vm/Debugger.cpp:3968
    #26 0x12f36e3 in js::Debugger::findScripts(JSContext*, unsigned int, JS::Value*) js/src/vm/Debugger.cpp:4282
    #27 0x14cd4d9 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
[...]
    #34 0x14d0202 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:736

SUMMARY: AddressSanitizer: heap-use-after-free js/src/opt64asan/dist/include/js/HashTable.h:789 js::detail::HashTableEntry<js::ReadBarriered<js::GlobalObject*> const>::isLive() const
Shadow bytes around the buggy address:
  0x0c2a7fffe010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2a7fffe020: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
  0x0c2a7fffe030: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Freed heap region:       fd
==3515==ABORTING

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

2 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f66ab359f3ef
user:        Jon Coppeard
date:        Wed Mar 23 02:30:00 2016 -0400
summary:     Bug 1258436 - Remove GC suppression in JSFunction::createScriptForLazilyInterpretedFunction. r=sfink

This iteration took 263.751 seconds to run.
Jon, is bug 1258436 a likely regressor?
Flags: needinfo?(jcoppeard)

Comment 3

2 years ago
UAF is security-sensitive by default.  The need for Debugger mitigates, but we don't know that all Debugger users won't tickle in just the right way, or web-content-provokably, so it's still a bad thing.
Group: javascript-core-security
(Assignee)

Comment 4

2 years ago
ScriptQuery::addCompartment calls JSCompartment::ensureDelazifyScriptsForDebugger and now that JSFunction::createScriptForLazilyInterpretedFunction can GC, we're hitting a GC and modifiying a debugger's debuggees set while it's being traversed by ScriptQuery::matchAllDebuggeeGlobals.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 5

2 years ago
Created attachment 8744918 [details] [diff] [review]
bug1266434-debugger-delazification

The simple thing to do is to do the delazification in a separate phase after finding all the compartments we are interested in.
Assignee: nobody → jcoppeard
Attachment #8744918 - Flags: review?(jimb)

Comment 6

2 years ago
Okay, I can reproduce this.
Assignee: jcoppeard → jimb

Comment 7

2 years ago
Oh, sorry, misread. Thought jonco was asking me to fix it...
Assignee: jimb → jcoppeard

Updated

2 years ago
Attachment #8744918 - Flags: review?(jimb) → review+
(Assignee)

Updated

2 years ago
Blocks: 1258436
(Assignee)

Comment 8

2 years ago
Comment on attachment 8744918 [details] [diff] [review]
bug1266434-debugger-delazification

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Hard, depends on GC timing and use of debugger.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? 48 is affected.

If not all supported branches, which bug introduced the flaw? Bug 1258436.

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Attachment #8744918 - Flags: sec-approval?
Keywords: csectype-uaf, sec-high
Giving sec-approval for Trunk (49). We'll want an Aurora (48) patch as well. Can you please make one or nominate the existing if it applies cleanly?
status-firefox47: --- → unaffected
status-firefox49: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox48: --- → +
tracking-firefox49: --- → +
Attachment #8744918 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/9892ac28ce0d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified

Comment 11

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
(Assignee)

Comment 12

2 years ago
Comment on attachment 8744918 [details] [diff] [review]
bug1266434-debugger-delazification

Approval Request Comment
[Feature/regressing bug #]: Bug 1258436.
[User impact if declined]: Possible security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 28th May.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8744918 - Flags: approval-mozilla-aurora?
Comment on attachment 8744918 [details] [diff] [review]
bug1266434-debugger-delazification

Fix for sec-high vulnerability, please uplift to aurora
Attachment #8744918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca5c6ee18745
status-firefox48: affected → fixed

Updated

2 years ago
status-firefox48: fixed → verified

Comment 15

2 years ago
JSBugMon: This bug has been automatically verified fixed on Fx48

Updated

2 years ago
Duplicate of this bug: 1262395
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.