1812932 - heap-buffer-overflow in [@ nsPNGEncoder::ConvertHostARGBRow] webgpu buffer size mismatch

Status: VERIFIED FIXED

(Core :: Graphics: WebGPU, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 220230126-b9c4ba784620 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

==28211==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000241a50 at pc 0x7f23d3b05565 bp 0x7f23b68ef740 sp 0x7f23b68ef738
READ of size 4 at 0x611000241a50 thread T10
    #0 0x7f23d3b05564 in nsPNGEncoder::ConvertHostARGBRow(unsigned char const*, unsigned char*, unsigned int, bool) /builds/worker/checkouts/gecko/image/encoders/png/nsPNGEncoder.cpp:677:22
    #1 0x7f23d3b03168 in nsPNGEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsTSubstring<char16_t> const&) /builds/worker/checkouts/gecko/image/encoders/png/nsPNGEncoder.cpp:283:7
    #2 0x7f23d3b021e9 in nsPNGEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsTSubstring<char16_t> const&) /builds/worker/checkouts/gecko/image/encoders/png/nsPNGEncoder.cpp:71:8
    #3 0x7f23d40468ef in GetInputStream /builds/worker/checkouts/gecko/dom/base/ImageEncoder.cpp:286:17
    #4 0x7f23d40468ef in mozilla::dom::ImageEncoder::ExtractDataInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, unsigned char*, int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, bool, mozilla::layers::Image*, nsICanvasRenderingContextInternal*, mozilla::layers::CanvasRenderer*, nsIInputStream**, imgIEncoder*) /builds/worker/checkouts/gecko/dom/base/ImageEncoder.cpp:315:10
    #5 0x7f23d4072d81 in mozilla::dom::EncodingRunnable::ProcessImageData(unsigned long*, void**) /builds/worker/checkouts/gecko/dom/base/ImageEncoder.cpp:168:19
    #6 0x7f23d407257c in mozilla::dom::EncodingRunnable::Run() /builds/worker/checkouts/gecko/dom/base/ImageEncoder.cpp:191:19
    #7 0x7f23d0d3c356 in nsThreadPool::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadPool.cpp:313:14
    #8 0x7f23d0d2f0eb in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1191:16
    #9 0x7f23d0d38bc4 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:477:10
    #10 0x7f23d24bb664 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:300:20
    #11 0x7f23d2339537 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:381:10
    #12 0x7f23d2339537 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:374:3
    #13 0x7f23d2339537 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:356:3
    #14 0x7f23d0d26bc5 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:383:10
    #15 0x7f23f30d2628 in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #16 0x7f23f3635b42 in start_thread nptl/pthread_create.c:442:8
    #17 0x7f23f36c79ff  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230127094652-f75c73066b88.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: 9530a7bf5efae110ca73aa6b62133d0c6ad04c65 (20220128000428)
End: f75c73066b887c2379158c73c994b5ef95460238 (20230127094652)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 2

[Timothy Nikkel (:tnikkel)]

I can reproduce with a Windows asan build I downloaded. The values coming into the png encoder look very reasonable, so I'm guessing pointer to the buffer that is passed to the png encoder is the problem.

Comment 3

[Timothy Nikkel (:tnikkel)]

Here

https://searchfox.org/mozilla-central/rev/8e9b4484408154b80d7ede9e1b035819fda48fd2/dom/webgpu/CanvasContext.cpp#171

is where we create the buffer, we think it has size 52x1. But here

https://searchfox.org/mozilla-central/rev/8e9b4484408154b80d7ede9e1b035819fda48fd2/dom/canvas/CanvasRenderingContextHelper.cpp#123

where we take the image buffer and pass it along with a width and height of 300x150.

Comment 4

[Jim Blandy :jimb]

This may be a duplicate of bug 1813719. Could you see if it reproduces in a tree containing b08dc4ed5dce?

Comment 5

[Tyson Smith [:tsmith]]

The fuzzers are still reporting this but the attached test case does not trigger the issue. I'll reduce another test case.

Comment 6

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Updated test case. Reproduces with m-c 20230203-a356e2d3cf46.

Comment 7

[Timothy Nikkel (:tnikkel)]

I can reproduce with the new testcase, the only difference is the sizes are now 300x205 and 300x150.

Comment 8

[Jim Blandy :jimb]

I think there may be a lot of ways to get bugs that end up being detected by the PNG encoder. If fixing the current testcase doesn't stop the fuzzers from reporting this, let's close this and file a new bug. Jason, would that make trouble for the fuzzing infrastructure?

Comment 9

[Jason Kratzer [:jkratzer]]

No, that's not a problem. Do you want us to file a new issue using the testcase from comment 6?

Comment 10

[Jim Blandy :jimb]

For now things are fine as they are, but if we get another test case with this particular stack, let's file that as a new bug.

Comment 11

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

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

For more information, please visit auto_nag documentation.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Ok, so this bug is only in webgpu, and it's because CanvasRenderingContextHelper::ToBlob serializes bytes via GetImageBuffer, and then assumes that that array of bytes is sized based on the canvas width and height, which is not always true, but is presently true in Gecko except for for webgpu.

The right way to fix this is to stop duplicating this fiddly code in four places (one of which is broken), which I'm doing in bug 1818411.

Comment 13

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

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.

Comment 14

[Kelsey Gilbert [:jgilbert]]

It's sec-high if undisabled, but probably sec-other otherwise.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1812932 - Add size outvar to GetImageBuffer.

Comment 16

[Kelsey Gilbert [:jgilbert]]

@tsmith: Can you test this patch?
I can't build asan on windows due to bug 1757116.

Comment 17

[Tyson Smith [:tsmith]]

I am unable to reproduce the issue with the patch applied.

Comment 18

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

Landed: https://hg.mozilla.org/integration/autoland/rev/7657d29cd8728fa8fd4a90635ed0811f7d3a60c8

Backed out for causing bc failures in browser_view_image.js:
https://hg.mozilla.org/integration/autoland/rev/a0bdc7598e4631409e9796ad984f423069644c15

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=VWFjB4mUSn65tPgTtzqJLg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=7657d29cd8728fa8fd4a90635ed0811f7d3a60c8
Failure log: https://treeherder.mozilla.org/logviewer?job_id=408096377&repo=autoland

[task 2023-03-07T07:07:01.621Z] 07:07:01     INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_view_image.js | tab - checking if popup is closed - 
[task 2023-03-07T07:07:01.622Z] 07:07:01     INFO - tab - Popup Shown
[task 2023-03-07T07:07:01.623Z] 07:07:01     INFO - Console message: [JavaScript Error: "TypeError: URL.createObjectURL: Argument 1 is not valid for any of the 1-argument overloads." {file: "resource:///actors/ContextMenuChild.sys.mjs" line: 68}]
[task 2023-03-07T07:07:01.623Z] 07:07:01     INFO - receiveMessage/</<@resource:///actors/ContextMenuChild.sys.mjs:68:31
[task 2023-03-07T07:07:01.624Z] 07:07:01     INFO - 
[task 2023-03-07T07:07:01.624Z] 07:07:01     INFO - Buffered messages finished
[task 2023-03-07T07:07:01.625Z] 07:07:01     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/contextMenu/browser_view_image.js | Test timed out - 

Comment 19

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/autoland/rev/c7edb291d1ad2cb0ed62d36cdc5e104de151482b

Hasn't been backed out, but still waiting on the merge to central.

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/c7edb291d1ad

Comment 21

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230322211554-e38643d52e3d.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.