1463244 - (CVE-2018-12361) Buffer Overflow in gfx::SwizzleCopy

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[r2]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

gfx::SwizzleCopy (gfx/2d/Swizzle.cpp) is unsafe:

static void
SwizzleCopy(const uint8_t* aSrc, int32_t aSrcGap,
            uint8_t* aDst, int32_t aDstGap,
            IntSize aSize, int32_t aBPP)
{
  if (aSrc != aDst) {
    int32_t rowLength = aBPP * aSize.width; // (1)
    for (int32_t height = aSize.height; height > 0; height--) {
      memcpy(aDst, aSrc, rowLength); // (0)
      aSrc += rowLength + aSrcGap; // (2)
      aDst += rowLength + aDstGap; // (3)
    }
  }
}

At (1), rowLength can overflow and become a very large negative value. For example, if aSize.width = 0x20000000, it will overflow to -2147483648 at 4 BPP. Then, the overflowed value is used at (2) and (3) to calculate pointers, that may either underflow the currently owned memory, or even wrap around aSrc/aDst on x86 arch. Then, the call to memcpy() at (0) with wrongly calculated pointers and count can cause an out of bound access.

In addition, both aSrcGap and aDstGap are calculated in the same unsafe manner in the caller:

bool
SwizzleData(const uint8_t* aSrc, int32_t aSrcStride, SurfaceFormat aSrcFormat,
            uint8_t* aDst, int32_t aDstStride, SurfaceFormat aDstFormat,
            const IntSize& aSize)
{
  if (aSize.IsEmpty()) {
    return true;
  }
  IntSize size = CollapseSize(aSize, aSrcStride, aDstStride); // (4)
  // Find gap from end of row to the start of the next row.
  int32_t srcGap = aSrcStride - BytesPerPixel(aSrcFormat) * aSize.width; // (5)
  int32_t dstGap = aDstStride - BytesPerPixel(aDstFormat) * aSize.width; // (6)
  MOZ_ASSERT(srcGap >= 0 && dstGap >= 0); // (7)

[...]

  if (aSrcFormat == aDstFormat) {
    // If the formats match, just do a generic copy.
    SwizzleCopy(aSrc, srcGap, aDst, dstGap, size, BytesPerPixel(aSrcFormat));
    return true;
  }

[...]
}

At (5) and (6), srcGap and dstGap can overflow in exactly the same way as described above. The possibly overflowed values are then passed to SwizzleCopy and used to calculate pointer values.

Here an example of the crash on Firefox 59.0.2 which happens when aDst underflows and wraps around:
https://crash-stats.mozilla.com/report/index/19b58581-7c59-4227-95c3-9d0b30180422

And the crashes which happen when aDst or aSrc point to unowned memory:
aSrc: https://crash-stats.mozilla.com/report/index/aa3f07a1-6ed3-4aad-ae17-ed1170180424
aDst: https://crash-stats.mozilla.com/report/index/905535d1-49f9-4657-8e3a-8b73d0180427

The input graphics region which causes incorrect calculations is sanitized in a few places before the call to SwizzleCopy, but it is not sanitized properly. For example, at (4) the call to CollapseSize (/gfx/2d/Swizzle.cpp) does check that aSize.width * aSize.height is within safe bounds of int32, but the check only happens if specific conditions are met. Also, it does not account for the BPP multiplier which is used at (1), (5), (6):

static inline IntSize
CollapseSize(const IntSize& aSize, int32_t aSrcStride, int32_t aDstStride)
{
  if (aSrcStride == aDstStride &&
      aSrcStride == 4 * aSize.width) {
    CheckedInt32 area = CheckedInt32(aSize.width) * CheckedInt32(aSize.height);
    if (area.isValid()) {
      return IntSize(area.value(), 1);
    }
  }
  return aSize;
}

One more check at (7) is a correct assert, however it's compiled out in release builds. A few more checks happen inside the CopyRect (gfx/2d/DataSurfaceHelpers.cpp) caller of SwizzleData, that again they do not account for the BytesPerPixel multiplier.

It would be optimal to insert early checks, for every acquired graphics region, that both values of width * bpp and height * bpp do not overflow int32, ideally with a margin. Because the value of BytesPerPixel is relatively small, such a check would automatically rule out majority of other possible overflows.

Comment 1

[Daniel Veditz [:dveditz]]

> Here an example of the crash on Firefox 59.0.2 which happens when aDst underflows and wraps around:
> https://crash-stats.mozilla.com/report/index/19b58581-7c59-4227-95c3-9d0b30180422
> 
> And the crashes which happen when aDst or aSrc point to unowned memory:
> aSrc: https://crash-stats.mozilla.com/report/index/aa3f07a1-6ed3-4aad-ae17-ed1170180424
> aDst: https://crash-stats.mozilla.com/report/index/905535d1-49f9-4657-8e3a-8b73d0180427

Do you have the PoCs that produce those?
What difference are you seeing in those last two that lets you know one was aSrc and the other aDst?

Comment 2

[Daniel Veditz [:dveditz]]

Evidence from crash-stats shows multiple people can hit _WRITE access violations here in practice. I thought we had size/width limits higher up in the code to backstop these kinds of integer overflows?

Comment 3

[r2]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> > And the crashes which happen when aDst or aSrc point to unowned memory:
> > aSrc: https://crash-stats.mozilla.com/report/index/aa3f07a1-6ed3-4aad-ae17-ed1170180424
> > aDst: https://crash-stats.mozilla.com/report/index/905535d1-49f9-4657-8e3a-8b73d0180427
> 
> Do you have the PoCs that produce those?

No, I decided not to invest the time into creating PoCs, since the crash stats provide a very strong proof of bug, and also indicate what exactly went wrong with this code.

Btw, here is a fresh one from 60.0.1: https://crash-stats.mozilla.com/report/index/21f0097d-a74f-4510-a530-7949c0180522

> What difference are you seeing in those last two that lets you know one was
> aSrc and the other aDst?

The difference is in the type of access violation. In the first crash, the type of exception is EXCEPTION_ACCESS_VIOLATION_READ and the crashing site is "memcpy(aDst, aSrc, rowLength)" within a loop. Therefore, it crashed while trying to read aSrc. 

Next, consider the crash address: 0x2d198000 - it's page aligned. Typically when you have an out of bounds read, it will read successfully (iterating over unowned but valid memory) until it hits the end of the heap chunk. Then it will crash while trying to read unallocated memory, at a page-aligned address. Heap chunks are always page-aligned. Thus, an address like this is a strong indication of an iterative OOB read.

For the second crash, the type of crash is EXCEPTION_ACCESS_VIOLATION_WRITE and the crashing site is the same "memcpy(aDst, aSrc, rowLength)". Therefore it crashed while trying to write to aDst. That could happen due to a number of reasons, for ex. aDst miscalculated, or rowLength was too long for the allocated buffer which is pointed to by aDst. However the third crash which I have referenced (https://crash-stats.mozilla.com/report/index/19b58581-7c59-4227-95c3-9d0b30180422) provides a strong indication of the former (aDst pointer miscalculation), since it's crashed with a bogus address 0xffffffffbda8fffc. This address is in kernel mode, and should never be referenced by a userland program. It can most likely be referenced due to a pointer arithmetic wrap-around. 

Thus I have concluded that the primary probable reason of crashes is due to pointer miscalculation. 

The root cause of such miscalculation may originate from one of the multiple issues with the code which calls "SwizzleCopy", as well as in SwizzleCopy itself. These issues I have reported in the original bug report.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: cleanup of swizzle stride calculations

This hardens some code in DataSurfaceHelpers.cpp to calculate buffer sizes in a safe way and also hardens the swizzle stride gap code such that overflows will be handled up front (and return early with an error) so that later swizzle code doesn't need to worry about safety.

Comment 5

[Lee Salzman [:lsalzman]]

Introduced by bug 738343 when I implemented the swizzle API.

Comment 6

[Lee Salzman [:lsalzman]]

Comment on attachment 8980074 [details] [diff] [review]
cleanup of swizzle stride calculations

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch description doesn't point at the problem, per se, but with knowledge of where the swizzle and data surface helpers are used, one might be able to guess how to create surfaces that would cause the overflow here.

> Which older supported branches are affected by this flaw?
This affects 54+ as I introduced the swizzle API in bug 738343. ESR 52 should be safe/unaffected.

> How likely is this patch to cause regressions; how much testing does it need?
This just causes the swizzle routines to bail when an overflow is detected at runtime. It shouldn't be very risky.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for trunk. If you nominate Beta and ESR60 patches, I'll approve those as well to land after it is on mozilla-central.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaf8f2a1975c57f5467968734d110ac7becc7ee
https://hg.mozilla.org/mozilla-central/rev/2aaf8f2a1975

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/ESR60 approval when you get a chance. It grafts cleanly to both as-landed.

Comment 10

[Lee Salzman [:lsalzman]]

Comment on attachment 8980074 [details] [diff] [review]
cleanup of swizzle stride calculations

Approval Request Comment
[Feature/Bug causing the regression]: bug 738343
[User impact if declined]: Potential buffer overruns
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: Makes some routines explicitly return failure rather than continuing under conditions that would produce an overflow, but should not affect legitimate use-cases.
[String changes made/needed]: none

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8980074 [details] [diff] [review]
cleanup of swizzle stride calculations

Fixes a sec-crit. Approved for 61.0b9 and ESR 60.1.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e14f2af3e3c
https://hg.mozilla.org/releases/mozilla-esr60/rev/78c708386b35