529648 - Potential integer overflow in nsEntryStack::EnsureCapacityFor

Status: RESOLVED DUPLICATE of bug 496779

(Core :: DOM: HTML Parser, defect)

Description

[Drew Yao]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_2; en-us) AppleWebKit/531.21.8 (KHTML, like Gecko) Version/4.0.4 Safari/531.21.10
Build Identifier: 

parser/htmlparser/src/nsDTDUtils.cpp

void nsEntryStack::EnsureCapacityFor(PRInt32 aNewMax,PRInt32 aShiftOffset) {
  if(mCapacity<aNewMax){ 

    const int kDelta=16;

    PRInt32 theSize = kDelta * ((aNewMax / kDelta) + 1);
    nsTagEntry* temp=new nsTagEntry[theSize]; 
    mCapacity=theSize;

    if(temp){ 
      PRInt32 index=0; 
      for(index=0;index<mCount;++index) {
        temp[aShiftOffset+index]=mEntries[index];
      }
      if(mEntries) delete [] mEntries;
      mEntries=temp;
    }

If theSize * sizeof(nsTagEntry) would overflow, an unexpectedly small buffer will be allocated, leading to a heap buffer overflow.   Note: some compilers detect integer overflow in the new[] operator; gcc does not.

It seems like it would be possible for aNewMax to be an arbitrarily large value with a maliciously crafted web page but I haven't tried to reproduce.


Reproducible: Didn't try

Comment 1

[Drew Yao]

This appears to affect 1.9.1 and 1.9.2.

Also it would be important to check if new[] returns NULL.  If the new[] fails, it just continues with the old mEntries which hasn't been increased in size, leading to memory corruption.

e.g, 

void nsEntryStack::Append(nsEntryStack *aStack) {
  if(aStack) {

    PRInt32 theCount=aStack->mCount;

    EnsureCapacityFor(mCount+aStack->mCount,0);  //size of mEntries not increased

    PRInt32 theIndex=0;
    for(theIndex=0;theIndex<theCount;++theIndex){
      mEntries[mCount]=aStack->mEntries[theIndex];   // buffer overflow

Comment 2

[Daniel Veditz [:dveditz]]

I'm not sure we could construct a crashing testcase that didn't make us fall over somewhere else first, but if we could this code is definitely wrong.

On most compilers we're using failing in "new" will throw and we crash relatively safely (re: comment 1).

Comment 3

[Daniel Veditz [:dveditz]]

Although there's no explicit check in nsEntryStack itself, CNavDTD does make sure there are never more than MAX_REFLOW_DEPTH (200) entries. We could add a double-check, but we're nowhere near an integer overflow.

The missing OOM checks will result in a crash because new throws in all our current compilers, but that's covered in bug 496779