Last Comment Bug 322215 - CVE-2006-0297 integer overflow in nsSVGCairoSurface::Init and nsCanvasRenderingContext2D
: CVE-2006-0297 integer overflow in nsSVGCairoSurface::Init and nsCanvasRenderi...
Status: RESOLVED FIXED
[sg:critical] need svg fix on branch;...
: fixed1.8.0.1, fixed1.8.1, qawanted
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-03 08:46 PST by georgi - hopefully not receiving bugspam
Modified: 2009-04-24 11:24 PDT (History)
8 users (show)
dveditz: blocking1.7.13-
dveditz: blocking‑aviary1.0.8-
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (669 bytes, text/xml)
2006-01-03 08:47 PST, georgi - hopefully not receiving bugspam
no flags Details
check for overflow in svg surface creation (1.02 KB, patch)
2006-01-03 14:18 PST, tor
no flags Details | Diff | Splinter Review
322215-canvas-size-check.patch (2.32 KB, patch)
2006-01-04 12:02 PST, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Splinter Review
dropping canvas size to 16k (2.32 KB, patch)
2006-01-04 13:38 PST, Vladimir Vukicevic [:vlad] [:vladv]
no flags Details | Diff | Splinter Review
reuse code from image frame for canvas size check (3.05 KB, patch)
2006-01-04 14:00 PST, Vladimir Vukicevic [:vlad] [:vladv]
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review
same check for svg surface creation (2.12 KB, patch)
2006-01-05 11:39 PST, tor
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2006-01-03 08:46:07 PST
in nsSVGCairoSurface::Init 

  mData = new PRUint8[4*width*height];

this may overflow due to int too big.

potential exploit may be:
making a hole lower on the heap with as much memory above it, mData gets there
placing C objects in the upper heap, then a race from another thread
uses the fecked data. not sure though.

same construct:
nsCanvasRenderingContext2D.cpp

    nsAutoArrayPtr<PRUint8> tmpBuf(new PRUint8[aSurfaceSize.width*aSurfaceSize.height*4]);

testcase to follow.
Comment 1 georgi - hopefully not receiving bugspam 2006-01-03 08:47:55 PST
Created attachment 207421 [details]
testcase
Comment 2 georgi - hopefully not receiving bugspam 2006-01-03 08:49:33 PST
tested on latest trunk.
Comment 3 Daniel Veditz [:dveditz] 2006-01-03 13:55:18 PST
The code exists on the 1.8 branch, but I can only confirm the crash with the testcase on the trunk. One difference is the branch spits out a CSS error. The crash hits somewhat later, but appears to be while painting. Presumably you could trigger this code in another way on the branch.

fwiw I crash at linw 428 in nsSVGFilterFrame.cpp (FilterPaint method)
    for (PRUint32 yy=0; yy<filterResY; yy++)
      for (PRUint32 xx=0; xx<filterResX; xx++) {
-->      alphaData[stride*yy + 4*xx]     = 0;

with stride == 0x20004, yy=2, and xx=0x66ca (I think that's about 100K past the actual size of the buffer).
Comment 4 tor 2006-01-03 14:11:05 PST
Filters are a trunk-only thing, so that way of crashing it is out.

Ways to get into that code on the branch would be group opacity or <svg:image>.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-03 14:17:58 PST
Is there a known issue/crash with the nsCanvasRenderingContext2D tmpBuf usage?   It seems correct, and the loops are bounded by the same variables that were used to create the buffer.
Comment 6 tor 2006-01-03 14:18:17 PST
Created attachment 207440 [details] [diff] [review]
check for overflow in svg surface creation
Comment 7 Daniel Veditz [:dveditz] 2006-01-03 22:48:24 PST
(In reply to comment #5)
> Is there a known issue/crash with the nsCanvasRenderingContext2D tmpBuf usage?

The code is wrong by inspection. How can I tickle that code to prove it crashes? (there may be limits elsewhere that weed out the bad values such as the ones in gfxImageFrame::Init() -- do those come into play with canvas?)
 
> the loops are bounded by the same variables that were
> used to create the buffer.

But the loop bounds don't overflow while the buffer creation does.
posit a width of 32052 and a height of 33500: the allocated buffer will be 704 bytes. Any loops bounded by 30K will stomp all over things.
Comment 8 georgi - hopefully not receiving bugspam 2006-01-04 08:04:54 PST
+  if (tmp / height != width)
+    return NS_ERROR_FAILURE;

this seems to work in this case, but in the general case may lead to division by 0.
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-04 12:02:12 PST
Created attachment 207538 [details] [diff] [review]
322215-canvas-size-check.patch

Patch for canvas; just doesn't allow width or height >= 0xffff.  This is still pretty generous and could leave to OOM conditions, but those should be handled (barring throw-on-OOM vs. return NULL issues...)
Comment 10 Jesse Ruderman 2006-01-04 12:09:04 PST
0xfffe * 0xfffe * 4 > 2^32.  I think we need smaller limits are needed (assuming we use that approach).
Comment 11 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-04 12:22:43 PST
Good point; how does 16k sound?  Even that's pretty generous.
Comment 12 Daniel Veditz [:dveditz] 2006-01-04 13:06:06 PST
Comment on attachment 207538 [details] [diff] [review]
322215-canvas-size-check.patch

0x7fff would be a safe value using this approach, but r- on ffff
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-04 13:38:51 PST
Created attachment 207544 [details] [diff] [review]
dropping canvas size to 16k

Drop the sizes down to 16k (I originally meant to do 32k even, not sure why I ended up at 64k.).  This avoids all the *4 problems and is also at the upper bound of "sane size" anyway, since a 16kx16k rendering would require 1GB of memory...
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-04 14:00:14 PST
Created attachment 207547 [details] [diff] [review]
reuse code from image frame for canvas size check

Ok, this reuses the same code from http://lxr.mozilla.org/mozilla1.8/source/gfx/src/shared/gfxImageFrame.cpp#62 -- we really should pull this code out somewhere so that SVG, canvas, image frame, and whatever else can share it, but I have no idea where that common place should be (or if we even want to do that for 1.8.0.1, or just make it nice on the trunk).
Comment 15 Daniel Veditz [:dveditz] 2006-01-04 17:18:28 PST
Comment on attachment 207547 [details] [diff] [review]
reuse code from image frame for canvas size check

r=dveditz
Comment 16 georgi - hopefully not receiving bugspam 2006-01-05 03:19:58 PST
trying to allocate about 3GB terminates mozilla (though there is no overflow).

terminate called after throwing an instance of 'std::bad_alloc'
  what():  St9bad_alloc

#3  0xb73c8049 in abort () from /lib/tls/libc.so.6
#4  0xb7579d67 in __gnu_cxx::__verbose_terminate_handler ()
   from /usr/lib/libstdc++.so.6
#5  0xb75778f5 in __cxa_call_unexpected () from /usr/lib/libstdc++.so.6
#6  0xb7577932 in std::terminate () from /usr/lib/libstdc++.so.6
#7  0xb7577ab2 in __cxa_throw () from /usr/lib/libstdc++.so.6
#8  0xb7577d02 in operator new () from /usr/lib/libstdc++.so.6
#9  0xb7577dbd in operator new[] () from /usr/lib/libstdc++.so.6
#10 0x086e47d6 in nsSVGCairoSurface::Init (this=0x9616d30, width=1, 
    height=805306368)
Comment 17 georgi - hopefully not receiving bugspam 2006-01-05 03:54:39 PST
nsSVGLibartBitmapAlpha::Init(nsIRenderingContext* ctx,
                               nsPresContext* presContext,
                               const nsRect & rect)

  mTempLineStride = rect.width * 4; // i.e. 4-bytes per pixel (RGBA)
  mTempLineStride = (mTempLineStride + 3) & ~0x3; // 32-bit align
  tempSize = mTempLineStride * rect.height;
  mTempBits = new PRUint8[tempSize];
  if (!mTempBits) return NS_ERROR_OUT_OF_MEMORY;

i am not sure this is safe, but don't know how to trigger it.
Comment 18 georgi - hopefully not receiving bugspam 2006-01-05 03:56:06 PST
mTempBits = new PRUint8[tempSize];
^^^^^^^^^^^^^^^ this may throw exception with gcc, i believe.

Comment 19 tor 2006-01-05 07:28:45 PST
(In reply to comment #17)
> nsSVGLibartBitmapAlpha::Init(nsIRenderingContext* ctx,
>                                nsPresContext* presContext,
>                                const nsRect & rect)

The libart SVG backend is unbuilt, depreciated code.
Comment 20 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-05 07:38:13 PST
(In reply to comment #18)
> mTempBits = new PRUint8[tempSize];
> ^^^^^^^^^^^^^^^ this may throw exception with gcc, i believe.

All of these allocations can (should?) be switched to PR_malloc, no?
Comment 21 georgi - hopefully not receiving bugspam 2006-01-05 08:32:49 PST
> All of these allocations can (should?) be switched to PR_malloc, no?
> 

probably to:
a=new(std::nothrow) char[1024*1024*1024*3];

don't know how portable is, though.

better limit numbers imho.
Comment 22 tor 2006-01-05 11:39:09 PST
Created attachment 207642 [details] [diff] [review]
same check for svg surface creation
Comment 23 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-05 15:01:17 PST
Canvas patch checked in on trunk.  (I noticed that gfxImage* and imagelib stuff uses new instead of PR_malloc, so I gave up on making the new->malloc changes in canvas.)
Comment 24 georgi - hopefully not receiving bugspam 2006-01-06 08:11:07 PST
nsresult nsImageWin::Init(PRInt32 aWidth, PRInt32 aHeight, PRInt32 aDepth,
                          nsMaskRequirements aMaskRequirements)
  const PRInt32 k64KLimit = 0x0000FFFF;
  if (aWidth > k64KLimit || aHeight > k64KLimit)
    return NS_ERROR_FAILURE;

  mImageBits = new unsigned char[mSizeImage];

this may also eventually overflow.

GTK image has lower checks.

Comment 25 Daniel Veditz [:dveditz] 2006-01-06 19:32:42 PST
cairo and svg aren't in 1.0.x
Comment 26 Vladimir Vukicevic [:vlad] [:vladv] 2006-01-07 13:09:36 PST
Ugh crap, I just landed the canvas bits on 1.8/1.8.0 along with the other two canvas bugs; thought I had approval for this bit as well.  Let me know if I should back this canvas bit out.
Comment 27 Daniel Veditz [:dveditz] 2006-01-07 16:09:34 PST
(In reply to comment #24)
> nsresult nsImageWin::Init [...] may also eventually overflow.
> GTK image has lower checks.

all "@mozilla.org/gfx/image;1" including nsImageWin are instantiated from gfxImageFrame.cpp which has already checked the size for overflow first.

Almost all: I did find an extra use of nsImageWin from nsImageClipboard.cpp. It has its own problems so I've filed bug 322708 for the clipboard and nsImageWin issue.

This bug is already covering two independent instances of this construct, let's not add more because tracking fixes and QA gets cumbersome and more likely to miss one.

(In reply to comment #19)
> The libart SVG backend is unbuilt, depreciated code.

Can we "cvs rm" it so it doesn't clutter up these sorts of scans for problematic patterns?
Comment 28 Daniel Veditz [:dveditz] 2006-01-07 16:10:02 PST
Comment on attachment 207547 [details] [diff] [review]
reuse code from image frame for canvas size check

a=dveditz
Comment 29 Daniel Veditz [:dveditz] 2006-01-07 16:13:44 PST
Comment on attachment 207642 [details] [diff] [review]
same check for svg surface creation

>+    if (width <= 0 || height <= 0)

These can't be negative, but I guess it can't hurt in case someone later changes the types and forgets to update the checks.

r=dveditz
Comment 30 georgi - hopefully not receiving bugspam 2006-01-08 00:47:50 PST
(In reply to comment #29)
> (From update of attachment 207642 [details] [diff] [review] [edit])
> >+    if (width <= 0 || height <= 0)
> 
> These can't be negative, but I guess it can't hurt in case someone later
> changes the types and forgets to update the checks.
> 

they cover the nonpositive (zero case) which may leave to div by 0.

sorry for filing several problems in one bug, but i wasn't sure they were reachable/exploitable, so basically it was request for comments.

Comment 31 georgi - hopefully not receiving bugspam 2006-01-08 00:54:42 PST
(In reply to comment #27)
> all "@mozilla.org/gfx/image;1" including nsImageWin are instantiated from
> gfxImageFrame.cpp which has already checked the size for overflow first.
> 

imho functions/methods should be as "self contained" as possible, i.e. making the necessary overflows checks because:
1. this makes analysis much easier (no need to check for all invokations)
2. prevents someone from introducing buggy code (missing checks) in the future

probably an exception may be very performance critical code.

> Almost all: I did find an extra use of nsImageWin from nsImageClipboard.cpp. It
> has its own problems so I've filed bug 322708 for the clipboard and nsImageWin
> issue.
> 

dveditz: what is the correct way to determine all invocation of methods?
are you sure grep/lxr are sufficient?

> This bug is already covering two independent instances of this construct, let's
> not add more because tracking fixes and QA gets cumbersome and more likely to
> miss one.
> 
> (In reply to comment #19)
> > The libart SVG backend is unbuilt, depreciated code.
> 
> Can we "cvs rm" it so it doesn't clutter up these sorts of scans for
> problematic patterns?

agreed. or at least documenting dead code.

Comment 32 Daniel Veditz [:dveditz] 2006-01-08 02:25:30 PST
> dveditz: what is the correct way to determine all invocation of methods?
> are you sure grep/lxr are sufficient?

In all but the simple cases grep probably is *not* sufficient. There's direct C++ class use, CreateInstance/Service using the CID (probably a single macro) or using the contractids (sometimes used literally, sometimes multiple macros), use from javascript.

nsImageWin seems to be one of the simple cases.
Comment 33 tor 2006-01-09 16:12:01 PST
SVG surface size test checked in on trunk.
Comment 34 Daniel Veditz [:dveditz] 2006-01-09 16:44:41 PST
Comment on attachment 207642 [details] [diff] [review]
same check for svg surface creation

a=dveditz, discussed in this morning's triage mtg.
Comment 35 Daniel Veditz [:dveditz] 2006-01-10 14:41:02 PST
Both parts of this bug are fixed on trunk, marking bug so.

tor: when you land the svg patch on the 1.8 and 1.8.0 branches please add the fixed1.8.1 and fixed1.8.0 keywords.
Comment 36 tor 2006-01-10 15:01:26 PST
Checked in on branch.
Comment 37 Daniel Veditz [:dveditz] 2006-01-10 16:16:24 PST
Moved bug from Firefox -> Core to get the right blocking flags back.
Comment 38 Bob Clary [:bc:] 2006-01-13 01:51:03 PST
no crash on windows for firefox 1.5 20051111, 1.8.0.1, 1.8.1, 1.9a1 20060112
no crash on linux firefox 1.5 20051111, 1.8.0.1, 1.8.1, 1.9a1 20060112

since I couldn't reproduce the original crash on 1.5 20051111 i won't verify.
Comment 39 georgi - hopefully not receiving bugspam 2006-01-19 05:18:57 PST
the canvas stuff may be related to Bug 324005.

large width/height still crash branch, trunk seems to accept the canvas.
Comment 40 georgi - hopefully not receiving bugspam 2006-01-19 05:19:44 PST
i mean 1.5.0.1rc1 crashes.
Comment 41 georgi - hopefully not receiving bugspam 2006-01-27 06:24:08 PST
regarding CVE in the summary:

i consider mitre/CVE corporate/government's puppies and have low opinion of
them.

mitre's  "Steven M. Christey" <coley@linus.mitre.org>
co-authored the "responsible disclosure draft rfc" which was
obviously a corporate plot and was ditched by even the IETF - check the
ietf archives for this corporate fiasco.
Comment 42 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2007-03-16 18:30:41 PDT
bclary, you marked this as in‑testsuite+ - can you tell me which test suite and which test covers this bug?
Comment 43 Bob Clary [:bc:] 2007-03-19 11:05:14 PDT
(In reply to comment #42)
> bclary, you marked this as in‑testsuite+ - can you tell me which test suite
> and which test covers this bug?

It was in an internal only suite that will not be continued forward.

Comment 44 Bob Clary [:bc:] 2009-04-24 11:24:11 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/a2f6d39a6184

Note You need to log in before you can comment on or make changes to this bug.