1637430 - Bounds check ref.func function index in global initializers

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Ryan Hunt [:rhunt]]

The decoder for ref.func in wasm globals doesn't bounds check the function index and later code assumes that it's valid [1] [2].

Being a bit paranoid here, I'm not sure if this could be exploited. This code is nightly only and this is a recent regression.

[1] https://searchfox.org/mozilla-central/rev/0688ffdef223dac527c2fcdb25560118c4e4df51/js/src/wasm/WasmValidate.cpp#2309
[2] https://searchfox.org/mozilla-central/rev/0688ffdef223dac527c2fcdb25560118c4e4df51/js/src/wasm/WasmGenerator.cpp#401

Comment 1

[Ryan Hunt [:rhunt]]

Attachment: Bug 1637430 - Validate ref.func indices in global initializers. r?lth

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1637430 - Add tests. r?lth

Depends on D74970

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1612804

Comment 4

[Julien Cristau [:jcristau]]

Per comment 0 this is nightly only.

Comment 5

[Daniel Veditz [:dveditz]]

Being equally paranoid we'll call this sec-high. We should never ever trust wasm to be well formed.

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/51706a543abf9fd2ad8812eb145db67f81c39217
https://hg.mozilla.org/mozilla-central/rev/51706a543abf

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 8

[Ryan Hunt [:rhunt]]

Submitted.

Comment 9

[Lars T Hansen [:lth]]

Test case has bitrotted: file moved from .../gc/... to .../ref-types/... and when suitably fixed it fails with a bunch of unrelated assertions, but this looks like it's localized to the test runner (conflicting definitions of globalSection). Will update and land.

Comment 10

[Lars T Hansen [:lth]]

Actually, Ryan - did this land, but without closing the patch? There's code at the bottom of ref-types/ref-func.js that looks a lot like this, but some of the names are changed.

Comment 11

[Ryan Hunt [:rhunt]]

I don't think this landed, which code are you looking at? This code [1] doesn't seem to be it.

Also, I can rebase the patch and land it now.

[1] https://searchfox.org/mozilla-central/rev/2a24205479519e70c0574929f45730d285141584/js/src/jit-test/tests/wasm/ref-types/ref-func.js#210

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc2974e9876d
Add tests. r=lth

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for SM bustages on asserts.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/6cfaa4fa1c1f3478074a0fec28155d28fa301345
Log link: https://treeherder.mozilla.org/logviewer?job_id=326706819&repo=autoland&lineNumber=11245

Comment 14

[Ryan Hunt [:rhunt]]

Ah, Cranelift uses a different failure message here and I didn't test on ARM64.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b95d22164a9f
Add tests. r=lth

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

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