Closed
Bug 1502886
Opened 6 years ago
Closed 6 years ago
Crash [@ mozilla::UniquePtr<js::wasm::CodeTier, JS::DeletePolicy<js::wasm::CodeTier> >::operator->] or Crash [@ js::gc::detail::GetCellLocation] with OOM, Debugger and use-after-free
Categories
(Core :: JavaScript: WebAssembly, defect)
Tracking
()
VERIFIED
FIXED
mozilla65
People
(Reporter: decoder, Assigned: bbouvier)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [jsbugmon:update,testComment=3,origRev=f7a97b344fa5][adv-main64+])
Crash Data
Attachments
(3 files, 3 obsolete files)
1.73 KB,
text/plain
|
Details | |
12.12 KB,
patch
|
bbouvier
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.26 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
The attached testcase crashes on mozilla-central revision 7007206a3cd4 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off). Backtrace: received signal SIGSEGV, Segmentation fault. #0 mozilla::UniquePtr<js::wasm::CodeTier, JS::DeletePolicy<js::wasm::CodeTier> >::operator-> (this=0xe5e5e5e5e5e5e5ed) at dist/include/mozilla/UniquePtr.h:320 #1 0x0000555555e49db4 in js::wasm::Code::codeTier (this=0xe5e5e5e5e5e5e5e5, tier=tier@entry=js::wasm::Tier::Baseline) at js/src/wasm/WasmCode.cpp:1326 #2 0x0000555555edf5dc in js::wasm::Code::metadata (iter=js::wasm::Tier::Baseline, this=<optimized out>) at js/src/wasm/WasmCode.h:725 #3 js::wasm::DebugState::metadata (t=js::wasm::Tier::Baseline, this=0x7ffff5503740) at js/src/wasm/WasmDebug.h:120 #4 js::wasm::DebugState::toggleBreakpointTrap (this=0x7ffff5503740, rt=0x7ffff5f1c000, offset=60, enabled=<optimized out>) at js/src/wasm/WasmDebug.cpp:191 #5 0x0000555555b18ee2 in js::Breakpoint::destroy (this=0x7ffff55ff680, fop=0x7fffffffd310) at js/src/vm/Debugger.cpp:624 #6 0x0000555555b5f4e4 in js::Debugger::removeDebuggeeGlobal (this=0x7ffff5f91800, fop=0x7fffffffd310, global=<optimized out>, debugEnum=<optimized out>) at js/src/vm/Debugger.cpp:4556 #7 0x0000555555b61ce9 in js::Debugger::sweepAll (fop=fop@entry=0x7fffffffd310) at js/src/vm/Debugger.cpp:3541 #8 0x00005555560396e7 in js::gc::GCRuntime::sweepDebuggerOnMainThread (this=this@entry=0x7ffff5f1c6d0, fop=fop@entry=0x7fffffffd310) at js/src/gc/GC.cpp:5856 #9 0x0000555556039fb3 in js::gc::GCRuntime::beginSweepingSweepGroup (this=0x7ffff5f1c6d0, fop=<optimized out>, budget=...) at js/src/gc/GC.cpp:6060 #10 0x0000555556067e60 in sweepaction::SweepActionSequence<js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f17380, args#0=0x7ffff5f1c6d0, args#1=0x7fffffffd310, args#2=...) at js/src/gc/GC.cpp:6748 #11 0x000055555606832a in sweepaction::SweepActionRepeatFor<js::gc::SweepGroupsIter, JSRuntime*, js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f190a0, args#0=0x7ffff5f1c6d0, args#1=0x7fffffffd310, args#2=...) at js/src/gc/GC.cpp:6812 #12 0x0000555556019d37 in js::gc::GCRuntime::performSweepActions (this=this@entry=0x7ffff5f1c6d0, budget=...) at js/src/gc/GC.cpp:6990 #13 0x0000555556049495 in js::gc::GCRuntime::incrementalSlice (this=this@entry=0x7ffff5f1c6d0, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME, session=...) at js/src/gc/GC.cpp:7590 #14 0x000055555604a89e in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f1c6d0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:7946 #15 0x000055555604b08d in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f1c6d0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:8127 #16 0x000055555604b459 in js::gc::GCRuntime::gc (this=this@entry=0x7ffff5f1c6d0, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/gc/GC.cpp:8210 #17 0x0000555555d407cf in JSRuntime::destroyRuntime (this=0x7ffff5f1c000) at js/src/vm/Runtime.cpp:300 #18 0x0000555555c8f87c in js::DestroyContext (cx=0x7ffff5f18000) at js/src/vm/JSContext.cpp:217 #19 0x00005555557ec562 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:10987 rax 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rbx 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rcx 0x0 0 rdx 0x3c 60 rsi 0x0 0 rdi 0xe5e5e5e5e5e5e5ed -1880844493789993491 rbp 0x7fffffffcb80 140737488341888 rsp 0x7fffffffcb68 140737488341864 r8 0x7ffff5fb0000 140737320255488 r9 0x7fffffffcbb0 140737488341936 r10 0x1b 27 r11 0x246 582 r12 0xe5e5e5e5e5e5e5ed -1880844493789993491 r13 0x7ffff5f91000 140737320128512 r14 0x7ffff5f91800 140737320130560 r15 0x7ffff55ff700 140737310095104 rip 0x555555e8d4d0 <mozilla::UniquePtr<js::wasm::CodeTier, JS::DeletePolicy<js::wasm::CodeTier> >::operator->() const> => 0x555555e8d4d0 <mozilla::UniquePtr<js::wasm::CodeTier, JS::DeletePolicy<js::wasm::CodeTier> >::operator->() const>: mov (%rdi),%rax 0x555555e8d4d3 <mozilla::UniquePtr<js::wasm::CodeTier, JS::DeletePolicy<js::wasm::CodeTier> >::operator->() const+3>: test %rax,%rax Marking s-s because this is a clear use-after-free/GC problem. The fact that it requires the debugger and some sort of OOM condition probably makes this sec-moderate.
Reporter | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
There's a compacting GC after a wasm instance has had breakpoints but the wasm instance isn't alive anymore. Going up on the stack leads up to https://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp#3480. Investigating more... Reduced a bit the test case: Object.defineProperty(this, "fuzzutils", { value:{ newGlobal: newGlobal, evaluate: function(c, o) { if (!o) { o = {}; } o.catchTermination = true; return evaluate(c, o); }, } }); try { fuzzutils.evaluate(` var g = newGlobal(); `); fuzzutils.evaluate(``); fuzzutils.evaluate(``); } catch(exc) {} fuzzutils.evaluate(` var g = newGlobal(); var dbg = Debugger(g); `); fuzzutils.evaluate(` gcparam("maxBytes", gcparam("gcBytes") + 4*1024); function runTest(wast, initFunc, doneFunc) { let of = newGlobal(''); g.eval(\` var { binary, offsets } = wasmTextToBinary('\${wast}', true); var m = new WebAssembly.Instance(new WebAssembly.Module(binary)); \`); var { offsets } = g; var wasmScript = dbg.findScripts().filter(s => s.format == 'wasm')[0]; initFunc({ wasmScript, breakpoints: offsets }); } runTest( '(module (func (nop) (nop)) (export "test" 0))', function ({wasmScript, breakpoints}) {} ); runTest( '(module (func (nop) (nop)) (export "test" 0))', function ({wasmScript, breakpoints}) { var handlers = []; breakpoints.forEach(function (offset, i) { wasmScript.setBreakpoint(offset, handlers[i] = {}); }); }, ); runTest(); `);
newGlobal(); g = newGlobal(); var dbg = Debugger(g); gcparam("maxBytes", gcparam("gcBytes") + 1); function f(x, initFunc) { newGlobal(); g.eval(` var { binary, offsets } = wasmTextToBinary('${x}', true); new WebAssembly.Instance(new WebAssembly.Module(binary)); `); var { offsets } = g; var wasmScript = dbg.findScripts().filter(s => s.format == 'wasm')[0]; initFunc({ wasmScript, breakpoints: offsets }) }; f('(module (funcnopnop)(export "" 0))', function({ wasmScript, breakpoints }) { breakpoints.forEach(function(offset) { wasmScript.setBreakpoint(offset, s = {}); }); } ); f(); is an even more reduced testcase from comment 2. Tested with --enable-debug --enable-more-deterministic, --fuzzing-safe --no-threads --no-baseline --no-ion on m-c rev f7a97b344fa5
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,testComment=3,origRev=f7a97b344fa5]
autobisectjs shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/38d2f921a918 user: Benjamin Bouvier date: Wed Jun 20 17:56:19 2018 +0200 summary: Bug 1447591: Stub out a few Debugger APIs for wasm; r=yury Probably related to bug 1447591?
Blocks: 1447591
Assignee | ||
Comment 5•6 years ago
|
||
The following is an attempt to fix this, by iterating over all wasm instances about to be destroyed when sweeping a zone, and destroying attached breakpoints if needed. Unfortunately, this requires iterating over a hash map, which elements get destroyed as we iterate over them. The solution previously was to allocate an array to store all the elements first, so we don't run into hash map generation issues, but it seems this Zone method can't return a bool value (in case of allocation failure). Any ideas regarding how to work around this, Jon?
Attachment #9020852 -
Flags: feedback?(jcoppeard)
Comment 6•6 years ago
|
||
Comment on attachment 9020852 [details] [diff] [review] attempt.patch Review of attachment 9020852 [details] [diff] [review]: ----------------------------------------------------------------- I feel like the best thing to do would to be to add a version of Breakpoint::destroy() that didn't call |site->destroyIfEmpty()| for this case, and then destroying them yourself in the loop if they end up empty. If you don't feel like doing that using AutoEnterOOMUnsafeRegion and crashing in you can't allocate is fine. ::: js/src/gc/Zone.cpp @@ +193,5 @@ > + if (!instance->debugEnabled()) { > + continue; > + } > + WasmInstanceObject* obj = instance->objectUnbarriered(); > + if (!IsAboutToBeFinalizedUnbarriered(&obj)) { It's best to avoid the ...Unbarriered methods(). Can you expose Instance::object_ to this method and use |IsAboutToBeFinalized(&instance->object_)| instead?
Attachment #9020852 -
Flags: feedback?(jcoppeard) → feedback+
Assignee | ||
Comment 7•6 years ago
|
||
Thanks for the feedback! It's definitely possible to change iteration to mutable iteration by breaking apart the Breakpoint/BreakpointSite deletion, as is done here. It's a nice refactoring overall. (Test not included since it might land separately)
Assignee: nobody → bbouvier
Attachment #9020852 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9021145 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #9021146 -
Flags: review?(jcoppeard)
Comment 9•6 years ago
|
||
Comment on attachment 9021145 [details] [diff] [review] fix.patch Review of attachment 9021145 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Zone.cpp @@ +195,5 @@ > + } > + if (!IsAboutToBeFinalized(&instance->object_)) { > + continue; > + } > + instance->debug().clearAllBreakpoints(fop, instance->object_); You want objectUnbarriered() here, it's only the IsAboutToBeFinalized call that needs the address of the the ReadBarrieredWasmInstanceObject. ::: js/src/vm/Debugger.h @@ +1810,5 @@ > mozilla::DoublyLinkedListElement<Breakpoint> siteLink; > > public: > Breakpoint(Debugger* debugger, BreakpointSite* site, JSObject* handler); > + void destroy(FreeOp* fop, bool mayDestroySite = true); nit: it would be better to use an enum rather than a bool here. ::: js/src/wasm/WasmDebug.cpp @@ +268,5 @@ > } > + if (site->isEmpty()) { > + fop->delete_(site); > + e.removeFront(); > + } Great, this is much nicer.
Attachment #9021145 -
Flags: review?(jcoppeard) → review+
Comment 10•6 years ago
|
||
Comment on attachment 9021146 [details] [diff] [review] test.patch Review of attachment 9021146 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/wasm/regress/bug1502886.js @@ +1,4 @@ > +newGlobal(); > +g = newGlobal(); > +var dbg = Debugger(g); > +gcparam("maxBytes", gcparam("gcBytes") + 1); Tests involving setting maxBytes have tended to be fragile in the past because they rely on how much memory the shell allocates on startup. Can you reproduce the failure using a gczeal setting instead? I'd suggest gczeal(2, N) where N is something smallish.
Attachment #9021146 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 11•6 years ago
|
||
Thanks for the review! Carrying over r+.
Attachment #9021145 -
Attachment is obsolete: true
Attachment #9021460 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
Test with gczeal(2, 100) which triggers the same issue very quickly. (any value < 100 actually works)
Attachment #9021146 -
Attachment is obsolete: true
Attachment #9021461 -
Flags: review+
Updated•6 years ago
|
status-firefox63:
--- → wontfix
status-firefox64:
--- → affected
status-firefox-esr60:
--- → unaffected
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 9021460 [details] [diff] [review] 1.fix.patch [Security Approval Request] How easily could an exploit be constructed based on the patch?: Not clear but hard to imagine. It's a use-after-free, but it happens in the Debugger code, which is privileged but on which end users have very little control (they can only affect it through setting breakpoints, stepping in/over etc.). I expect that in the worst case, the memory corruption would lead to a crash. But I am not sure about this; maybe the security team has previously seen issues like this and would have a different opinion? 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?: Probably all since we implemented wasm debugging; needs to be confirmed. If not all supported branches, which bug introduced the flaw?: None Do you have backports for the affected branches?: No If not, how different, hard to create, and risky will they be?: They would probably not be very different and not very risky. How likely is this patch to cause regressions; how much testing does it need?: Not likely to cause regressions. Automated tests of the Debugger API (shell tests) pass. More manual testing on a browser build would be appreciated, but not mandatory.
Attachment #9021460 -
Flags: sec-approval?
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 9021461 [details] [diff] [review] 2.test.patch [Security Approval Request] How easily could an exploit be constructed based on the patch?: Test accompanying the previous fix. 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?: If not all supported branches, which bug introduced the flaw?: None Do you have backports for the affected branches?: Yes If not, how different, hard to create, and risky will they be?: How likely is this patch to cause regressions; how much testing does it need?:
Attachment #9021461 -
Flags: sec-approval?
Comment 15•6 years ago
|
||
We usually rate bugs that require the debugger do particular things as sec-moderate. Sec-moderate bugs don't need sec-approval to land.
Keywords: sec-moderate
Updated•6 years ago
|
tracking-firefox64:
--- → +
tracking-firefox65:
--- → +
Comment 16•6 years ago
|
||
Comment on attachment 9021460 [details] [diff] [review] 1.fix.patch Clearing sec-approval? requests.
Attachment #9021460 -
Flags: sec-approval?
Updated•6 years ago
|
Attachment #9021461 -
Flags: sec-approval?
Assignee | ||
Comment 17•6 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab04d8c725fd0cbd61e6c32e4096ee4caa53d55c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa663ad84e3695d8be66f8b30cdcf663d19228e4
Comment 18•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ab04d8c725fd https://hg.mozilla.org/mozilla-central/rev/aa663ad84e36
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment 19•6 years ago
|
||
Please request Beta approval on this when you get a chance.
Flags: needinfo?(bbouvier)
Flags: in-testsuite+
Assignee | ||
Comment 20•6 years ago
|
||
Comment on attachment 9021460 [details] [diff] [review] 1.fix.patch [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: wasm debugging User impact if declined: Possible crashes when/after debugging wasm code. Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): Passes all wasm tests on all platforms, had been in Nightly for a few days without reports of issues, reuses existent code. String changes made/needed:
Flags: needinfo?(bbouvier)
Attachment #9021460 -
Flags: approval-mozilla-beta?
Comment 21•6 years ago
|
||
Comment on attachment 9021460 [details] [diff] [review] 1.fix.patch [Triage Comment] Fixes a crash while debugging wasm code. Approved for 64.0b8.
Attachment #9021460 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f65ddbf6c005 https://hg.mozilla.org/releases/mozilla-beta/rev/12500028fe79
Flags: qe-verify-
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Comment 23•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,testComment=3,origRev=f7a97b344fa5] → [jsbugmon:update,testComment=3,origRev=f7a97b344fa5][adv-main64+]
Updated•5 years ago
|
Comment 24•5 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx64
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•