1587050 - table.copy on reftypes fails to verify that source and destination tables have compatible types

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

Clearly readMemOrTableCopy in WasmOpIter.h needs to verify that the types are compatible, but even if that's fixed I think there's a bug in Table::copy in that it dispatches on the destination table's kind and assumes the source table is of the same type. With a sufficiently interesting test case this will assert in a debug build, which is how I found it.

How serious is this?

Wasm reference types are enabled only in Nightly, so it's definitely Nightly-only.

The spec's not done, though I think Chrome ships in Canary and JSC may have something pre-release too, so there's awareness that the thing exists. However it's exceedingly unlikely that real content is affected in any way.

An attacker might be able to craft something so that an anyref is confused with a funcref or either is confused with garbage. In principle the anyref might point to user-controllable data. It is thus remotely conceivable that somebody could use this to craft bytes that represent executable code, though it would be in non-executable memory; however those bytes might point into executable memory. ASLR is a thing but can be defeated, etc. In short, we should fix this :)

Comment 1

[Lars T Hansen [:lth]]

Attachment: Bug 1587050 - Adjust table.copy part 1. r=jseward

Comment 2

[Lars T Hansen [:lth]]

Attachment: Bug 1587050 - Adjust table.copy part 2. r=jseward

Depends on D48536

Comment 3

[Lars T Hansen [:lth]]

Julian, a couple notes. I split test and code apart since we may have to land the code alone, test cases revealing too much. No commit msgs for the same reason, and I could also remove comments from the code. And the test cases are a little more comprehensive than necessary, I added tests for more table operators.

Comment 4

[Lars T Hansen [:lth]]

Somewhat arguably, this "regressed" with the introduction of generalized table operators:

# HG changeset patch
# User Luke Wagner <[e-mail]>
# Date 1557524986 18000
#      Fri May 10 16:49:46 2019 -0500
# Node ID dbd86e905a1d34120404798b7b89a97211f7c99b
# Parent  357a1b6e657c680d015d8b579ef82346f480a1ec
Bug 1546138 - Allow funcref in table operators (r=lth)

although I believe we've had both table-of-anyref longer than that, as well as table.copy, so the bug may well be older. Still Nightly-only this entire time.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 9099553 [details]
Bug 1587050 - Adjust table.copy part 2. r=jseward

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Somebody who knows the code well can probably see what's going on, but creating an exploit is probably tricky.
• 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?: None - Nightly only
• If not all supported branches, which bug introduced the flaw?: Unknown - it's been around for a while
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: No reason to create them - Nightly only
• How likely is this patch to cause regressions; how much testing does it need?: It has unit tests, which are in the accompanying patch, they can land separately if necessary, but I would prefer not to wait.

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Assigning sec-high; because this is Nightly only; you don't need sec-approval; so rubber stamp sec-approval+.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Tom Ritter [:tjr] (needinfo for responses to sec-[approval/ratings/advisories]) from comment #6)

because this is Nightly only; you don't need sec-approval; so rubber stamp sec-approval+.

Not what the guidelines I found say, I quote: "Core-security bug fixes can be landed by a developer without any explicit approval if ... The bug is a recent regression on mozilla-central") and this is not a "recent regression". https://wiki.mozilla.org/Security/Bug_Approval_Process.

But (a) there are some cases no law can be framed to cover and (b) I'm cool with that and (c) I'm happy to get this landed :-)

Comment 8

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

https://hg.mozilla.org/integration/autoland/rev/83d82f306f89326f6a9c81702780cca0bde80f90
https://hg.mozilla.org/integration/autoland/rev/f8479467f708eae59cd544baecf59705e8971077

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/83d82f306f89
https://hg.mozilla.org/mozilla-central/rev/f8479467f708