URL Parser integer overflow in memory calculation

RESOLVED FIXED in mozilla9

Status

()

RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: msg, Assigned: sworkman)

Tracking

unspecified
mozilla9
x86
All
Points:
---
Bug Flags:
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.

Updated

12 years ago
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.
Status: UNCONFIRMED → NEW
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

Updated

7 years ago
Assignee: dveditz → nobody

Updated

7 years ago
Whiteboard: [sg:moderate?] check other ints in the file; need trunk fix → [sg:moderate] check other ints in the file; need trunk fix

Updated

7 years ago
Assignee: nobody → sjhworkman
(Assignee)

Comment 3

7 years ago
Created attachment 561087 [details] [diff] [review]
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.
Attachment #561087 - Flags: review?

Updated

7 years ago
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


r-
Attachment #561087 - Flags: review?(honzab.moz) → review-
(Assignee)

Comment 5

7 years ago
Created attachment 561316 [details] [diff] [review]
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.
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]:
-----------------------------------------------------------------

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.
Attachment #561316 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 7

7 years ago
Created attachment 561342 [details] [diff] [review]
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.
Attachment #561316 - Attachment is obsolete: true
Attachment #561342 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/af88e531499d
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

3 years ago
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.