598523 - mozGetAsFile Buffer overrun

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[David Chan [:dchan]]

nsHTMLCanvasElement::ExtractData uses the wrong variable to resize a buffer. This results in ExtractData writing off the end of the buffer

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#278

PR_Realloc should be passing in bufSize instead of aSize

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Doh!

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Thanks for catching this David!

Jonas, is there a way to write a test for this that would fail reliably?  A test that tried to toDataURL a ridiculously big canvas should cause an access violation eventually on all platforms, right?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 477367 [details] [diff] [review]
Patch

The problem is that this only kicks in if Available returns a number smaller than what is actually available, right? I don't know if any of the current encoders produce streams where that is the case.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ah, good point.  No way to test this then.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 477367 [details] [diff] [review]
Patch

I'm not sure if security bugs still require sr, people are telling me different things.

Comment 6

[Daniel Veditz [:dveditz]]

As long as we're here... is there anything preventing imgStream from reporting 4Gb available? Obviously it would take a beefy system, but if Available() can return just shy of 4Gb then the padding causes an integer overflow.

260   PRUint32 bufSize;
261   rv = imgStream->Available(&bufSize);
262   NS_ENSURE_SUCCESS(rv, rv);
263 
264   // ...leave a little extra room so we can call read again and make sure we
265   // got everything. 16 bytes for better padding (maybe)
266   bufSize += 16;

And of course bufSize *= 2 overflows at anything over 2Gb.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Overflowing on the += 16 doesn't cause anything interesting to happen in and of itself.  The bufSize *= 2 is more interesting.  That leads to a bad place (we'll shrink the buffer, then we'll try to read and bufSize - aSize will be negative and will become a large positive number).

As sicking points out, the resize loop isn't actually hit under normal conditions, but it could be hit if you get the += 16 to overflow.

Note that this would be a problem on older branches too, this code isn't new, it just moved around.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 477367 [details] [diff] [review]
Patch

Lets just rip the resizing code out all together.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Lets do this instead.  Because the encoding should be done by the time we get to this part of the code, we allocate for a buffer based on what we're told, and we do a debug only check (because failure causes a buffer overrun) that the encoder didn't lie to us.

Comment 10

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

Comment on attachment 479174 [details] [diff] [review]
Patch

Well, the initial problem is that that won't compile :)  ("debugcheck" vs. "debugCheck").  Other than that, I'm not sure how Read behaves when you read off the end of the stream; does it really return NS_OK?  Is there some call where you can just assert that the stream is at its end?

Not sure if you need the #ifdef DEBUG block at all, really; other things will break if an encoder decides to be async.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

EOF is signaled by NS_OK with an outparam value of 0.  There's no other way to check for EOF on an nsIInputStream.

I'm fine with losing the debug check.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This isn't an issue of sync vs. async. There is no guarantees that Available will report the full amount of data even if it is synchronously available. It might even be the case that some of our buffering streams has this behavior though I'm less sure about that.

So I don't think we can do this unfortunately.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 479174 [details] [diff] [review]
Patch

Minusing based on that.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #12)
> This isn't an issue of sync vs. async. There is no guarantees that Available
> will report the full amount of data even if it is synchronously available. It
> might even be the case that some of our buffering streams has this behavior
> though I'm less sure about that.
> 
> So I don't think we can do this unfortunately.

But all we have to care about are the image encoders, which don't do any of that.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Lets try this then.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also, we should file a followup on factoring this out into nsNetUtils.h or something.  I'm sure we do this (empty an input stream) in other places and we probably have similar bugs lurking there.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/8d04493d2046

If we're worried about the integer overflow stuff on the stable branches, we should file a separate bug.  The fix would be a lot different because CheckedInt.h is trunk only.