Closed Bug 1224396 (CVE-2017-5113) Opened 4 years ago Closed 2 years ago

Overflow in makeSpace causes potential memory-safety bug

Categories

(Core :: Graphics, defect, P3)

42 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ fixed
firefox57 --- wontfix
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: q1, Assigned: lsalzman)

References

Details

(Keywords: sec-high, Whiteboard: [gfx-noted][adv-main58+][adv-esr52.6+] Bug fix from upstream)

Attachments

(3 files)

makeSpace (gfx\skia\skia\include\core\SkPathRef.h) can experience an integer overflow. If it does, it attempts to allocate a buffer that is far too small. makeSpace's callers then overrun the buffer, writing to memory that they don't own. This bug can ever practically be realized only in 32 bit builds.

Because of Windows memory allocations policies and another bug, this bug appears unlikely to be realizable on Windows 7. It might be realizable under other OSes.


Details
-------

The bug is in line 389, which doesn't check for overflow:

373:     void makeSpace(size_t size) {
374:         SkDEBUGCODE(this->validate();)
375:         ptrdiff_t growSize = size - fFreeSpace;
376:         if (growSize <= 0) {
377:             return;
378:         }
379:         size_t oldSize = this->currSize();
380:         // round to next multiple of 8 bytes
381:         growSize = (growSize + 7) & ~static_cast<size_t>(7);
382:         // we always at least double the allocation
383:         if (static_cast<size_t>(growSize) < oldSize) {
384:             growSize = oldSize;
385:         }
386:         if (growSize < kMinSize) {
387:             growSize = kMinSize;
388:         }
389:         size_t newSize = oldSize + growSize;
390:         // Note that realloc could memcpy more than we need. It seems to be a win anyway. TODO:
391:         // encapsulate this.
392:         fPoints = reinterpret_cast<SkPoint*>(sk_realloc_throw(fPoints, newSize));
...
402:     }

Under Windows 7 SP1, the bug appears unlikely to be realizable because of a confluence between Windows 7's memory allocation policy and something of a bug in |huge_ralloc| (memory\mozjemalloc\jemalloc.c). That bug is that |huge_ralloc| only ever (1) extends an existing block forward in memory or (2) mallocs a new block, copies the existing block to it, and frees the old block. What it doesn't do is (3) to attempt to extend an existing block *backward* in memory, then memmove the existing contents as appropriate; and/or (4) extend an existing block forward in memory if it's in a different "size class". The upshot is that |huge_ralloc| sometimes fails to satisfy a request that it could satisfy. (Which, while dodging a security bug here, must cause some unnecessary OOM crashes).

The above two factors make the makeSpace bug unlikely to be realized in our case. When running the POC under Windows 7, |newSize| in the call to |sk_realloc_throw| (line 392) eventually becomes 0x20000000. Windows allocates that block at 0x80000000. Then the next time line 392 reallocs, the size is 0x40000000. |huge_ralloc| treats this as a different "size class" from 0x20000000, and so doesn't try to extend the block at 0x80000000. Instead, it allocates a new block (which Windows puts at 0xa0000000), copies the existing block into it, and frees it, thus fragmenting the address space. Then the final time line 392 reallocs, the size is 0x80000000. Again this is a different "size class" from the old size, so |huge_ralloc| tries again the malloc-and-free approach. But there is no contiguous block of address space large enough, so the malloc fails.

The bug might still be realizable if the OS allocates memory in the "right" way. This would occur if, for example, the OS allocated the 0x20000000-long block at 0x80000000, the 0x40000000-long block at 0x40000000 (with |huge_ralloc| then freeing the 0x20000000-long block), then the 0x80000000-long block at 0x80000000 (with |huge_ralloc| then freeing the 0x40000000-long block). This also would occur if |huge_ralloc| repeatedly extended an existing 0x20000000-long block that was allocated at 0x80000000. (Note some OSes, e.g., Windows 7, reserve a few hundred KB at the very top of the address space, so the previous addresses might have to be adjusted downward by that amount).


Finally, there is also a technical bug in line 375: |ptrdiff_t| is guaranteed only to be able to contain the difference between any two *valid* pointers into the *same array* (see C++11 s.5.7(6)). It is not guaranteed to be able to contain the difference of two arbitrary |size_t|s ("Unless both pointers point to elements of the same array object, or one past the last element of the array object, the behavior is undefined." Id.).
Comment on attachment 8686863 [details]
POC. Use with a breakpoint in makeSpace to see the bug in action.

><!DOCTYPE html>
>
><html lang="en" xmlns="http://www.w3.org/1999/xhtml">
><head>
>    <meta charset="utf-8" />
>    <title>SKIA overflow!</title>
></head>
><body>
><canvas id="canvas" width="150" height="150"></canvas>
><script type="text/javascript">
>function draw() {
>  var canvas = document.getElementById('canvas');
>  if (canvas.getContext){
>      var ctx = canvas.getContext('2d');
>
>// Try to overflow calculation of |newSize| in |MakeSpace|.
>
>    ctx.beginPath()
>    ctx.moveTo(125, 125);
>
>    for (var i = 0; i < 1000000000; ++i) {
>        ctx.lineTo(125, 45);
>    }
>
>    ctx.closePath();
>    ctx.stroke();
>  }
>}
>
>draw();
>
></script>
></body>
></html>
Attachment #8686863 - Attachment description: POC. Use wth a breakpoint in makeSpace to see the bug in action. → POC. Use with a breakpoint in makeSpace to see the bug in action.
Sorry about the spurious comment 1. There are a few garbage characters at the beginning of the POC. Just cut them out.
P.S. You have to run the POC on a system without Direct2D, or set |gfx.direct2d.disabled| to |true|.
There are also similar potential-overflow bugs in the same module in |incReserve| (line 312) and |resetToSize| (lines 328-30), and in SkPathRef.cpp in |growForRepeatedVerb| (line 321).
Flags: sec-bounty?
Group: core-security → gfx-core-security
Keywords: testcase-wanted
Oops, somehow didn't notice the attached testcase.
Keywords: testcase-wanted
Lee, can you put a security rating on this or confirm how likely we are to hit this?
Flags: needinfo?(lsalzman)
(In reply to Al Billings [:abillings] from comment #6)
> Lee, can you put a security rating on this or confirm how likely we are to
> hit this?

It is somewhat hard to rate this. As the reporter discussed above, it is basically impossible to cause on a 64 bit platform since you will always hit an out-of-memory condition before it could happen.

On Linux 32 bit we don't use Skia at all, we just use Cairo.

So we're left with Windows XP/7 32 bit builds, on which Skia is the canvas fallback, and Android builds.

It seems like you would more likely run afoul of an out-of-memory even on 32 bit platforms well before you ever got to this overflow condition. I.e. you'd need to create a path big enough to take up almost 4GB of memory to overflow the 32 bit size, and you're doing it by bumping it in small increments. Also note that the path has a bunch of other memory structures around that would grow in size comparably, so you'd be almost certain to run out of memory for allocations long before this particular buffer in the path could hit that 4 GB threshold. I don't think the OS would even allow the process to keep around that much user memory in the first place.

If and only if you ignore that it seems unfeasible, it would be a sec-high or a sec-critical, I think.

Regardless, I did communicate the issue upstream to Mike Reed to see if the Skia folks at Google wanted to do anything about it.
Flags: needinfo?(lsalzman)
Slightly misread the code. It is more or less doubling the size of the path on each reallocation. Though the reporter's description of its infeasibility on Windows more or less stands. Memory limits on Android would trigger long before this would as well there.
Lee: is there any reasonable way to re-write the code to prevent this? Or is it impossible enough that we can just let testcases like this be a denial of service crash?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(lsalzman)
Keywords: sec-moderate
(In reply to Daniel Veditz [:dveditz] from comment #9)
> Lee: is there any reasonable way to re-write the code to prevent this? Or is
> it impossible enough that we can just let testcases like this be a denial of
> service crash?

The best you could hope for is to rewrite the code to trigger an OOM if it detected an overflow, whereas we already expect an OOM to trigger before it ever gets to the overflow. Either way, you get OOM. So it's still a crash no matter what.

I already CC'd Mike Reed at Google on this but there has been no movement upstream as of yet.
Flags: needinfo?(lsalzman)
Whiteboard: [gfx-noted]
Milan, please consider adding me to the CC list for the bug you just cited. Thanks.
This looks like they only just recently fixed this upstream. These are the upstream patches that are relevant:

https://skia.googlesource.com/skia/+/6229b1240aae8961a4bf34493b964d944a0a06ee
https://skia.googlesource.com/skia/+/ac32662d128484eae3230653e3794a6f33dd9f5b

This patch is just a straight backport of those two.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8930669 - Flags: review?(milan)
Comment on attachment 8930669 [details] [diff] [review]
Skia path allocation cleanups

Review of attachment 8930669 [details] [diff] [review]:
-----------------------------------------------------------------

What's the difference between "abort" and "release assert"?
Attachment #8930669 - Flags: review?(milan) → review+
(In reply to Milan Sreckovic [:milan] from comment #13)
> Comment on attachment 8930669 [details] [diff] [review]
> Skia path allocation cleanups
> 
> Review of attachment 8930669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's the difference between "abort" and "release assert"?

Not much, i.e.

#define SkASSERT_RELEASE(cond) \
        static_cast<void>( (cond) ? (void)0 : []{ SK_ABORT("assert(" #cond ")"); }() )
SkArenaAlloc did not exist yet for us in 52 ESR. So just include the makeSpace cleanup here.
Attachment #8931011 - Flags: review?(milan)
https://hg.mozilla.org/mozilla-central/rev/db4543ce21ce
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attachment #8931011 - Flags: review?(milan) → review+
The changesets in comment 12 refer to https://crbug.com/747043 and https://crbug.com/728936, both labelled "Security_Severity-High" by the Chrome team. 747043 was assigned CVE-2017-5113, which we should use rather than assigning a duplicate CVE to the same fix. 728936 didn't affect a shipping Chrome so they didn't assign a CVE for that one.
Alias: CVE-2017-5113
Whiteboard: [gfx-noted] → [gfx-noted] Bug fix from upstream
Comment on attachment 8931011 [details] [diff] [review]
Skia path allocation cleanups (52 ESR)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high. Fix already publicly released upstream so bug is known.
User impact if declined: Memory overruns via canvas 2d paths.
Fix Landed on Version: 59
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8931011 - Flags: approval-mozilla-esr52?
Comment on attachment 8930669 [details] [diff] [review]
Skia path allocation cleanups

Approval Request Comment
[Feature/Bug causing the regression]: Pre-existing condition in Skia affecting all released versions.
[User impact if declined]: Memory overruns via canvas 2d paths.
[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?]: Already released/tested in upstream Skia/Chrome. Just sets a sane limit on memory allocation to avoid overflow.
[String changes made/needed]: none
Attachment #8930669 - Flags: approval-mozilla-beta?
Comment on attachment 8930669 [details] [diff] [review]
Skia path allocation cleanups

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8930669 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8931011 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Group: gfx-core-security → core-security-release
(In reply to Lee Salzman [:lsalzman] from comment #19)
> [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

Has automated coverage, does not need manual testing - per Lee.
Flags: qe-verify-
Whiteboard: [gfx-noted] Bug fix from upstream → [gfx-noted][adv-main58+][adv-esr52.6+] Bug fix from upstream
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.