1837450 - Potential Integer Overflow from malicious content process with custom cursors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tom Ritter [:tjr] (OOTO until April)]

In BrowserParent::RecvSetCursor( we have the following code:

mozilla::ipc::IPCResult BrowserParent::RecvSetCursor(
    const nsCursor& aCursor, const bool& aHasCustomCursor,
    Maybe<BigBuffer>&& aCursorData, const uint32_t& aWidth,
    const uint32_t& aHeight, const float& aResolutionX,
    const float& aResolutionY, const uint32_t& aStride,
    const gfx::SurfaceFormat& aFormat, const uint32_t& aHotspotX,
    const uint32_t& aHotspotY, const bool& aForce) {
   ...
  if (aHasCustomCursor) {
    if (!aCursorData || aHeight * aStride != aCursorData->Size() ||
        aStride < aWidth * gfx::BytesPerPixel(aFormat)) {
      return IPC_FAIL(this, "Invalid custom cursor data");
    }
    const gfx::IntSize size(aWidth, aHeight);
    RefPtr<gfx::DataSourceSurface> customCursor =
        gfx::CreateDataSourceSurfaceFromData(size, aFormat, aCursorData->Data(),
                                             aStride);

aHeight * aStride != aCursorData->Size() are all attacker controlled, as are aStride < aWidth * gfx::BytesPerPixel(aFormat).

It seems like it would be possible to choose a very large aHeight, such that aHeight * aStride would overflow - and the overflowed value would equal aCursorData->Size(). aStride would be reasonable, so aStride < aWidth * gfx::BytesPerPixel(aFormat) would still be true.

We will eventually wind up with a raw pointer to data, and a corresponding height that is way too big which will probably result in a buffer overread or overwrite somewhere.

I'm not certain this is an issue, but I'm doing some code inspection based on patterns of previous sandbox escapes, so I'd appreciate another set of eyes confirming this is or isn't an issue.

Comment 2

[Andrew McCreight [:mccr8]]

Emilio, does this seem like a valid issue? Thanks.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

It seems believable.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Comment 5

[Andrew McCreight [:mccr8]]

I'll conservatively mark this sec-high then. Thanks.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Security Approval Request

• How easily could an exploit be constructed based on the patch?: not easily, I believe.
• 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?: Should apply cleanly
• How likely is this patch to cause regressions; how much testing does it need?: not likely. It just improves IPC validation.
• Is Android affected?: Yes

Comment 7

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Approved to request uplift and land

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Beta/Release Uplift Approval Request

• User impact if declined: theoretical attack, so ideally not much
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: N/A
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple validation fix.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 9

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

Check for overflow when validating custom cursor data. r=nika
https://hg.mozilla.org/integration/autoland/rev/8b00e3ecb05c7b19cc9945a01725d8083612f071
https://hg.mozilla.org/mozilla-central/rev/8b00e3ecb05c

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Approved for 115.0b8.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/72eea2a38bc8

Comment 12

[Donal Meehan [:dmeehan]]

:emilio could you add an esr102 approval on this and a patch based on esr?

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: comment 0
• Fix Landed on Version: 116
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): More IPC validation.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: ESR102 patch.

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9339569 [details]
Bug 1837450 - Check for overflow when validating custom cursor data. r=nika

Approved for 102.13esr.

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/dfbd8a4c3e2b

Comment 17

[Christian Holler (:decoder)]

I think I was able to confirm this with fuzzing:

INFO: Replaying IPC packet with payload:
  0x00 0x00 0x00 0x00 0x00 0x00 0x11 0xFF 0x98 0xFF 0xFF 0xFF 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x00 0x00 0x00 0x0A 0x00 0x00
  0xFF 0x20 0x00 0x21 0x00 0x01 0x00 0x00 0x00 0x00 0x00 0x20 0x00 0xFF 0xFF 0xFF
  0x30 0x00 0x00 0x00 0x0A 0x00 0xFF 0xFF 0xFF 0xFF 0x98 0xFF 0xFF 0xFF 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00

=================================================================
==4090346==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020002dd951 at pc 0x5605330af60d bp 0x7ffd98bcd870 sp 0x7ffd98bcd038
READ of size 49152 at 0x6020002dd951 thread T0
    #0 0x5605330af60c in __asan_memcpy /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
    #1 0x7fa7d29d428b in mozilla::gfx::SwizzleCopy(unsigned char const*, int, unsigned char*, int, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, int) gfx/2d/Swizzle.cpp:764:7
    #2 0x7fa7d29ceca7 in mozilla::gfx::SwizzleData(unsigned char const*, int, mozilla::gfx::SurfaceFormat, unsigned char*, int, mozilla::gfx::SurfaceFormat, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) gfx/2d/Swizzle.cpp:1183:5
    #3 0x7fa7d275454d in mozilla::gfx::CopyRect(mozilla::gfx::DataSourceSurface*, mozilla::gfx::DataSourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits>) gfx/2d/DataSurfaceHelpers.cpp:325:3
    #4 0x7fa7d2752630 in mozilla::gfx::CreateDataSourceSurfaceFromData(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, unsigned char const*, int) gfx/2d/DataSurfaceHelpers.cpp:50:7
    #5 0x7fa7dc4016b6 in mozilla::dom::BrowserParent::RecvSetCursor(nsCursor const&, bool const&, mozilla::Maybe<mozilla::ipc::BigBuffer>&&, unsigned int const&, unsigned int const&, float const&, float const&, unsigned int const&, mozilla::gfx::SurfaceFormat const&, unsigned int const&, unsigned int const&, bool const&) dom/ipc/BrowserParent.cpp:2225:9
    #6 0x7fa7dc67b6e2 in mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&) objdir/ipc/ipdl/PBrowserParent.cpp:4007:81
    [...]

0x6020001261f1 is located 0 bytes after 1-byte region [0x6020001261f0,0x6020001261f1) 
allocated by thread T0 here:
    #0 0x55ca319d539e in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
    #1 0x7ff46214824c in IPC::ParamTraits<mozilla::ipc::BigBuffer>::Read(IPC::MessageReader*, mozilla::ipc::BigBuffer*) ipc/glue/BigBuffer.cpp:94:34
    #2 0x7ff463662b50 in IPC::ReadResult<mozilla::ipc::BigBuffer, std::is_default_constructible_v<mozilla::ipc::BigBuffer> || detail::HasDeprecatedReadParamPrivateConstructor<mozilla::ipc::BigBuffer>(0)> IPC::ReadParam<mozilla::ipc::BigBuffer>(IPC::MessageReader*) ipc/chromium/src/chrome/common/ipc_m
essage_utils.h:482:13
    #3 0x7ff46d117e93 in IPC::ParamTraits<mozilla::Maybe<mozilla::ipc::BigBuffer>>::Read(IPC::MessageReader*, mozilla::Maybe<mozilla::ipc::BigBuffer>*) objdir/dist/include/ipc/IPCMessageUtilsSpecializations.h:498:31
    #4 0x7ff46d09cd90 in IPC::ReadResult<mozilla::Maybe<mozilla::ipc::BigBuffer>, std::is_default_constructible_v<mozilla::Maybe<mozilla::ipc::BigBuffer>> || detail::HasDeprecatedReadParamPrivateConstructor<mozilla::Maybe<mozilla::ipc::BigBuffer>>(0)> IPC::ReadParam<mozilla::Maybe<mozilla::ipc::BigBuffer>>(IPC::MessageReader*) /srv/repos/i
pc-research/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:482:13 
    #5 0x7ff46cf1995a in mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&) objdir/ipc/ipdl/PBrowserParent.cpp:3886:44
    [...]

SUMMARY: AddressSanitizer: heap-buffer-overflow compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3 in __asan_memcpy
Shadow bytes around the buggy address:
  0x602000126100: fa fa fd fd fa fa fd fd fa fa 00 00 fa fa 00 00
=>0x602000126180: fa fa 00 00 fa fa 00 00 fa fa fd fd fa fa[01]fa
  0x602000126200: fa fa 00 00 fa fa 00 00 fa fa fd fd fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd

Also confirmed it does not reproduce anymore with the patch here. Took 6 CPU hours on the IPC_SingleMessage target (which is experimental).