373900 - nsMultiMixedConv::OnDataAvailable integer overflow in memory allocation

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[lsg-mtso]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.2) Gecko/20070225 BonEcho/2.0.0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.2) Gecko/20070225 BonEcho/2.0.0.2

the initial allocation of buffer does not check for integer overflow.  Ensure when performing dynamic memory allocations, the size requested does not wrap an integer boundry, as the request will be inconstant.

Reproducible: Didn't try

Steps to Reproduce:
1. Check for overflow.
2.
3.



Memory requests should be consistent with there use.  count + mBufLen is not the same as count or mBufLen individually (when they are not checked for validity)

Comment 1

[Martijn Wargers (dead)]

You meant the line of code, I just pasted in the url, right?

Comment 2

[lsg-mtso]

yes, that's the one

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm. seems somewhat hard to exploit this... I guess if you send 4 GB of header data, or a 4 GB long boundary you could exploit this...

ok, confirming

Comment 4

[Daniel Veditz [:dveditz]]

We wouldn't accept a header line 4Gb long (nsHttpTransaction.cpp rejects header lines over 10K) but the boundary line could be in the body.

Updating URL to be safe from future file revisions (there was already an 18 line drift from discovery to comment 1)

Comment 5

[Josh Aas]

Updating title, referring to a bug solely by the source line is not a good idea.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 diff

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 561097 [details] [diff] [review]
v1.0 diff

Review of attachment 561097 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/streamconv/converters/nsMultiMixedConv.cpp
@@ +459,5 @@
>      // fill buffer
>      {
>          bufLen = count + mBufLen;
> +        NS_ENSURE_TRUE((bufLen >= count) && (bufLen >= mBufLen),
> +                       NS_ERROR_FAILURE);

I would a tiny bit rather see the check as (PR_UINT32_MAX - count >= mBufLen) but this works the same way and is type-independent.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Thanks, Honza.  I've submitted the patch to the try server.

Comment 9

[Josh Aas]

pushed to mozilla-inbound for Steve

https://hg.mozilla.org/integration/mozilla-inbound/rev/682459dc5a1c

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/mozilla-central/rev/682459dc5a1c