Closed Bug 373899 Opened 15 years ago Closed 10 years ago

URL Parser integer overflow in memory calculation


(Core :: Networking, defect)

Not set





(Reporter: msg, Assigned: sworkman)


(Whiteboard: [sg:moderate] check other ints in the file; need trunk fix)


(1 file, 2 obsolete files)

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

nsStandardURL::AppendToSubstring() Does not check for possiable intager overflow in it's memory allocation.  The input can be generated from several places and should be protected.

Reproducible: Didn't try

Steps to Reproduce:
1. Pass in a sufficiently large string such that the integer calculation in AppendToSubstring will succeed.  
Actual Results:  
len and tailLen and the number 1 are all accumulated in a malloc.  They should be calculated separately and analyzed for there validity.

Any time dynamic memory is calculated, be weary of performing any calculations in the context of the argument for the request.  Calculate the request seporately from the request.
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Summary: URL Parser intager overflow in memory calculation → URL Parser integer overflow in memory calculation
confirming... sounds like this may happen.
Ever confirmed: true
This routine is internal to nsStandardURL and most of the time is just munging around the URL we already have. In the unlikely event we didn't die on url strings that long you could trigger the interger overflow by having a really long relative URL appended to a really long base (no user would download a multi-gigabyte file, but it could be served gzipped).

With strings that long I worry about the mixture of unsigned position and signed int lengths elsewhere in this file.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Whiteboard: [sg:moderate?] check other ints in the file
Assignee: nobody → dveditz
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Flags: blocking1.8.1.8+
Flags: blocking1.8.0.14+
Whiteboard: [sg:moderate?] check other ints in the file → [sg:moderate?] check other ints in the file; need trunk fix
Assignee: dveditz → nobody
Whiteboard: [sg:moderate?] check other ints in the file; need trunk fix → [sg:moderate] check other ints in the file; need trunk fix
Assignee: nobody → sjhworkman
Attached patch v1.0 diff (obsolete) — Splinter Review
Added assertions to check for valid values of pos and length, for the length of the mSpec string and the tail str to be appended.

Checked rest of file (memcpys and allocs) for mix of PRUnit32 and PRInt32 - seems ok to me.
Attachment #561087 - Flags: review?
Attachment #561087 - Flags: review? → review?(honzab.moz)
Comment on attachment 561087 [details] [diff] [review]
v1.0 diff

Review of attachment 561087 [details] [diff] [review]:

First, this patch doesn't change a bit in the production optimized build, so it is not a fix for this security bug.  NS_Alloc takes PRSize which is unsigned, so any negative number in PRInt32 newLen is converted to an unsigned int resulting in an unexpected amount of allocated memory.

Second, I can see we use URLSegment::mLen, that is PRInt32 (signed), on a lot of places and assign or convert it silently to unsigned int almost everywhere in the file.

IMO best would be to change mLen member to PRUint32 or even better to PRSize and have an additional flag to indicate the segment is valid.  Now we check a segment keeps any data by examining mLen be a non-negative number.

This is a large overhaul, and might be risky if we don't have tests to check the functionality.

So, the fix for this one particular bug could be:
- remove tailLen argument, it is never used
- return NULL when len < 0
- return NULL when tail is NULL
- store result of strlen(tail) to an unsigned int (say PRSize tailLen)
- return NULL when mSpec.Length() < pos or when mSpec.Length() - pos < len (actually, your two assertions, but turn to be fatal)
- return NULL when PR_UINT32_MAX - (PRUint32)len <= tailLen

Attachment #561087 - Flags: review?(honzab.moz) → review-
Attached patch Updated diff (obsolete) — Splinter Review
Thanks for the comments, Honza.  I misunderstood NS_ASSERTION - didn't realize it was disabled in production releases.  Diff updated to reflect your suggestions.
Attachment #561087 - Attachment is obsolete: true
Attachment #561316 - Flags: review?(honzab.moz)
Comment on attachment 561316 [details] [diff] [review]
Updated diff

Review of attachment 561316 [details] [diff] [review]:


::: netwerk/base/src/nsStandardURL.cpp
@@ +869,5 @@
> +
> +    PRUint32 tailLen = strlen(tail);
> +
> +    // Check for int overflow for proposed length of combined string
> +    if (PR_UINT32_MAX - ((PRUint32)len + 1) <= tailLen)

You don't need |+ 1| here.

We need len + tailLen + 1.  PR_UINT32_MAX - len is what is left for tailLen, but we need one more byte.  Therefor, if PR_UINT32_MAX - len is equal tailLen, we won't fit that one more byte already.  |<=| operator gives this check to you.
Attachment #561316 - Flags: review?(honzab.moz) → review+
Thanks for the comment Honza.  Rather than use |<=| I used |<| and |+1|.  The logic is the same, and although |<=| might look cleaner, having the |+1| should make it very clear that this verification is related to the calculation in NS_Alloc() a couple of lines later.  Submitting the diff to the try server.
Attachment #561316 - Attachment is obsolete: true
Attachment #561342 - Flags: review+
Closed: 10 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.