Closed Bug 1689834 Opened 4 years ago Closed 3 years ago

WebGPU crash - getMappedRange is not bounds checked

Categories

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

Firefox 87
defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- disabled
firefox87 --- disabled
firefox88 --- disabled
firefox89 --- disabled
firefox90 --- disabled
firefox98 --- disabled
firefox99 --- disabled
firefox100 --- disabled
firefox101 --- disabled
firefox102 --- fixed

People

(Reporter: SNCPlay42, Assigned: aosmond)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [reporter-external][post-critsmash-triage])

Attachments

(2 files)

Attached file repro document (deleted) —

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.

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.

Group: core-security → gfx-core-security

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

Severity: -- → S3
Summary: getMappedRange is not bounds checked → WebGPU crash - getMappedRange is not bounds checked

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Keywords: stalled

"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.

Keywords: stalled

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.

Flags: needinfo?(jimb)

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

Flags: needinfo?(jimb)

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

Assignee: nobody → jimb

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.

Assignee: jimb → aosmond

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.

Attached file Bug 1689834. (deleted) —

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
Attachment #9276123 - Flags: sec-approval?

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

Attachment #9276123 - Flags: sec-approval? → sec-approval+

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);
Flags: needinfo?(aosmond)
Flags: needinfo?(aosmond)
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
Flags: in-testsuite+
Regressions: 1769788
Flags: qe-verify+
Whiteboard: [reporter-external] → [reporter-external][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: