User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:126.96.36.199) 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
I meant "Reproducible: Haven't tried yet"
Created attachment 499345 [details] [diff] [review] Check for overflow before allocating memory Woopie! My first patch!
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? :)
(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.
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.
Opening this up, and marking invalid.