Closed Bug 1587050 Opened 10 months ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: lth, Assigned: lth)

Details

(Keywords: sec-high)

Attachments

(2 files)

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 :)

Depends on D48536

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.

Attachment #9099552 - Attachment description: Bug 1587050 - Adjust table.copy part 1. r?jseward → Bug 1587050 - Adjust table.copy part 1. r=jseward
Attachment #9099553 - Attachment description: Bug 1587050 - Adjust table.copy part 2. r?jseward → Bug 1587050 - Adjust table.copy part 2. r=jseward

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

# HG changeset patch
# User Luke Wagner <luke@mozilla.com>
# 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 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.
Attachment #9099553 - Flags: sec-approval?
Attachment #9099552 - Flags: sec-approval?

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

Group: core-security → javascript-core-security
Keywords: sec-high
Keywords: checkin-needed

(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 :-)

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.