373899 - URL Parser integer overflow in memory calculation

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

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.  
2. 
3. 
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.

Comment 1

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

confirming... sounds like this may happen.

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 diff

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.

Comment 4

[Honza Bambas (:mayhemer)]

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


r-

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Updated diff

Thanks for the comments, Honza.  I misunderstood NS_ASSERTION - didn't realize it was disabled in production releases.  Diff updated to reflect your suggestions.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 561316 [details] [diff] [review]
Updated diff

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

r=honzab

::: 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.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: Minor updates based on Honza's 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.

Comment 8

[Josh Aas]

pushed to mozilla-inbound for Steve

https://hg.mozilla.org/integration/mozilla-inbound/rev/af88e531499d

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/mozilla-central/rev/af88e531499d