Bugzilla

Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)

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();
`);

Comment 3

•

6 years ago

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]

Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)

Comment 4

•

6 years ago

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

Attached patch attempt.patch (obsolete) — Details — 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)

Jon Coppeard (:jonco)

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

Attached patch fix.patch (obsolete) — Details — 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)

Assignee

Comment 8

•

6 years ago

Attached patch test.patch (obsolete) — Details — Splinter Review

Attachment #9021146 - Flags: review?(jcoppeard)

Jon Coppeard (:jonco)

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+

Jon Coppeard (:jonco)

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

Attached patch 1.fix.patch — Details — Splinter Review

Thanks for the review! Carrying over r+.

Attachment #9021145 - Attachment is obsolete: true

Attachment #9021460 - Flags: review+

Assignee

Comment 12

•

6 years ago

Attached patch 2.test.patch — Details — Splinter 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+

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?

Andrew McCreight [:mccr8]

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?

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

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

status-firefox65: affected → fixed

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

status-firefox64: affected → fixed

Flags: qe-verify-

Updated

•

6 years ago

Status: RESOLVED → VERIFIED

status-firefox65: fixed → verified

Comment 23

•

6 years ago

JSBugMon: This bug has been automatically verified fixed.

Updated

•

5 years ago

Whiteboard: [jsbugmon:update,testComment=3,origRev=f7a97b344fa5] → [jsbugmon:update,testComment=3,origRev=f7a97b344fa5][adv-main64+]

Updated

•

5 years ago

status-firefox64: fixed → verified