1545086 - Crash when the start function is added to the table in webassembly

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[arpad.perenyi]

Attachment: index.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.103 Safari/537.36

Steps to reproduce:

Instantiate a WebAssembly module which has the start function added to the WebAssembly module table.

Sample WebAssembly code:
(module
(func)
(start 0)
(table $0 1 anyfunc)
(elem 0 (offset (i32.const 0)) 0)
)

Load in JavaScript as binary:
var buffer = new Uint8Array([0,97,115,109,1,0,0,0,1,4,1,96,0,0,3,2,1,0,4,4,1,112,0,1,8,1,0,9,7,1,0,65,0,11,1,0,10,4,1,2,0,11]);
new WebAssembly.Instance(new WebAssembly.Module(buffer));

Actual results:

Firefox error page: Your tab crashed

Submitted Crash bug Report ID: bp-e6b5de07-d721-45fe-b9e5-766b40190417

Tested in Firefox, Firefox Developer Edition, Firefox Nightly (Build ID: 20190416220148)

Expected results:

No error

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Confirmed; could repro on both nightly and release. Locking until we get a security assessment: this code has changed recently and I get why it crashes in Nightly, but I'm less certain about release.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

cc'ing Christian since it looks like a bug fuzzers could have found; and Luke because I think the Nightly crash is a recent regression from the lazier stubs bug.

Comment 3

[Luke Wagner [:luke]]

Thanks for clear report! Crash on release sounds like the issue is pre-existing; will take a look.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

On Nightly, this is safe. What's going on is that the start function is called without a callee function (it's set to the initialization value of Value, i.e. 0), and in GetInterpEntry we try to set the callee function's entry to set the jit entry. So it's a nullptr deref there: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmInstance.cpp#1570

Comment 5

[Christian Holler (:decoder)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)

cc'ing Christian since it looks like a bug fuzzers could have found; and Luke because I think the Nightly crash is a recent regression from the lazier stubs bug.

I am investigating this. The fuzzing target we have does crash with a modified sample, so this should be detectable. I don't know why libfuzzer wouldn't have picked up this, but one suspicion I have is its length setting. We run with a fairly large test length (as we concat multiple tests together for import/export behavior), which might make the fuzzing less effective to find bugs like this. I am running locally now with a much reduced maximum length to see if we detect it then.

Comment 6

[Luke Wagner [:luke]]

Thanks Benjamin!

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

[Tracking Requested - why for this release]: this is both a recent regression on Nightly builds (bug 1529957) and another older issue.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Found the bug on release:

• the function is marked exported twice: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmGenerator.cpp#357,369
• then we remove duplicates in the exported function list: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmGenerator.cpp#374
• which is based on ExportedFunc operator==, which only looks at the index!
• this results in the first ExportedFunc instance being kept, which doesn't have the isExplicit bit set to true.

So the exported function ends up not getting an eager stub, while we asked for one.

This results in a safe crash in a release optimized-non-debug build, because this will result in a nullptr deref here (the index is set to 0, but the mBegin member of the vector is set to nullptr): https://searchfox.org/mozilla-central/source/js/src/wasm/WasmCode.cpp#839 . I've checked locally, and this indeed safely crashes at the next line because the read of the lazyStubSegmentIndex is folded into the load.

Luke, do you agree? If so, we could reopen this bug. (I'll work on the fixes tomorrow)

Comment 9

[Luke Wagner [:luke]]

Nice job hunting that down; noone expects the faulty-operator=-quisition. Agreed. Because not s-s, we could also land and let the fix ride the trains, which is convenient.

Comment 10

[Luke Wagner [:luke]]

Thinking about the fix, though: it means we need to compare them not-equal but still "merge" them in a way that ||'s the isExplicit bits. Or, we could more-eagerly maintain a unified hash set that sets the bit if already-present.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

I've mistakenly flagged this bug as core-security, but I can't unflag it. Can you do it please? (We've analyzed and agreed in comment 8 and 9 that it was safe to expose, since this is a clean nullptr dereference in all the cases)

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1545086: Merge wasm exported function info to not overwrite the explicit bit; r=luke

And also change the deduplication/sorting way to be more explicit by using a
sorted array, in which we do binary search on insertion (to keep the array
sorted and avoid duplicates). The key here is that explicitly exported functions
(exported or the start function) are inserted before the table functions, so we
don't need to merge the explicit bit.

Comment 13

[Pascal Chevrel:pascalc]

Tracking for 67 in case an upliftable patch materializes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd41eefaf1b9
Merge wasm exported function info to not overwrite the explicit bit; r=luke

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/fd41eefaf1b9

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9059272 [details]
Bug 1545086: Merge wasm exported function info to not overwrite the explicit bit; r=luke

Beta/Release Uplift Approval Request

• User impact if declined: Trivial way to crash Firefox.
• 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): Self-contained changes.
• String changes made/needed:

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9059272 [details]
Bug 1545086: Merge wasm exported function info to not overwrite the explicit bit; r=luke

I forced a crash to have a signature we can monitor in the bug to have an idea of the impact on real users. Today this is not an issue in the wild and we are preparing our last week of betas before RC week. Given that this landed recently on nightly, I don't think we have enough baking time to make sure that this patch would not create regressions. I am not taking it in our last betas and prefer to let it ride the trains, if the impact to users changes during the 67 cycle we can revisit and potentially take it in a dot release. Thanks