nsMultiMixedConv::OnDataAvailable integer overflow in memory allocation

RESOLVED FIXED in mozilla9

Status

()

defect
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: msg, Assigned: sworkman)

Tracking

unspecified
mozilla9
x86
Other
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate], )

Attachments

(1 attachment)

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)
You meant the line of code, I just pasted in the url, right?
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Summary: nsMultiMixedConv.coo Line 420 intager overflow in memory allocation → nsMultiMixedConv.cpp Line 420 integer overflow in memory allocation
yes, that's the one
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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)
Updating title, referring to a bug solely by the source line is not a good idea.
Summary: nsMultiMixedConv.cpp Line 420 integer overflow in memory allocation → nsMultiMixedConv::OnDataAvailable integer overflow in memory allocation
Whiteboard: [sg:moderate?] → [sg:moderate]
Assignee: nobody → sjhworkman
Posted patch v1.0 diffSplinter Review
Attachment #561097 - Flags: review?(honzab.moz)
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.
Attachment #561097 - Flags: review?(honzab.moz) → review+
Thanks, Honza.  I've submitted the patch to the try server.
https://hg.mozilla.org/mozilla-central/rev/682459dc5a1c
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.