Crash in Wasm Ion code when gczeal is used with reference types and builtin instance method calls
Categories
(Core :: JavaScript: WebAssembly, defect, P1)
Tracking
()
People
(Reporter: asumu, Assigned: asumu)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-uaf, reporter-external, sec-high, Whiteboard: [sec-survey][adv-main95+][adv-ESR91.4.0+])
Attachments
(3 files, 2 obsolete files)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr91+
tjr
:
sec-approval+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
334 bytes,
text/plain
|
Details |
The following Wasm jit-test test case triggers a crash in Ion:
// crash.js
gczeal(2, 1); // Collect on every allocation.
let exports = wasmEvalText(
`(module
(table $tab (export "tab") 5 externref)
(elem declare funcref (ref.func $g))
(func $g)
(func $f (export "f") (param externref) (result)
(ref.func $g) ;; force a collection via allocation in instance call
;; stack map is not at the right offset as failure mode is FailOnInvalidRef
(ref.is_null)
(if
(then)
;; crashes here due to GC happening above
;; at this point the externref on stack is a poisoned value
(else (i32.const 0) (local.get 0) (table.set $tab)))
)
)`,
{}
).exports;
exports.f("foo");
Running this produces a crash like the following
$ mach jit-test --ion crash.js
Assertion failure: ThreadId::ThisThreadId() == owningThread_, at /home/asumu/mozilla-unified/js/src/threading/Mutex.cpp:63
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==28940==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x557b685d175c bp 0x7ffec0503ba0 sp 0x7ffec0503b70 T28940)
==28940==The signal is caused by a WRITE memory access.
==28940==Hint: address points to the zero page.
This is reduced from another test case (in exception handling Ion code) where it was more obvious that the externref
parameter to the function $f
is getting collected and overwritten with a poisoned value, but you can see in gdb that the externref is overwritten with 0x2F
(JS_FRESH_NURSERY_PATTERN
) if you break in the tableSet
method.
What appears to be happening is that stack maps for Ion calls to Wasm instance method calls are not being installed correctly (at the right offset), so they are not actually looked up during GC. The GC then misses roots in the stack frame. This happens on methods where the failure mode is not Infallible
, as there are some error handling instructions inserted between the call return point and the location of the LSafepoint
.
I'll attach a patch that should fix the offsets later.
I think it is difficult to trigger this bug without gczeal turned on, but I've marked this as a security bug just in case.
Assignee | ||
Comment 1•3 years ago
|
||
Note: this particular test case seems to only crash on debug builds, and observing the memory poisoning also requires having debug on so it's difficult to tell if something bad is happening on non-debug builds.
Assignee | ||
Comment 3•3 years ago
|
||
Assignee | ||
Comment 4•3 years ago
|
||
Depends on D130527
Assignee | ||
Updated•3 years ago
|
Comment 5•3 years ago
|
||
It sounds like a GC rooting issue, so I'm going to mark it sec-high.
Comment 6•3 years ago
|
||
I can't tell yet if this is some corner case in not-yet-shipped code or a real emergency, will discuss with Ryan later today.
Comment 7•3 years ago
•
|
||
Analysis:
- this is about wasm builtin calls only (other calls are ok)
- this is about ion only (baseline is ok)
- this is specific to builtin calls that can return an error and can trigger gc, probably only ref.func
- a pointer on the stack is not traced because the stackmap is not found by the gc because its location marker is off, if the correct phase of gc is triggered by the allocation in question
- an attacker could perhaps place many pointers on the stack in an attempt to get one that points to something interesting, but this will be hard to count on and will easily crash
- fission helps us (attack does not escape to other domains) but also hurts us (gc behavior may be less turbulent than it would be in the single-process browser)
- the issue is serious, we do uplifts all the way
Updated•3 years ago
|
Comment 8•3 years ago
|
||
Obviously not asking for a bounty for myself, but for Asumu, if eligible. This is an important bug.
Comment 9•3 years ago
|
||
Comment on attachment 9249520 [details]
Bug 1739683 - Fix code offset for safepoints on Wasm instance method calls
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Unsure, but my guess is not easily. The patch changes how we record offsets for stack maps for wasm calls so it could lead someone to suspect that certain calls could lead to the wrong stack map. From there they would have to figure out that it's only certain kinds, specifically just ref.func to our knowledge (see Lars' post). From there they would have to manage the heap non-determinism, as Lars again points out, to trigger a GC in the ref.func call that will collect an object they control on the stack and then allocate an object into the same pointer and then use it somehow.
- 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?: All
- 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?: Unlikely, we have good coverage of many stack map possible issues.
Comment 10•3 years ago
|
||
As marked in the security approval request, I think we should change the commit message to not mention safepoints nor builtin instance methods.
Updated•3 years ago
|
Assignee | ||
Comment 11•3 years ago
|
||
I retitled the patch given the above comments to not mention the details. Happy to adjust it again if needed.
Comment 12•3 years ago
|
||
Comment on attachment 9249520 [details]
Bug 1739683 - Fix code offset for safepoints on Wasm instance method calls
Approved to land and request uplift
Updated•3 years ago
|
Comment 13•3 years ago
|
||
Fix code offset use in Wasm calls r=rhunt,lth
https://hg.mozilla.org/integration/autoland/rev/a972b4f1f667f9b5945d74ad351b429b6c5be9b7
https://hg.mozilla.org/mozilla-central/rev/a972b4f1f667
Comment 14•3 years ago
|
||
Please nominate this patch for Beta and ESR91 approval when you get a chance.
Comment 15•3 years ago
|
||
Comment on attachment 9249520 [details]
Bug 1739683 - Fix code offset for safepoints on Wasm instance method calls
Beta/Release Uplift Approval Request
- User impact if declined: Potentially exploitable GC rooting hazard due to stack maps issue.
- Is this code covered by automated tests?: No
- 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): Not risky, we have good coverage of many stack map possible issues.
- String changes made/needed:
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration:
- User impact if declined: Potentially exploitable GC rooting hazard due to stack maps issue.
- Fix Landed on Version: 96
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Not risky, we have good coverage of many stack map possible issues.
- String or UUID changes made by this patch:
Comment 16•3 years ago
|
||
Comment on attachment 9249520 [details]
Bug 1739683 - Fix code offset for safepoints on Wasm instance method calls
Approved for 95.0b9.
Comment 17•3 years ago
|
||
uplift |
Comment 18•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Comment on attachment 9249520 [details]
Bug 1739683 - Fix code offset for safepoints on Wasm instance method calls
Approved for 91.4esr.
Comment 20•3 years ago
|
||
uplift |
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 21•3 years ago
|
||
My understanding of wasm and wasm instance methods is... limited. The advisory draft is addressing this by being very vague. It's generally OK if it's "vague but not wrong", but, can you review the advisory to make sure I'm not saying something incorrect or stupid? :)
Comment 22•3 years ago
|
||
Hi Frederik, I think the advisory is probably misleading as it stands. A title like "GC rooting failure" with explanation "Failure to correctly record the location of live pointers across wasm instance calls resulted in a GC occuring within the call not tracing those live pointers. This could cause some live objects to be collected and the caller could later reference unused or reused memory. " would be comprehensive.
Comment 23•3 years ago
|
||
Thanks, Lars!
I've added the relevant feature to the title and replaced your last sentence with our template sentence of "This can lead to a user-after-free causing a potentially exploitable crash."
Comment 24•3 years ago
|
||
That sounds good, though (nitpicking) I think the preferred spelling is "use-after-free"?
Updated•3 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•1 year ago
|
Comment 26•1 year ago
|
||
Comment 27•1 year ago
|
||
bugherder |
Updated•8 months ago
|
Description
•