322215 - CVE-2006-0297 integer overflow in nsSVGCairoSurface::Init and nsCanvasRenderingContext2D

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[georgi - hopefully not receiving bugspam]

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]

Attachment: testcase

Comment 2

[georgi - hopefully not receiving bugspam]

tested on latest trunk.

Comment 3

[Daniel Veditz [:dveditz]]

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]

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] (needinfo me, slow to respond)]

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]

Attachment: check for overflow in svg surface creation

Comment 7

[Daniel Veditz [:dveditz]]

(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]

+  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] (needinfo me, slow to respond)]

Attachment: 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]

0xfffe * 0xfffe * 4 > 2^32.  I think we need smaller limits are needed (assuming we use that approach).

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Good point; how does 16k sound?  Even that's pretty generous.

Comment 12

[Daniel Veditz [:dveditz]]

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] (needinfo me, slow to respond)]

Attachment: 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] (needinfo me, slow to respond)]

Attachment: 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]]

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]

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]

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]

mTempBits = new PRUint8[tempSize];
^^^^^^^^^^^^^^^ this may throw exception with gcc, i believe.

Comment 19

[tor]

(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] (needinfo me, slow to respond)]

(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]

> 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]

Attachment: same check for svg surface creation

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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]

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]]

cairo and svg aren't in 1.0.x

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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]]

(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]]

Comment on attachment 207547 [details] [diff] [review]
reuse code from image frame for canvas size check

a=dveditz

Comment 29

[Daniel Veditz [:dveditz]]

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]

(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]

(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]]

> 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]

SVG surface size test checked in on trunk.

Comment 34

[Daniel Veditz [:dveditz]]

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]]

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]

Checked in on branch.

Comment 37

[Daniel Veditz [:dveditz]]

Moved bug from Firefox -> Core to get the right blocking flags back.

Comment 38

[Bob Clary [:bc] (inactive)]

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]

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]

i mean 1.5.0.1rc1 crashes.

Comment 41

[georgi - hopefully not receiving bugspam]

regarding CVE in the summary:

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

mitre's  "Steven M. Christey" <[e-mail]>
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]]

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] (inactive)]

(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] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/a2f6d39a6184