Closed Bug 1502886 Opened 3 years ago Closed 3 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)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 + verified
firefox65 + verified

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,testComment=3,origRev=f7a97b344fa5][adv-main64+])

Crash Data

Attachments

(3 files, 3 obsolete files)

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.
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
Attached patch attempt.patch (obsolete) — Splinter Review
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 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+
Attached patch fix.patch (obsolete) — Splinter Review
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)
Attached patch test.patch (obsolete) — Splinter Review
Attachment #9021146 - Flags: review?(jcoppeard)
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 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+
Attached patch 1.fix.patchSplinter Review
Thanks for the review! Carrying over r+.
Attachment #9021145 - Attachment is obsolete: true
Attachment #9021460 - Flags: review+
Attached patch 2.test.patchSplinter Review
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+
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?
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?
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
Comment on attachment 9021460 [details] [diff] [review]
1.fix.patch

Clearing sec-approval? requests.
Attachment #9021460 - Flags: sec-approval?
Attachment #9021461 - Flags: sec-approval?
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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta approval on this when you get a chance.
Flags: needinfo?(bbouvier)
Flags: in-testsuite+
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 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+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,testComment=3,origRev=f7a97b344fa5] → [jsbugmon:update,testComment=3,origRev=f7a97b344fa5][adv-main64+]
JSBugMon: This bug has been automatically verified fixed on Fx64
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.