1762441 - Stackmaps are not serialized/deserialized with WebAssembly caching

Status: RESOLVED FIXED

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

Description

[Ryan Hunt [:rhunt]]

Attachment: caching.js

I have been rewriting our module serialization/deserialization code as it has had security issues before and is difficult to maintain.

During this process I discovered that we are not serializing or deserializing stack maps [1] [2]. This mean that no tracing of reference values in wasm stack frames will be done when a module is deserialized from the cache.

I've attached a test case for this.

// |jit-test| skip-if: !wasmCachingEnabled()
load(libdir + "wasm-caching.js");

function alloc() {
	return {};
}

testCached(`(module
	(func (import "" "alloc") (result externref))
	(func (import "" "gc"))
	(func (export "r") (result externref)
		(local externref)
                (; store an object in our frame, it exists nowhere else ;)
		(local.set 0 call 0)
                (; trigger a gc ;)
		call 1
                (; return the object, which points to freed memory ;)
		local.get 0
	)
)`, { "": { alloc, gc } }, i => {
        // Get a pointer to freed memory
	let obj = i.exports.r();
        // Write to it
	for (let i = 0; i < 1000; i++) {
		obj[i] = 0xff;
	}
});

This is exposed to web content when they use compileStreaming with a cacheable fetch. It's controlled by the javascript.options.wasm_caching pref. This is enabled in 100, 99, and 98.

We should disable this pref for release and beta. Nightly might depend on if we believe it's fixable in a reasonable way quickly.

[1] https://searchfox.org/mozilla-central/rev/625c3d0c8ae46502aed83f33bd530cb93e926e9f/js/src/wasm/WasmCode.h#467
[2] https://searchfox.org/mozilla-central/rev/625c3d0c8ae46502aed83f33bd530cb93e926e9f/js/src/wasm/WasmCode.cpp#561

Comment 1

[Lars T Hansen [:lth]]

Good (but frightening) find. For now I agree it's OK just to disable everywhere (including Nightly). When we fix this we should worry about stack map encoding a little bit; the volume may be large and the encoding is not necessarily very dense. At a minimum we need to consider whether it's OK to ship a simple fix now and then go back and implement a better encoding later, or if we need to implement something proper right away.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1762441 - wasm: Disable code caching. r?yury

Comment 3

[Ryan Hunt [:rhunt]]

Marking this as sec-critical as this is a very easy to trigger GC hazard which would lead to UaF of arbitrary JS values. I'm not an expert in exploits though, so happy to downgrade if this doesn't meet the mark.

Comment 4

[Ryan Hunt [:rhunt]]

I should note that we have not gotten any reports of this triggering in the wild, nor being exploited. It's possible the combination of features required, wasm_caching and wasm_reftypes are used infrequently enough that it has not been stumbled upon. Once they are used together though, it's very easy to find. wasm_caching shipped in 98 too, so that's recent.

Comment 5

[Ryan Hunt [:rhunt]]

We are having discussions with release managers about whether this is worth a late RC respin vs. riding along a dot release. We would likely want to land this under a cover bug that would point the blame at "stability issues" or something like it. There's also a chance we could use Normandy to do the pref-flip, as it should be a live pref that doesn't require a restart.

Comment 6

[Ryan Hunt [:rhunt]]

Comment on attachment 9270286 [details]
Bug 1762441 - wasm: Disable code caching. r?yury

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Likely not easily, but is suspicious. We would likely want to land this under a cover bug that would point the blame at "stability issues" or something like it.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: Fx98, Fx99, Fx100
• If not all supported branches, which bug introduced the flaw?: 1737405 (turned on the feature which is vulnerable)
• 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?: Unlikely. Disabling caching will just mean that we re-compile more frequently. The perf hit is likely acceptable because most pages don't use caching yet.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan Hunt [:rhunt] from comment #5)

There's also a chance we could use Normandy to do the pref-flip, as it should be a live pref that doesn't require a restart.

I like this option, but the drawback is that we'd only be able to do so for desktop builds. One advantage this gives us is the ability to target users on 98, though.

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 9270286 [details]
Bug 1762441 - wasm: Disable code caching. r?yury

A cover bug or normandy sounds fine to me. There's really no indication what in this feature is concerning, so as long as we're not landing it obtrusively and letting it sit for a week I don't have concerns.

Comment 9

[Donal Meehan [:dmeehan]]

Tom, do you think this is ok to follow-up in a later release for 99?
Trying to get a handle if this is time sensitive and we need an RC respin or if you're ok with it being in a dot release?

Comment 10

[Tom Ritter [:tjr]]

I think it's probably fine as a dot release. I can't tell how long this bug has existed (it seems like it would cause crashes if triggered accidentally, and we apparently haven't had accidental crashes from it before) - but it seems like it's sat undiscovered for a bit, so letting it sit a bit longer should be okay.

Comment 11

[Donal Meehan [:dmeehan]]

Thanks for (In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #10)

I think it's probably fine as a dot release. I can't tell how long this bug has existed (it seems like it would cause crashes if triggered accidentally, and we apparently haven't had accidental crashes from it before) - but it seems like it's sat undiscovered for a bit, so letting it sit a bit longer should be okay.

Thanks for this input

Comment 12

[Ryan Hunt [:rhunt]]

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #10)

I think it's probably fine as a dot release. I can't tell how long this bug has existed (it seems like it would cause crashes if triggered accidentally, and we apparently haven't had accidental crashes from it before) - but it seems like it's sat undiscovered for a bit, so letting it sit a bit longer should be okay.

The vulnerable code has existed for a while, but it wasn't exposed until wasm code caching was enabled in Fx98. And as noted above, it requires both the wasm code caching and wasm reftypes features to be used. And we have evidence reftypes is used almost nowhere at this point. So the lack of reports may be due to this, as once both are used it would very quickly lead to crashes.

So it's possible that someone has discovered it, but it has been a whole release cycle, so I'd guess that another week likely won't hurt in my opinion.

Comment 13

[Yury Delendik (:yury)]

Attachment: Instrumentation for jit-tests to detect failed serialization

Attaching WIP instrumentation to detect failing serialization cases for existing wasm jit-tests. Currently failing:

wasm/exceptions/bug-1744663-extended.js
wasm/exceptions/reftypes.js
wasm/multi-value/call-ref.js
wasm/ref-types/externref.js
wasm/ref-types/stackmaps1.js
wasm/ref-types/stackmaps2.js
wasm/ref-types/stackmaps3.js
wasm/ref-types/stackmaps4-params-n-locals.js
wasm/bigint/stubs.js

Most of them reftypes/stackmaps related.

Comment 14

[Ryan Hunt [:rhunt]]

The patch to disable the code caching pref has landed in bug 1762619 for Firefox 100. Firefox 99 is now released, and we've requested release approval to ride along in any dot release that happens for 99.

In addition to that, we're doing a normandy rollout to disable the pref for Fx99 (and Fx98 stragglers) [1]. This won't affect mobile, but should help fill the gaps. The rollout is live now, and I was able to confirm it on my MacBook.

The WebAssembly team met and decided that we don't want to try to uplift a fix that could re-enable wasm code caching for Fx100 beta. We're planning on re-enabling code caching with new serialization/deserialization code in Fx101. This will likely happen in a separate bug that hasn't been filed yet.

I don't think anything else needs to be done in this bug, so I'm going to mark it as resolved to reflect that fixes have landed.

[1] https://experimenter.services.mozilla.com/experiments/disable-webassembly-code-caching/

Comment 15

[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 16

[Ryan VanderMeulen [:RyanVM]]

If we're not intending to fix the root cause in this bug, please set a dependency with the bug where that work is happening.