WebGPU crash - getMappedRange is not bounds checked
Categories
(Core :: Graphics: WebGPU, defect, P3)
Tracking
()
People
(Reporter: SNCPlay42, Assigned: aosmond)
References
Details
(Keywords: csectype-bounds, sec-high, Whiteboard: [reporter-external][post-critsmash-triage])
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-phabricator-request
|
dveditz
:
sec-approval+
|
Details |
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.
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Marking as S3 for not being exposed in Stable, and not being enabled in Nightly by default.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 3•4 years ago
|
||
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.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•3 years ago
|
Comment 4•3 years ago
|
||
"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•3 years ago
|
||
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•3 years ago
|
||
Per comment 2, this is not in a shipping feature, and thus far has not been enabled in nightly.
Comment 7•3 years ago
|
||
sec related, adding an owner. WebGPU related, which isn't shipping.
Assignee | ||
Comment 8•3 years ago
|
||
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 | ||
Comment 9•3 years ago
|
||
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.
Assignee | ||
Comment 10•3 years ago
|
||
Assignee | ||
Comment 11•3 years ago
|
||
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
Updated•3 years ago
|
Comment 12•3 years ago
|
||
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•3 years ago
|
||
Backed out changeset db25bd6ef77e (Bug 1689834) for causing build bustages in webgpu/Buffer.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/18280bb61bc3ac22633438366819a119c07b9a61
^~~~~~~~~~~~~~~
[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);
Assignee | ||
Updated•3 years ago
|
Comment 14•3 years ago
|
||
r=gfx-reviewers,lsalzman
https://hg.mozilla.org/integration/autoland/rev/743c221a039ebe7b9a4f9f6191d54aebb6725a41
https://hg.mozilla.org/mozilla-central/rev/743c221a039e
Updated•3 years ago
|
Updated•3 years ago
|
Updated•2 years ago
|
Description
•