Possible allocation size overflow in nsTextFragment::Append

RESOLVED INVALID

Status

()

Core
Layout
RESOLVED INVALID
7 years ago
6 years ago

People

(Reporter: Alex Miller, Assigned: Alex Miller)

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:needinfo], URL)

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: 

Yes, I actually did some research before filing this one. If an attacker can provide a string of substantial length (it can vary), it is possible to overflow the allocation size without overflowing the string length, resulting in a buffer too small for the provided string. Also, because of the fact that the value that may overflow is explicitly UInt32, this problem can be triggered more easily on 64-bit systems.

Reproducible: Couldn't Reproduce
(Assignee)

Comment 1

7 years ago
I meant "Reproducible: Haven't tried yet"
(Assignee)

Updated

7 years ago
Summary: Allocation size overflow in nsTextFragment::Append → Possible allocation size overflow in nsTextFragment::Append
(Assignee)

Updated

7 years ago
Version: unspecified → Trunk
Assignee: nobody → alexander.miller
(Assignee)

Comment 2

7 years ago
Created attachment 499345 [details] [diff] [review]
Check for overflow before allocating memory

Woopie! My first patch!
Attachment #499345 - Flags: review?(jst)
Attachment #499345 - Flags: approval2.0?
Attachment #499345 - Flags: approval1.9.2.14?
(Assignee)

Updated

7 years ago
Attachment #499345 - Flags: approval2.0?
Attachment #499345 - Flags: approval1.9.2.14?
Good first patch, Alex! :)

But I do suspect this problem *may* not be triggerable from unsafe code (i.e. by manipulating the DOM from JS) due to the CanGrowBy() check in nsGenericDOMDataNode::SetTextInternal(). If I'm correct, we're ok without this patch, if I'm wrong, we should take this change but use something like nsTextFragment::CanGrowBy() at allocation sites since an nsTextFragment can't actually hold PR_UINT32_MAX characters (it has a length limit of 29 bits, see the definition of mLength in nsTextFragment.h).
Alex, any thoughts on my ramblings above? :)
Whiteboard: [sg:needinfo]
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)
> Alex, any thoughts on my ramblings above? :)

My only thought is if aLength is (2^32 - 2^29) bytes long, an overflow would be possible.

Comment 6

7 years ago
alex: fwiw, there should be a space between 'if' and '(' and we don't use <tab>s in our code
Comment on attachment 499345 [details] [diff] [review]
Check for overflow before allocating memory

Clearing review request here since this is not a change that we need. Manual code auditing and dxr says the only caller of nsTextFragment::Append() is nsGenericDOMData::SetTextInternal(), and it already checks that the fragment can grow to the requested size, which guarantees that there's no overflow here.
Attachment #499345 - Flags: review?(jst)
Opening this up, and marking invalid.
Group: core-security
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.