1850983 - AddressSanitizer: heap-buffer-overflow [@ mozilla::gfx::ComputesRGBLuminanceMask] with READ of size 1

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Schwartzentruber (:truber)]

The attached testcase crashes on mozilla-central revision 20230830-c4e74daae186 (build with --enable-fuzzing & moz2d target patch).

==2221572==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6290006f327f at pc 0x7f031c6de3d0 bp 0x7fffc6c7f100 sp 0x7fffc6c7f0f8
READ of size 1 at 0x6290006f327f thread T0
SCARINESS: 12 (1-byte-read-heap-buffer-overflow)
    #0 0x7f031c6de3cf in mozilla::gfx::ComputesRGBLuminanceMask(unsigned char const*, int, unsigned char*, int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, float) /home/truber/src/m/u/gfx/2d/DrawTarget.cpp:84:19
    #1 0x7f031c6dddad in mozilla::gfx::DrawTarget::IntoLuminanceSource(mozilla::gfx::LuminanceType, float) /home/truber/src/m/u/gfx/2d/DrawTarget.cpp:237:7
    #2 0x7f031c702bd5 in mozilla::gfx::DrawTargetOffset::IntoLuminanceSource(mozilla::gfx::LuminanceType, float) /home/truber/src/m/u/gfx/2d/DrawTargetOffset.cpp:184:20
    #3 0x7f031c702bd5 in mozilla::gfx::DrawTargetOffset::IntoLuminanceSource(mozilla::gfx::LuminanceType, float) /home/truber/src/m/u/gfx/2d/DrawTargetOffset.cpp:184:20
    #4 0x7f031c613142 in mozilla::gfx::RecordedIntoLuminanceSource::PlayEvent(mozilla::gfx::Translator*) const /home/truber/src/m/u/gfx/2d/RecordedEventImpl.h:3472:35
    #5 0x7f031c6037cc in mozilla::gfx::InlineTranslator::TranslateRecording(char*, unsigned long)::$_0::operator()(mozilla::gfx::RecordedEvent*) const /home/truber/src/m/u/gfx/2d/InlineTranslator.cpp:78:31
    #6 0x7f031c603576 in std::_Function_handler<bool (mozilla::gfx::RecordedEvent*), mozilla::gfx::InlineTranslator::TranslateRecording(char*, unsigned long)::$_0>::_M_invoke(std::_Any_data const&, mozilla::gfx::RecordedEvent*&&) /home/truber/.mozbuild/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/std_function.h:282:9
    #7 0x7f031c653130 in std::function<bool (mozilla::gfx::RecordedEvent*)>::operator()(mozilla::gfx::RecordedEvent*) const /home/truber/.mozbuild/sysroot-x86_64-linux-gnu/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/std_function.h:687:14
    #8 0x7f031c5f7ee6 in bool mozilla::gfx::RecordedEvent::DoWithEvent<mozilla::gfx::InlineTranslator::TranslateRecording(char*, unsigned long)::MemReader>(mozilla::gfx::InlineTranslator::TranslateRecording(char*, unsigned long)::MemReader&, mozilla::gfx::RecordedEvent::EventType, std::function<bool (mozilla::gfx::RecordedEvent*)> const&) /home/truber/src/m/u/gfx/2d/RecordedEventImpl.h:4198:5
    #9 0x7f031c5f5ccc in mozilla::gfx::InlineTranslator::TranslateRecording(char*, unsigned long) /home/truber/src/m/u/gfx/2d/InlineTranslator.cpp:68:20

For detailed crash information, see attachment.

To reproduce the issue:

1. Build an ASan --enable-fuzzing build including gtests with https://phabricator.services.mozilla.com/D186833 and https://phabricator.services.mozilla.com/D186161 applied.
2. Run FUZZER=Moz2D objdir/dist/bin/firefox test.bin

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: Detailed Crash Information

Comment 2

[Jesse Schwartzentruber (:truber)]

Attachment: Testcase

Comment 3

[Jesse Schwartzentruber (:truber)]

Attachment: crash.cpp

Decoded test.bin

Comment 4

[Daniel Veditz [:dveditz]]

At first glance a one-byte over-read that goes into a color value doesn't sound too terrible, but would it have kept reading more and more if ASAN hadn't detected the problem? Or is it just coincidentally 1 byte too far because of specific data in the testcase and might be arbitrarily distant if the testcase were different? Either of those might justified rating this higher than moderate.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Daniel Veditz [:dveditz] from comment #4)

At first glance a one-byte over-read that goes into a color value doesn't sound too terrible, but would it have kept reading more and more if ASAN hadn't detected the problem? Or is it just coincidentally 1 byte too far because of specific data in the testcase and might be arbitrarily distant if the testcase were different? Either of those might justified rating this higher than moderate.

The read is here and is only a 1 byte read, but it's reading through a byte at a time.

Without a working windows ASan build, I'm guessing from the Detailed Crash Information that we're calling CreateClippedDrawTarget with a format that is incompatible with IntoLuminanceSource.
As far as I can tell there are no checks for that and it just assumes the format has 4 bytes per pixel (confirmed on element with jrmuizel that this is what is required for IntoLuminanceSource).
I'll add in some checks.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1850983: Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel!

Comment 7

[Bob Owen (:bobowen)]

I got my windows build working (just needed addition to PATH), but the test case wouldn't run.
I got the following error:
Fuzzing Interface: Error: No testing callback found

So I bit the bullet and set up a linux build and can reproduce.
Attached patch means we run without an issue.

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 9351814 [details]
Bug 1850983: Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily, it is in code that is before the actual issue and how the issue might be triggered (from a compromised process) is also not obvious.
  Asking for approval because comment 4 suggests that this might be upgraded to sec-high. Comment 5 provides a little more detail about the read.
• 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?: Yes
• 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, just a simple check for the correct format.
• Is Android affected?: Unknown

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9351814 [details]
Bug 1850983: Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel!

Approved to land and uplift

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fec6b3b46bc6
Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel

Comment 11

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

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

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9351814 [details]
Bug 1850983: Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel!

Approved for landing on mozilla-beta before the merge, will be in the 118.0 release candidate, thanks.

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/b1ac8263854f

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9351814 [details]
Bug 1850983: Add check for correct format in DrawTarget::IntoLuminanceSource. r=jrmuizel!

Approved for ESR 115.3, thanks.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/9ad762667d23