mozGetAsFile Buffer overrun

RESOLVED FIXED

Status

()

Core
Canvas: 2D
--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dchan, Assigned: khuey)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][critsmash:patch])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
Doh!
Assignee: nobody → khuey
Created attachment 477367 [details] [diff] [review]
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?
Attachment #477367 - Flags: review?(jonas)
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.
Attachment #477367 - Flags: review?(jonas) → review+
Ah, good point.  No way to test this then.
Flags: in-testsuite-
Comment on attachment 477367 [details] [diff] [review]
Patch

I'm not sure if security bugs still require sr, people are telling me different things.
Attachment #477367 - Flags: superreview?(jst)
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.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: [sg:critical]
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 on attachment 477367 [details] [diff] [review]
Patch

Lets just rip the resizing code out all together.
Attachment #477367 - Attachment is obsolete: true
Attachment #477367 - Flags: superreview?(jst)

Updated

8 years ago
Whiteboard: [sg:critical] → [sg:critical][critsmash:patch]
Created attachment 479174 [details] [diff] [review]
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.
Attachment #479174 - Flags: superreview?(jonas)
Attachment #479174 - Flags: review?(vladimir)
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.
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.
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 on attachment 479174 [details] [diff] [review]
Patch

Minusing based on that.
Attachment #479174 - Flags: superreview?(jonas) → superreview-
(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.
Attachment #479174 - Attachment is obsolete: true
Attachment #479174 - Flags: review?(vladimir)
Created attachment 480546 [details] [diff] [review]
Patch

Lets try this then.
Attachment #480546 - Flags: superreview?(jonas)
Attachment #480546 - Flags: review?(vladimir)
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.
Attachment #480546 - Flags: superreview?(jonas) → superreview+
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.