Update on buffer over-run bug in ATL::CPath functions - FDBK26503.

I reported a bug about a buffer over-run and security flaw in some of the ATL::CPath functions, bug FDBK26503. The bug was resolved as by design because the Microsoft developer who reviewed the bug did not do a thorough job of reading the online documentation for GetBuffer() and failed to follow the code path that determines when a reallocation is performed. Here is Microsoft's response to my bug report and my response to the Microsoft response.


Resolved as By Design by Microsoft on 2005-05-16 at 12:28:11 
   
Storage for the terminating null is automatically added to the requested size. This can be seen in CAfxStringMgr::Allocate and in CAfxStringMgr::Reallocate. There must be some other cause of the corruption. 

Edited by John Hensley on 2005-05-16 at 15:16:47 
   
Before reporting this problem I had carefully followed the code path for GetBuffer( int nMinBufferLength ) in atlsimpstr.h and found that the reallocation you are referring to will only occur if the current buffer length minus the value passed in nMinBufferLength is less than zero. The reallocation will not occur if the lengths are the same and this results in a buffer over-run when the terminating zero is appended by ::AppendPath() as I have reported in the initial bug report.

The determination of whether the buffer should be reallocated is made in PrepareWrite() and the reallocation is performed in PrepareWrite2(). You can clearly see from the PrepareWrite() source code that nTooShort will be zero if the existing buffer is equal in length to the specified value for nMinBufferLength.

int nTooShort = pOldData->nAllocLength - nLength;

If nTooShort is zero, the call to PrepareWrite2() will not be made and the buffer will be too small for the terminating zero.

PXSTR PrepareWrite( __in int nLength )
{
  CStringData* pOldData = GetData();

  // nShared < 0 means true, >= 0 means false
  int nShared = 1-pOldData->nRefs;

  // nTooShort < 0 means true, >= 0 means false
  int nTooShort = pOldData->nAllocLength - nLength;

  // If either sign bit is set (i.e. either is less than zero), we need to copy data
  if( (nShared | nTooShort) < 0 )
  {
  // This is where the realloc() will be performed!
  PrepareWrite2( nLength );
  }
  return( m_pszData );
}

The online documentation also confirms that terminating zero is not accounted for by the call to GetBuffer( int nMinBufferLength ). From the online documentation

“MinBufferLength - The minimum size of the character buffer in characters. This value does not include space for a null terminator.”

I hope that you will reopen this bug report so that this serious ongoing security flaw can be fixed in the released product.
 

BTW I am a former development lead from the Microsoft OS group and I'm a bit surprised at Microsoft's response to this security flaw report.

John Hensley