1689834 - WebGPU crash - getMappedRange is not bounds checked

Status: RESOLVED FIXED

(Core :: Graphics: WebGPU, defect, P3)

Description

[SNCPlay42]

Attachment: repro document

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:84.0) Gecko/20100101 Firefox/84.0

Steps to reproduce:

Opened the attached html document

Actual results:

getMappedRange happily maps 10000 bytes of a 4 byte buffer.

The loop finds strings like "clnt_raw.c - Fatal header serialisation error." and "GLIBC_2.2.5", suggesting that arbitrary process memory is being read.

If I turn up N by a few more orders of magnitude the tab crashes.

Expected results:

Error because 10000 bytes exceeds the size of the buffer.

Comment 1

[SNCPlay42]

This resulted from me poking at https://github.com/gfx-rs/wgpu-rs/issues/735, which is publicly visible and could maybe hint people towards this, should that be deleted/hidden?

Then again, I don't know how serious a security issue in an opt-in nightly feature is.

Comment 2

[Dzmitry Malyshau [:kvark]]

Marking as S3 for not being exposed in Stable, and not being enabled in Nightly by default.

Comment 3

[Jim Mathies [:jimm]]

For sec folks - WebGPU won't roll out for about a year, and the code here is likely to change as we iterate on it. Currently WebGPU is behind a pref in Nightly and is disabled by default.

Comment 4

[Daniel Veditz [:dveditz]]

"stalled" was not a correct state: we were not floundering for a way to reproduce or diagnose the issue; we had a perfectly good testcase.

The testcase crashes for me still (bp-f59dbaa1-91f2-4335-a0fa-b60390220228) but it may not be the same problem.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jimb, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 6

[Jim Mathies [:jimm]]

Per comment 2, this is not in a shipping feature, and thus far has not been enabled in nightly.

Comment 7

[Jim Mathies [:jimm]]

sec related, adding an owner. WebGPU related, which isn't shipping.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

So this isn't just a problem of checking the range. The shmem could get released by a GPU process crash and we need to explicitly clear the array buffer reference to it when that happens. As well, when the Buffer object goes out of scope, we don't appear to actually call Buffer::Unmap anywhere, which is the only place that breaks the array buffer binding AFAICT. This means we currently rely upon JavaScript to explicitly call unmap to update the array buffer state.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

GPU process crash doesn't seem to be a concern. It is true that the protocol frees the Shmem mappings, but our copy of the Shmem struct should keep the buffer backed by SharedMemory alive.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1689834.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9276123 [details]
Bug 1689834.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: In theory very, it is a bounds checking and buffer lifetime issue. In practice difficult because WebGPU can only be enabled on nightly and it is still disabled by default. Asking for sec-approval because it is still marked as sec-high.
• 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?: None (disabled)
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• 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. I expanded the existing tests as well.
• Is Android affected?: Yes

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 9276123 [details]
Bug 1689834.

sec-approval = dveditz

It's fine to land the test with this since the feature is still disabled

Comment 13

[Sandor Molnar[:smolnar]]

Backed out changeset db25bd6ef77e (Bug 1689834) for causing build bustages in webgpu/Buffer.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/18280bb61bc3ac22633438366819a119c07b9a61

Push with failures

Failure log

 ^~~~~~~~~~~~~~~
[task 2022-05-17T00:47:09.231Z] 00:47:09     INFO -                        JS::Handle<JS::Value>
[task 2022-05-17T00:47:09.231Z] 00:47:09     INFO -  In file included from Unified_cpp_dom_webgpu0.cpp:29:
[task 2022-05-17T00:47:09.232Z] 00:47:09    ERROR -  /builds/worker/checkouts/gecko/dom/webgpu/Buffer.cpp:178:7: error: You must immediately return after calling this function
[task 2022-05-17T00:47:09.232Z] 00:47:09     INFO -        aRv.NoteJSContextException(aCx);

Comment 14

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

r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/743c221a039ebe7b9a4f9f6191d54aebb6725a41
https://hg.mozilla.org/mozilla-central/rev/743c221a039e