1438917 - Possible integer overflow in GrResourceCache::changeUniqueKey

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is
report bp-cde464fd-6aea-4a3d-8341-4fff90180215.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.dylib mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 libmozglue.dylib mozalloc_handle_oom memory/mozalloc/mozalloc_oom.cpp:54
2 libmozglue.dylib moz_xcalloc memory/mozalloc/mozalloc.cpp:84
3 XUL GrResourceCache::changeUniqueKey gfx/skia/skia/src/core/SkTDynamicHash.h:256
4 XUL GrClipStackClip::createSoftwareClipMask const gfx/skia/skia/src/gpu/GrClipStackClip.cpp:485
5 XUL GrClipStackClip::apply const gfx/skia/skia/src/gpu/GrClipStackClip.cpp:319
6 XUL GrRenderTargetContext::addLegacyMeshDrawOp gfx/skia/skia/src/gpu/GrRenderTargetContext.cpp:1630
7 XUL GrSmallPathRenderer::onDrawPath gfx/skia/skia/src/gpu/ops/GrSmallPathRenderer.cpp:769
8 XUL GrRenderTargetContext::internalDrawPath gfx/skia/skia/src/gpu/GrPathRenderer.h:174
9 XUL GrRenderTargetContext::drawPath gfx/skia/skia/src/gpu/GrRenderTargetContext.cpp:1385

=============================================================

I came across this crash on Nightly, on this line:
 fArray = (T**)sk_calloc_throw(sizeof(T*) * fCapacity);

This is an OOM crash, and the allocation size is 18,446,744,056,529,682,432 bytes, which leads me to think that there's an integer overflow here.

I also see another one like this, on 59:
  bp-6d60fdbe-a90f-430e-bfbd-d9f920180215

Maybe somebody was just running a test case from a Skia security bug or something, though I don't see any other bugs with changeUniqueKey in the summary.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

In the middle of updating Skia, it'd be interesting to see if this is around after that gets updated.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Note: that value is 0xFFFFFFFC00000000

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: limit allocations in SkTDynamicHash

fCapacity here can overflow to become negative, which then can cause a crash downwind in sk_calloc_throw. fCapacity can only be a power of two, so the overflow value will be exactly 2^31, which is INT_MIN.

In practice, this negative value will be cast to an unsigned size_t value, then multiplied by sizeof(T*) which is either 4 or 8. On 64 bit systems this will simply cause sk_calloc_throw to trigger an exception, which is not exploitable. 

On 32 bit systems, size_t(INT_MIN) * sizeof(T*) will become 0. However, due to the fact that fCapacity is thusly set to negative, and find/add/remove routines are guarded by index < fCapacity checks, this should not be practically exploitable there either beyond just triggering debug asserts and/or a release crash.

So this is mostly a correctness fix that ensures we assert up front rather than just crashing in various ways downwind.

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8958155 [details] [diff] [review]
limit allocations in SkTDynamicHash

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
No known exploit yet besides above crash.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Nothing specific.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Depends on machinery from Skia m66 update in bug 1444506.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, just trades overflow for an assert.

Comment 5

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

As a sec-audit, this doesn't need sec-approval to go in.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/49bbe768f2389958b159bad3db913d1d0a893323
https://hg.mozilla.org/mozilla-central/rev/49bbe768f238

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you're comfortable doing so.

Comment 8

[Lee Salzman [:lsalzman]]

Comment on attachment 8958155 [details] [diff] [review]
limit allocations in SkTDynamicHash

Approval Request Comment
[Feature/Bug causing the regression]: Pre-existing.
[User impact if declined]: Possible overflows, undefined behavior, and crashes.
[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?]: Just guards a potential security exploit with an assert.
[String changes made/needed]: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8958155 [details] [diff] [review]
limit allocations in SkTDynamicHash

Approved for 60.0b6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25c727364e92