1833681 - Subtypes can leak through block params and results

Status: RESOLVED FIXED

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

Description

[Ben Visness [:bvisness]]

When exiting a block, we verify that the stack matches the block's result type, but we do not actually replace the stack with the block's result type. Now that subtyping has been introduced to wasm, more specific types can leak out of blocks and allow code to validate that shouldn't.

Comment 1

[Ben Visness [:bvisness]]

Attachment: Bug 1833681: Fix checkTopTypeMatches. r=rhunt

Comment 2

[Daniel Veditz [:dveditz]]

I assume this is an old problem and not a recent regresion? Will we need to fix ESR?

Comment 3

[Ryan Hunt [:rhunt]]

This is a latent issue in our type checking code that the introduction of subtyping in the wasm function-references and GC proposals exposed. It should not be triggerable without these features enabled. Neither of those proposals are enabled on any version yet though, so we should not need to uplift this.

Comment 4

[Ryan Hunt [:rhunt]]

From looking at this code with Ben, this also affects block params in addition to results.

Comment 5

[Ben Visness [:bvisness]]

Attachment: Bug 1833681: Add tests for wasm stack rewriting. r=rhunt

Depends on D178346

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86c7006980d3

Going off blame, it looks like this goes back to at least 107 (bug 1784276). As a sec-high affecting more than just Nightly, this should have gotten sec-approval prior to landing. Please request approval on that retroactively so we can assess the next steps.

Comment 7

[Ryan Hunt [:rhunt]]

This bug is only triggerable with the non-standard wasm-gc extension enabled. It's never been enabled by default on any version, including nightly.

My understanding is that this makes it less severe than the "B. The bug is a recent regression on mozilla-central" branch of approval process, which is why I recommended we land it without approval. Is sec-high appropriate for issues that would be sec-high if the feature was enabled, but is not enabled by default anywhere?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I'll redirect to Dan on that. But regardless, comment 2 asked for clarification before this landed as well.

Comment 9

[Andrew McCreight [:mccr8]]

If this requires some disabled feature, then it is okay to land without sec-approval. Comment 3 does mention that the feature it affects is disabled.

We give things security ratings as though they were enabled. Part of the idea here is that it helps keep it on people's radars so they don't forget about it before being enabled, but it can be a bit of a hassle for people who are working on projects that spend a long period being disabled.