Closed Bug 1743767 (CVE-2022-31737) Opened 3 years ago Closed 2 years ago

heap-buffer-overflow in mozilla::gl::GLContext::raw_fReadPixels

Categories

(Core :: Graphics: CanvasWebGL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox-esr91 101+ fixed
firefox100 --- wontfix
firefox101 + fixed
firefox102 + fixed

People

(Reporter: attekett, Assigned: jgilbert)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main101+][adv-esr91.10+])

Attachments

(6 files, 1 obsolete file)

Tested on:
OS: Ubuntu 20.04 in VMware VM
Firefox:

fuzzfetch --target firefox --os Linux --asan --fuzzing -n firefox
[2021-12-01 10:14:05] Identified task: https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-fuzzing-asan-opt
[2021-12-01 10:14:05] > Task ID: Frxzk0u7Qeu4nG7rilMWAQ
[2021-12-01 10:14:05] > Rank: 1638335107
[2021-12-01 10:14:05] > Changeset: 89800efd9e5cfcf0146767961a63d5c4e2a86e2c
[2021-12-01 10:14:05] > Build ID: 20211201050507
[2021-12-01 10:14:05] > Downloading: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Frxzk0u7Qeu4nG7rilMWAQ/artifacts/public/build/target.tar.bz2 (382.95MiB total)

prefs.js: fuzzing prefs.js generated with prefpicker, adding as an attachment.

I have reproduced the issue on my machine with/without the fuzzing prefs.js, and with/without Xvfb . Including prefs.js as an attachment to ensure reproducibility.

The write size can be changed by changing the canvas width. For example changing width and 300px, the write will be size 1200, 10px and it will be 40. 1px on fuzzfetch --fuzzing build will result to a

Hit MOZ_CRASH(Trying to run a non-debug fuzzing build.) at /builds/worker/checkouts/gecko/third_party/rust/neqo-crypto/src/aead_fuzzing.rs:19

I don't think it is related to the issue. Using 1px with regular asan-build downloaded without --fuzzing doesn't crash, but other tested values 10px up reproduced sameway on regular and fuzzing builds.

Reproducing:

$ ffpuppet -p ~/prefs.js -u ./heap-buffer-overflow-mozilla-glGLContextraw_fReadPixels.html ./firefox/firefox --xvfb -d
[2021-12-01 10:37:01] Launching Firefox...
[2021-12-01 10:37:13] Running Firefox (pid: 16680)...
[2021-12-01 10:37:14] Shutting down...
[2021-12-01 10:37:16] Firefox process is closed. (Reason: <Reason.ALERT: 0>)
[2021-12-01 10:37:16] Displaying logs...
===
=== Dumping 'log_ffp_asan_16655.log.16807.txt' (14.89KB)
===
=================================================================
==16807==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f59218c8c00 at pc 0x55ee081ba203 bp 0x7fff0cc77200 sp 0x7fff0cc769c0
WRITE of size 1600 at 0x7f59218c8c00 thread T0 (file:// Content)
    #0 0x55ee081ba202 in __interceptor_memcpy /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:827:5

ASAN-trace(truncated, full trace as an attachment):

==16807==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f59218c8c00 at pc 0x55ee081ba203 bp 0x7fff0cc77200 sp 0x7fff0cc769c0
WRITE of size 1600 at 0x7f59218c8c00 thread T0 (file:// Content)
    #0 0x55ee081ba202 in __interceptor_memcpy /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:827:5
    #1 0x7f59333e3ef8  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x320ef8)
    #2 0x7f59332997fc  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x1d67fc)
    #3 0x7f59333e42dc  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x3212dc)
    #4 0x7f59333e4b15  (/usr/lib/x86_64-linux-gnu/dri/swrast_dri.so+0x321b15)
    #5 0x7f59c93ecd70 in mozilla::gl::GLContext::raw_fReadPixels(int, int, int, int, unsigned int, unsigned int, void*) /builds/worker/workspace/obj-build/dist/include/GLContext.h:1568:5
    #6 0x7f59cc103cb1 in mozilla::WebGLContext::DoReadPixelsAndConvert(mozilla::webgl::FormatInfo const*, mozilla::webgl::ReadPixelsDesc const&, unsigned long, unsigned long, unsigned int) /builds/worker/checkouts/gecko/dom/canvas/WebGLContextGL.cpp
    #7 0x7f59cc106190 in mozilla::WebGLContext::ReadPixelsImpl(mozilla::webgl::ReadPixelsDesc const&, unsigned long, unsigned long) /builds/worker/checkouts/gecko/dom/canvas/WebGLContextGL.cpp:1238:7
    #8 0x7f59cc1041a7 in mozilla::WebGLContext::ReadPixelsInto(mozilla::webgl::ReadPixelsDesc const&, mozilla::Range<unsigned char> const&) /builds/worker/checkouts/gecko/dom/canvas/WebGLContextGL.cpp:962:10
    #9 0x7f59cbfe89b4 in mozilla::HostWebGLContext::ReadPixelsInto(mozilla::webgl::ReadPixelsDesc const&, mozilla::Range<unsigned char> const&) const /builds/worker/checkouts/gecko/dom/canvas/HostWebGLContext.h:646:22
...
    #53 0x55ee08252dfd in content_process_main(mozilla::Bootstrap*, int, char**) /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #54 0x55ee08253228 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:327:18
    #55 0x7f59e54b50b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #56 0x55ee081a1ec9 in _start (/firefox/firefox+0x5cec9)

0x7f59218c8c00 is located 0 bytes to the right of 640000-byte region [0x7f592182c800,0x7f59218c8c00)
allocated by thread T0 (file:// Content) here:
    #0 0x55ee0821e632 in __interceptor_calloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:138:3
    #1 0x7f59d39151cb in AllocateArrayBufferContents /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.cpp:459:11
    #2 0x7f59d39151cb in std::tuple<js::ArrayBufferObject*, unsigned char*> js::ArrayBufferObject::createBufferAndData<(js::ArrayBufferObject::FillContents)0>(JSContext*, unsigned long, js::AutoSetNewObjectMetadata&, JS::Handle<JSObject*>) /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.cpp:1351:18
    #3 0x7f59d390ceea in js::ArrayBufferObject::createZeroed(JSContext*, unsigned long, JS::Handle<JSObject*>) /builds/worker/checkouts/gecko/js/src/vm/ArrayBufferObject.cpp:1414:7
...
    #22 0x7f59ce5744aa in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/checkouts/gecko/dom/script/ScriptElement.cpp:118:18
    #23 0x7f59c91a2d6e in nsIScriptElement::AttemptToExecute() /builds/worker/workspace/obj-build/dist/include/nsIScriptElement.h:211:18

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:827:5 in __interceptor_memcpy
Shadow bytes around the buggy address:
...
==16807==ABORTING

Attached file prefs.js

Attaching the prefs.js used to reproduce, if prefpicker generated doesn't reproduce clearly.

Full AddressSanitizer output.

Group: firefox-core-security → gfx-core-security
Component: General → Canvas: WebGL
Product: Firefox → Core

The prefs include webgl.enable-draft-extensions and webgl.enable-privileged-extensions, so maybe these aren't standard web-exposed APIs being used. (I haven't looked at the test case.)

The issue reproduces without usage of the prefs.js. Also I tested that it reproduces with the mentioned webgl.enable-draft-extensions and webgl.enable-privileged-extensions set to false, from the attached prefs.js.

Assignee: nobody → jgilbert
Flags: sec-bounty?

So the y=-1 is valid, but we're actually writing too far, rather than too early.
Our code to handle this is super wrong, and just resets to row0 but doesn't decrease the amount it reads, so it tries to read 400x400px starting at a 100x1px offset into the destination buffer, which gladly writes off the end of that buffer.

We need better ranged types here. We cast Range into uintptr_t too early, but unfortunately that's because we need to handle both the void* and uintptr_t-into-pbo usecase.
Sending a void* that doesn't point to valid memory into glReadPixels is actually super against the rules in c++ (pointers must point within a valid range of their types, with one element of slack on either end for end/reverse-end), so we try to minimize the amount of code we have that treats this pbo offset as a void*.
We could go in-for-a-penny-in-for-a-pound and use a Range<uint8_t>[0,pbo->ByteSize()], but that seems like tempting fate.

Instead I'm making a RangedInt<T> struct with CheckedInt<T>, that we can use as RangedInt<uintptr_t>.
Writing this code, it actually makes some of the bounds checks much easier, so win-win.

The severity field is not set for this bug.
:jgilbert, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jgilbert)
Severity: -- → S3
Flags: needinfo?(jgilbert)
Priority: -- → P1

Please retest now that we've landed bug 1750310.

Flags: needinfo?(attekett)

Still reproduces.

From mozilla-central log, it looks like the patch from bug #1750310 is included in the version that fuzzfetch downloads.


:$ fuzzfetch --target firefox --central --os Linux --asan --fuzzing -n firefox
[2022-02-24 23:42:17] Identified task: https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-fuzzing-asan-opt
[2022-02-24 23:42:17] > Task ID: OuiYrcUqRpyXyRqT52NfQQ
[2022-02-24 23:42:17] > Rank: 1645739161
[2022-02-24 23:42:17] > Changeset: fb443d9a5f9cfaa17acc81c25473d7093d5cf696
[2022-02-24 23:42:17] > Build ID: 20220224214601
[2022-02-24 23:42:17] > Downloading: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/OuiYrcUqRpyXyRqT52NfQQ/artifacts/public/build/target.tar.bz2 (436.42MiB total)
[2022-02-24 23:42:31] .. downloaded (32.73MB/s)
[2022-02-24 23:42:31] .. extracting
[2022-02-24 23:46:07] Extracted into /firefox

Reproducing:

:$ ffpuppet ./firefox/firefox -u ./heap-buffer-overflow-mozilla-glGLContextraw_fReadPixels.html -d
[2022-02-24 23:50:10] Launching Firefox...
[2022-02-24 23:50:23] Running Firefox (pid: 3217697)...
[2022-02-24 23:50:25] Shutting down...
[2022-02-24 23:50:28] Firefox process is closed. (Reason: ALERT)
[2022-02-24 23:50:28] Displaying logs...
===
=== Dumping 'log_ffp_asan_3217695.log.3217783.txt' (16.62KB)
===
=================================================================
==3217783==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fe68cffac00 at pc 0x55a99b0c1c13 bp 0x7ffe42bd5ab0 sp 0x7ffe42bd5270
WRITE of size 1600 at 0x7fe68cffac00 thread T0 (file:// Content)
    #0 0x55a99b0c1c12 in __interceptor_memcpy /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:827:5
    #1 0x7fe694dfa758  (/usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so+0x39a758)
    #2 0x7fe694cb44cb  (/usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so+0x2544cb)
    #3 0x7fe694dfab34  (/usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so+0x39ab34)
    #4 0x7fe694dfb385  (/usr/lib/x86_64-linux-gnu/dri/vmwgfx_dri.so+0x39b385)
    #5 0x7fe6af901950 in mozilla::gl::GLContext::raw_fReadPixels(int, int, int, int, unsigned int, unsigned int, void*) /builds/worker/workspace/obj-build/dist/include/GLContext.h:1568:5
    #6 0x7fe6b28f1953 in mozilla::WebGLContext::DoReadPixelsAndConvert(mozilla::webgl::FormatInfo const*, mozilla::webgl::ReadPixelsDesc const&, unsigned long, unsigned long, unsigned int) /builds/worker/checkouts/gecko/dom/canvas/WebGLContextGL.cpp
...
Flags: needinfo?(attekett)

Thanks!

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jgilbert)

S2 is probably right for this, though it's primarily on Mesa it seems?

Severity: S3 → S2
Flags: needinfo?(jgilbert)

Well it doesn't repro on this system anymore. I wonder if this is a different machine than the one I investigated it on originally?

Comment on attachment 9274154 [details]
Bug 1743767 - Tighten bounds and add asserts for row-by-row ReadPixels.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Tricky but not impossible.
    I'm definitely adding asserts in a patch to a bug that's restricted-access, though I don't say exactly what the problem is. I don't know how to answer "is this a bullseye on the security problem", so I said "no", because there's still some thinking required of any adversary looking at this.
  • 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?: all
  • 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?: Should be pretty easy.
  • How likely is this patch to cause regressions; how much testing does it need?: unlikely if it gets through CI
  • Is Android affected?: Yes
Attachment #9274154 - Flags: sec-approval?

Comment on attachment 9274154 [details]
Bug 1743767 - Tighten bounds and add asserts for row-by-row ReadPixels.

Let's land and uplift this one mid-next week, on May 11.

Attachment #9274154 - Flags: sec-approval?
Attachment #9274154 - Flags: sec-approval+
Attachment #9274154 - Flags: approval-mozilla-esr91+
Attachment #9274154 - Flags: approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

Comment on attachment 9274154 [details]
Bug 1743767 - Tighten bounds and add asserts for row-by-row ReadPixels.

This needs a rebased patch for ESR91.

Flags: needinfo?(jgilbert)
Attachment #9274154 - Flags: approval-mozilla-esr91+

I can't build esr91 on Windows or Mac, and I can't seem to push it to Try either, so here's the patch I guess? I hope it builds!

Flags: needinfo?(ryanvm)

Comment on attachment 9276352 [details]
[esr91 backport] 0001-Bug-1743767-esr91-Tighten-bounds-and-add-asserts-for.patch

Busted on Try:
https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=d65aef008c21ee597b78aea149d1bc8d53ad89c2

FWIW, I find for ESR, the easiest way to get a Try push going is to use |./mach try empty| and add jobs to the push via TH.

Attachment #9276352 - Attachment is obsolete: true
Flags: needinfo?(ryanvm) → needinfo?(jgilbert)

Thanks! I'm trying that.

Comment on attachment 9276496 [details] [diff] [review]
0001-Bug-1743767-esr91-Tighten-bounds-and-add-asserts-for.patch

Approved for 91.10esr. Thanks for the quick rebase!

Flags: needinfo?(ryanvm)
Attachment #9276496 - Flags: approval-mozilla-esr91+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main101+]
Attached file advisory.txt
Whiteboard: [post-critsmash-triage][adv-main101+] → [post-critsmash-triage][adv-main101+][adv-esr91.10+]
Alias: CVE-2022-31737
Flags: sec-bounty? → sec-bounty+

Hit MOZ_CRASH(Trying to run a non-debug fuzzing build.) at /builds/worker/checkouts/gecko/third_party/rust/neqo-crypto/src/aead_fuzzing.rs:19

This is bug 1743672, for reference.

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: