My favorites | Sign in
Logo
             
New issue | Search
for
| Advanced search | Search tips
Issue 10736: SkMask::computeImageSize() integer overflow
  Back to list
 
Reported by skylined@chromium.org, Apr 19, 2009
An integer overflow can be triggered in 
skia\sgl\skmask.cpp!SkMask::computeImageSize, which leads to Chrome (2.x) 
not allocating sufficient memory for data. This issue can likely be 
exploited to execute arbitrary code. The vulnerable code is:

size_t SkMask::computeImageSize() const
{
    return fBounds.height() * fRowBytes;
}

As you can see, there is no check if the result of the multiplication can 
fit in a size_t. Here's some JavaScript that can be used to trigger the 
issue:

<SCRIPT>
  oCanvas=document.createElement("CANVAS");
  o2dContext=oCanvas.getContext("2d");
  o2dContext.transform(78,76,-6,0,-10000,9);
  o2dContext.translate(-55,0);
  o2dContext.rotate(1000);
  o2dContext.shadowBlur=1000;
  o2dContext.shadowOffsetX=1;
  o2dContext.clearRect(1,5,48,-21);
</SCRIPT>

Here's more details:
    CdbFatalExceptionInfo(WriteAV()[arbitrary]@chrome!memset+0x65) (100% 
reproducable after 11 attempts, reducing)
        Attempt to write to arbitrary memory at arbitrary, instruction:
        03b6bf35 8807            mov     byte ptr [edi],al
        Registers:
            eax=000000ff ebx=03419b90 ecx=8b936f2d edx=00000001 
esi=05fee548 edi=8b936f2d esp=05fee50c ebp=05fee534 eip=03b6bf35
        Stack:
            ChildEBP RetAddr  
            05fee50c 03fc9abf chrome_28f0000!memset(unsigned char * dst = 
0xffffcf15 "--- memory read error at address 0xffffcf15 ---", unsigned char 
value = 0x14 '', unsigned long count = 1)+0x65
            05fee534 03fbf403 chrome_28f0000!SkA8_Blitter::blitH(int x = -
12523, int y = 380692, int width = 1)+0x5f
            05fee57c 03fbf27a chrome_28f0000!walk_edges(struct SkEdge * 
prevHead = 0x05fee5f0, SkPath::FillType fillType = kWinding_FillType (0), 
class SkBlitter * blitter = 0x0d079020, int stop_y = 384133, <function> * 
proc = 0x00000000)+0xd3
            05fee638 03fc0058 chrome_28f0000!sk_fill_path(class SkPath * 
path = 0x05feed98, struct SkIRect * clipRect = 0x00000000, class SkBlitter 
* blitter = 0x0d079020, int stop_y = 384133, int shiftEdgesUp = 0, class 
SkRegion * clipRgn = 0x05feee7c)+0x17a
            05fee6c0 03fd6c4b chrome_28f0000!SkScan::FillPath(class SkPath 
* path = 0x05feed98, class SkRegion * clip = 0x05feee7c, class SkBlitter * 
blitter = 0x0d079020)+0xd8
            05feebd0 03fb958a chrome_28f0000!SkScan::AntiFillPath(class 
SkPath * path = 0x05feed98, class SkRegion * clip = 0x05feee7c, class 
SkBlitter * blitter = 0x0d079020)+0xeb
            05feedd4 03f8d912 chrome_28f0000!SkDraw::drawPath(class SkPath 
* origSrcPath = 0x05fef194, class SkPaint * paint = 0x05feee08, class 
SkMatrix * prePathMatrix = 0x00000000, bool pathIsMutable = false)+0x3ca
            05feedf0 03fbdcfc chrome_28f0000!SkDraw::drawPath(class SkPath 
* src = 0x05fef194, class SkPaint * paint = 0x05feee08)+0x22
            05feeee0 03fbd9e4 chrome_28f0000!draw_into_mask(struct SkMask * 
mask = 0x05feefa4, class SkPath * devPath = 0x05fef194)+0xfc
            05feeef0 03fd6f62 chrome_28f0000!SkDraw::DrawToMask(class 
SkPath * devPath = 0x05fef194, struct SkIRect * clipBounds = 0x096af028, 
class SkMaskFilter * filter = 0x0d0a5020, class SkMatrix * filterMatrix = 
0x0a14d04c, struct SkMask * mask = 0x05feefa4, SkMask::CreateMode mode = 
kComputeBoundsAndRenderImage_CreateMode (2))+0x94
            05feefc4 03fb94dd chrome_28f0000!SkMaskFilter::filterPath(class 
SkPath * devPath = 0x05fef194, class SkMatrix * matrix = 0x0a14d04c, class 
SkRegion * clip = 0x096af028, class SkBounder * bounder = 0x00000000, class 
SkBlitter * blitter = 0x0d2d5020)+0x42
            05fef1d0 03f8d912 chrome_28f0000!SkDraw::drawPath(class SkPath 
* origSrcPath = 0x05fef36c, class SkPaint * paint = 0x05fef518, class 
SkMatrix * prePathMatrix = 0x00000000, bool pathIsMutable = false)+0x31d
            05fef1ec 03fb8d90 chrome_28f0000!SkDraw::drawPath(class SkPath 
* src = 0x05fef36c, class SkPaint * paint = 0x05fef518)+0x22
            05fef3a0 03f8d8ae chrome_28f0000!SkDraw::drawRect(struct SkRect 
* rect = 0x05fef55c, class SkPaint * paint = 0x05fef518)+0xe0
            05fef3b4 03f9066b chrome_28f0000!SkDevice::drawRect(class 
SkDraw * draw = 0x05fef3d4, struct SkRect * r = 0x05fef55c, class SkPaint * 
paint = 0x05fef518)+0x1e
            05fef450 03688600 chrome_28f0000!SkCanvas::drawRect(struct 
SkRect * r = 0x05fef55c, class SkPaint * paint = 0x05fef518)+0xcb
            05fef574 0352909d 
chrome_28f0000!WebCore::GraphicsContext::clearRect(class WebCore::FloatRect 
* rect = 0x05fef584)+0xf0
            05fef5a0 02c07654 
chrome_28f0000!WebCore::CanvasRenderingContext2D::clearRect(float x = 71, 
float y = -16, float width = 48, float height = 21)+0xad
            05fef5f8 03419eb9 
chrome_28f0000!WebCore::CanvasRenderingContext2DInternal::clearRectCallback
(class v8::Arguments * args = 0x05fef658)+0xe4
            05fef730 064f018b 
chrome_28f0000!v8::internal::Builtin_HandleApiCall(int __argc__ = 5, class 
v8::internal::Object ** __argv__ = 0x05fef760)+0x329
            WARNING: Frame IP not in any known module. Following frames may 
be wrong.
            05fef824 03351891 0x64f018b
            05fef8b8 03351762 chrome_28f0000!v8::internal::Invoke(bool 
construct = true, class v8::internal::Handle<v8::internal::JSFunction> func 
= class v8::internal::Handle<v8::internal::JSFunction>, class 
v8::internal::Handle<v8::internal::Object> receiver = class 
v8::internal::Handle<v8::internal::Object>, int argc = 100595660, class 
v8::internal::Object *** args = 0x0650a6b9, bool * has_pending_exception = 
0x06a3c4b9)+0x111
            05fef8dc 03320bb8 
chrome_28f0000!v8::internal::Execution::Call(class 
v8::internal::Handle<v8::internal::JSFunction> func = class 
v8::internal::Handle<v8::internal::JSFunction>, class 
v8::internal::Handle<v8::internal::Object> receiver = class 
v8::internal::Handle<v8::internal::Object>, int argc = 0, class 
v8::internal::Object *** args = 0x00000000, bool * pending_exception = 
0x05fef92b)+0x22
            05fef96c 02a5e5ef chrome_28f0000!v8::Function::Call(class 
v8::Handle<v8::Object> recv = class v8::Handle<v8::Object>, int argc = 0, 
class v8::Handle<v8::Value> * argv = 0x00000000)+0x108
            05fef9a4 02ace0bf 
chrome_28f0000!WebCore::V8Proxy::CallFunction(class 
v8::Handle<v8::Function> function = class v8::Handle<v8::Function>, class 
v8::Handle<v8::Object> receiver = class v8::Handle<v8::Object>, int argc = 
0, class v8::Handle<v8::Value> * args = 0x00000000)+0x5f
            05fefa20 035cb3da 
chrome_28f0000!WebCore::ScheduledAction::execute(class 
WebCore::ScriptExecutionContext * context = 0x08f59054)+0xff
            05fefa5c 036920c5 
chrome_28f0000!WebCore::DOMTimer::fired(void)+0x12a
            05fefa94 036921ea 
chrome_28f0000!WebCore::ThreadTimers::fireTimers(double fireTime = 
1239927738.0545001, class WTF::Vector<WebCore::TimerBase *,0> * 
firingTimers = 0x05fefaac)+0xb5
            05fefac8 03692136 
chrome_28f0000!WebCore::ThreadTimers::sharedTimerFiredInternal(void)+0xaa
            05fefad0 02904562 
chrome_28f0000!WebCore::ThreadTimers::sharedTimerFired(void)+0x16
            05fefae0 02904fdc 
chrome_28f0000!webkit_glue::WebKitClientImpl::DoTimeout(void)+0x22
            05fefaec 02904af4 
chrome_28f0000!DispatchToMethod<webkit_glue::WebKitClientImpl,void (class 
webkit_glue::WebKitClientImpl * obj = 0x061a9020, <function> * method = 
0x02904540, struct Tuple0 * arg = 0x05fefb03)+0xc
            05fefb08 03ac4b29 
chrome_28f0000!base::BaseTimer<webkit_glue::WebKitClientImpl,0>::TimerTask:
:Run(void)+0x54
            05fefbbc 03ac4bd5 chrome_28f0000!MessageLoop::RunTask(class 
Task * task = 0x0d195020)+0xb9
            05fefbcc 03ac5246 
chrome_28f0000!MessageLoop::DeferOrRunPendingTask(struct 
MessageLoop::PendingTask * pending_task = 0x05fefbf0)+0x35
            05fefc10 03b51aa0 
chrome_28f0000!MessageLoop::DoDelayedWork(class base::Time * 
next_delayed_work_time = 0x05dc9038)+0x116
            05fefcf8 03ac444b 
chrome_28f0000!base::MessagePumpDefault::Run(class 
base::MessagePump::Delegate * delegate = 0x05fefeb0)+0xf0
            05fefda8 03ac42b0 
chrome_28f0000!MessageLoop::RunInternal(void)+0xfb
            05fefde0 03ac413a 
chrome_28f0000!MessageLoop::RunHandler(void)+0x90
            05fefe08 03ae9138 chrome_28f0000!MessageLoop::Run(void)+0x3a
            05feffa4 03ae8271 
chrome_28f0000!base::Thread::ThreadMain(void)+0xb8
            05feffb4 7c80b713 chrome_28f0000!`anonymous 
namespace'::ThreadFunc(void * closure = 0x05ded02c)+0x21
            05feffec 00000000 kernel32!BaseThreadStart+0x37


Comment 1 by skylined@chromium.org, Apr 19, 2009
Repro:

http://skypher.com/SkyLined/Repro/Chrome/Issue%2010736%20-
%20SkMask%20computeImageSize()%20integer%20overflow/repro.html

AFAIK this does not affect Chrome 1.x
Comment 2 by deanm@chromium.org, Apr 20, 2009
(No comment was entered for this change.)
Cc: bre...@chromium.org r...@android.com
Comment 3 by skylined@chromium.org, Apr 20, 2009
There are more similar problems:
Same file:

size_t SkMask::computeTotalImageSize() const
{
    size_t size = this->computeImageSize();

    if (fFormat == SkMask::k3D_Format)
        size *= 3;
    return size;
}

Again, multiplication without checking for integer overflows.

I'll have a look at more of the code in skia\sgl to check if this prevails throughout 
the code.
Comment 4 by skylined@chromium.org, Apr 20, 2009
There are more files that contain dubious length calculations (eg. SkBitmap.cpp, SkWrite32.cpp). It makes sense to scan all the code for them. Without knowing if the 
variables used in the calculations are limited in some way, it's not possible to 
determine which ones are vulnerable and which are not. To err on the side of caution, 
it makes sense to add checks for integer overflows to all of them if this doesn't 
incur a significant performance penalty. Maybe we can use/create something like 
SafeInt (http://msdn2.microsoft.com/en-us/library/ms972705.aspx).
Comment 5 by mal.chromium, Apr 20, 2009
(No comment was entered for this change.)
Owner: bre...@chromium.org
Cc: secur...@chromium.org
Labels: Mstone-2.0
Comment 6 by mal.chromium, Apr 20, 2009
oops.. meant to assign
Status: Assigned
Comment 7 by brettw@chromium.org, Apr 22, 2009
This also probably affects Android's browser (I think they support Canvas).
Comment 8 by brettw@chromium.org, Apr 22, 2009
For reference, here's the patch I started:

 size_t SkMask::computeImageSize() const
 {
-    return fBounds.height() * fRowBytes;
+    int result = (int)fBounds.height() * (int)fRowBytes;
+    if (result < 0)
+        return -1;
+    return result;
 }

 size_t SkMask::computeTotalImageSize() const
 {
     size_t size = this->computeImageSize();
+    if (size == -1)
+        return -1;

-    if (fFormat == SkMask::k3D_Format)
+    if (fFormat == SkMask::k3D_Format) {
+        if (size >= 0x55555555)
+            return -1;  // Prevent overflow.
         size *= 3;
+    }
     return size;
 }


I got pretty bogged down making all callers check for -1 before doing anything with 
the buffer (or trying to allocate it). Several cases weren't clear to me what to do 
on error and be in a state that wouldn't be inconsistent.

I'm going to just kill the process if this overflows for now. Hopefully we can come 
up with a better solution later.
Comment 9 by brettw@chromium.org, Apr 22, 2009
Here is the patch which I tested with BJ's testcase. I'm not sure how to do a code 
review. Can somebody check it?

 size_t SkMask::computeImageSize() const
 {
-    return fBounds.height() * fRowBytes;
+    // Prevent an integer overflow by killing the process. This can lead to an
+    // exploitable buffer overflow (we check for signed overflows just for good

+    // measure, since a bitmap that size is clearly a problem anyway).
+    //
+    // Ideally, all callers would check for an error from this function and we
+    // could continue.
+    long long size = (long long)fBounds.height() * (long long)fRowBytes;
+    if (size >= 0xFFFFFFFFll / 2)
+        __debugbreak();
+
+    return size;
 }
Status: Started
Comment 10 by abarth@chromium.org, Apr 22, 2009
I wish we could express this in terms of MAX_INT or some kind of limits constant on 
size_t.  Also, does this have perf impact of the long long?
Comment 11 by abarth@chromium.org, Apr 22, 2009
My grammar fails me.  :)

What I mean is something like (Limits<size_t>::Max >> 2) or whatever you actually 
want.
Comment 12 by brettw@chromium.org, Apr 22, 2009
I thought Skia didn't use STL, but I found another file that used <limits>, so here's 
the new version:

+#include <limits>
+
 #include "SkMask.h"

 size_t SkMask::computeImageSize() const
 {
-    return fBounds.height() * fRowBytes;
+    // Prevent an integer overflow by killing the process. This can lead to an
+    // exploitable buffer overflow. Ideally, all callers would check for an
+    // error from this function and we could continue.
+    long long size = (long long)fBounds.height() * (long long)fRowBytes;
+    if (size >= std::numeric_limits<size_t>::max() / 2)
+        __debugbreak();
+
+    return size;
 }
Comment 13 by abarth@chromium.org, Apr 22, 2009
LGTM provided perf isn't a problem.  Can we check in BJ's test case too?
Comment 14 by brettw@chromium.org, Apr 22, 2009
This only gets called when creating a new bitmap in some certain circumstances (I think 
when it needs one for an effect). It doesn't get called very much at all, and even if 
it did, the perf would be covered up by mallocing a bunch of memory.
Comment 15 by lcamtuf, Apr 23, 2009
Since we already seem to be doing a cleanup of CANVAS implementation in this bug, you
might want to dupe issue 8864 against this one?
Comment 16 by lcamtuf, Apr 23, 2009
[I think BJ is using the same fuzzer, just ported to the fuzzing infrastructure]
Comment 17 by skylined@chromium.org, Apr 23, 2009
No, I've created a new one that uses templates to allow it to fuzz pretty much 
anything JavaScript can touch directly. I've implemented templates to fuzz all CANVAS 
features defined by w3.org and I'm working on other parts of HTML, such as IMG 
elements (these are used as arguments to some of the CANVAS methods) and HTML 
elements in general (The CANVAS element is an HTML element after all).

Comment 18 by r...@android.com, Apr 23, 2009
I will roll in a change into skia/trunk that fixes this as well, but handles the callers so we don't have to kill the 
process. I'll ping brettw when I have that CL.
Comment 19 by r...@android.com, Apr 23, 2009
BTW - I don't think the neg-check catches all overflows. It is only checking that bit 32 of the (possibly 64bit) 
result was set.
Labels: BTW
Comment 20 by abarth@chromium.org, Apr 23, 2009
Why's that?  I'm not that familiar with long long variables, but I thought the new >= 
would be a 64 bit comparison.
Comment 21 by r...@android.com, Apr 23, 2009
ah, you're right. I was still looking at this (older) version:

+    int result = (int)fBounds.height() * (int)fRowBytes;
+    if (result < 0)
+        return -1;


Comment 22 by abarth@chromium.org, Apr 23, 2009
What's the ETA for checking in this fix?
Comment 23 by abarth@chromium.org, Apr 23, 2009
(No comment was entered for this change.)
Cc: aba...@chromium.org
Comment 24 by mal.chromium, Apr 23, 2009
The patch in comment #12 looks fine for computeImageSize(), but what about 
computeTotalImageSize()?

I'm still worried about the size *= 3 call there.
Comment 25 by r...@android.com, Apr 24, 2009
BTW - skia/trunk has a fixed checked in. It is essentially the same as the above patch (it also checks the *3 
mulitply), but it has also update the call-sites that use the result to allocate memory, and aborts the drawing in 
those cases if we overflowed.

http://code.google.com/p/skia/source/detail?r=159

(it also has the additional porder-duff mode ADD)
Comment 26 by brettw@chromium.org, Apr 24, 2009
Thanks Mike!
Comment 27 by brettw@chromium.org, Apr 24, 2009
I'll make the *3 crash as well and check it in as normal (since Skia's fixed it 
upstream, there's no sense in keeping this seekret.
Comment 28 by brettw@chromium.org, Apr 24, 2009
Should be fixed in r14454.
Status: Fixed
Comment 29 by bugdroid1@chromium.org, Apr 24, 2009
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=14454 

------------------------------------------------------------------------
r14454 | brettw@chromium.org | 2009-04-24 12:50:03 -0700 (Fri, 24 Apr 2009) | 3 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/skia/sgl/SkMask.cpp?r1=14454&r2=14453

Fix a bug in the size computation for Skia masking.

BUG=10736
------------------------------------------------------------------------

Comment 30 by bugdroid1@chromium.org, Apr 25, 2009
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=14552 

------------------------------------------------------------------------
r14552 | mal@chromium.org | 2009-04-25 17:43:56 -0700 (Sat, 25 Apr 2009) | 5 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/release_154.next/src/skia/sgl/SkMask.cpp?r1=14552&r2=14551

Merge r14454 to 154.

Fix a bug in the size computation for Skia masking.

BUG=10736
------------------------------------------------------------------------

Comment 31 by bugdroid1@chromium.org, Apr 25, 2009
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=14553 

------------------------------------------------------------------------
r14553 | mal@chromium.org | 2009-04-25 17:46:51 -0700 (Sat, 25 Apr 2009) | 5 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/172/src/skia/sgl/SkMask.cpp?r1=14553&r2=14552

Merge r14454 to 172.

Fix a bug in the size computation for Skia masking.

BUG=10736
------------------------------------------------------------------------

Comment 32 by sunandt@chromium.org, Apr 27, 2009
http://skypher.com/SkyLined/Repro/Chrome/Issue%2010736%20-
%20SkMask%20computeImageSize()%20integer%20overflow/repro.html

Loading the above testcase still crashes the renderer in Chromium 2.0.177.0 (Developer 
Build 14622) and Google Chrome 1.0.154.61 (Official Build ) doesn't crash.
Cc: m...@chromium.org anan...@chromium.org
Comment 33 by brettw@chromium.org, Apr 27, 2009
the current fix in Chrome is to crash using a DebugBreak when this condition is 
detected, so you will still get a renderer crash.
Comment 34 by sunandt@chromium.org, Apr 29, 2009
Verified with Google Chrome 2.0.172.13. This crashes at DebugBreak in 
SkMask::computeImageSize instead of crashing at memset.
Comment 35 by sunandt@chromium.org, May 04, 2009
Works as expected with Google Chrome 2.0.172.18
Comment 36 by mal.chromium, May 05, 2009
154.64 has been released.
Labels: -private
Comment 37 by sunandt@chromium.org, May 07, 2009
Works as expected with Google Chrome 2.0.172.22
Sign in to add a comment