CVE-2018-12584: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: The ConnectionBase::preparseNewBytes function in resip/stack/ConnectionBase.cxx in reSIProcate through 1.10.2 allows remote attackers to cause a denial of service (buffer overflow) or possibly execute arbitrary code when TLS communication is enabled.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 9.8
Published: 2018-07-16T20:29Z
Repository: https://github.com/resiprocate/resiprocate
Language: Other
Files changed: resip/stack/ConnectionBase.cxx
Commit date: 2018-06-21 10:24:38 -0400
Commit message: -fixed an issue where the stack could crash if a TCP based sender sends a Content-Length that is less that the actual SIP body transmitted
 in order to trigger this condition the SIP message body must arrive in a separate read request, either from the start or part way through the body
 and there must be data following the body in the same read.

Diff: diff --git a/resip/stack/ConnectionBase.cxx b/resip/stack/ConnectionBase.cxx
index 8e7c69a82..2ee7663d0 100644
--- a/resip/stack/ConnectionBase.cxx
+++ b/resip/stack/ConnectionBase.cxx
@@ -395,11 +395,11 @@ ConnectionBase::preparseNewBytes(int bytesRead)
                   mBuffer = newBuffer;
                   mBufferPos = 0;
                   mBufferSize = size;
                   
                   DebugLog (<< "Extra bytes after message: " << overHang);
-                  DebugLog (<< Data(mBuffer, overHang));
+                  //DebugLog (<< Data(mBuffer, overHang));
                   
                   bytesRead = overHang;
                }
 
                // The message body is complete.
@@ -469,15 +469,40 @@ ConnectionBase::preparseNewBytes(int bytesRead)
             //.jacob. Shouldn't the state also be set here?
             return false;
          }
 
          mBufferPos += bytesRead;
-         if (mBufferPos == contentLength)
+         if (mBufferPos >= contentLength)
          {
+            int overHang = mBufferPos - (int)contentLength;
+            char *overHangStart = mBuffer + contentLength;
+
             mMessage->addBuffer(mBuffer);
             mMessage->setBody(mBuffer, (UInt32)contentLength);
-            mBuffer=0;
+            mConnState = NewMessage;
+            mBuffer = 0;
+
+            if (overHang > 0)
+            {
+                // The next message has been partially read.
+                size_t size = overHang * 3 / 2;
+                if (size < ConnectionBase::ChunkSize)
+                {
+                    size = ConnectionBase::ChunkSize;
+                }
+                char* newBuffer = MsgHeaderScanner::allocateBuffer((int)size);
+                memcpy(newBuffer, overHangStart, overHang);
+                mBuffer = newBuffer;
+                mBufferPos = 0;
+                mBufferSize = size;
+
+                DebugLog(<< "Extra bytes after message: " << overHang);
+                //DebugLog(<< Data(mBuffer, overHang));
+
+                bytesRead = overHang;
+            }
+
             // .bwc. basicCheck takes up substantial CPU. Don't bother doing it
             // if we're overloaded.
             CongestionManager::RejectionBehavior b=mTransport->getRejectionBehaviorForIncoming();
             if (b==CongestionManager::REJECTING_NON_ESSENTIAL
                   || (b==CongestionManager::REJECTING_NEW_WORK
@@ -513,15 +538,20 @@ ConnectionBase::preparseNewBytes(int bytesRead)
                Transport::stampReceived(mMessage);
                resip_assert( mTransport );
                mTransport->pushRxMsgUp(mMessage);
                mMessage = 0;
             }
-            mConnState = NewMessage;
+            
+            if (overHang > 0) 
+            {
+               goto start;
+            }
          }
          else if (mBufferPos == mBufferSize)
          {
-            // .bwc. We've filled our buffer; go ahead and make more room.
+            // .bwc. We've filled our buffer and haven't read contentLength bytes yet; go ahead and make more room.
+            assert(contentLength >= mBufferSize);
             size_t newSize = resipMin(mBufferSize*3/2, contentLength);
             char* newBuffer = 0;
             try
             {
                newBuffer=new char[newSize];

Vulnerable code:                   DebugLog (<< Data(mBuffer, overHang));
         if (mBufferPos == contentLength)
            mBuffer=0;
            mConnState = NewMessage;
            // .bwc. We've filled our buffer; go ahead and make more room.
Fixed code:                   //DebugLog (<< Data(mBuffer, overHang));
         if (mBufferPos >= contentLength)
            int overHang = mBufferPos - (int)contentLength;
            char *overHangStart = mBuffer + contentLength;

            mConnState = NewMessage;
            mBuffer = 0;

            if (overHang > 0)
            {
                // The next message has been partially read.
                size_t size = overHang * 3 / 2;
                if (size < ConnectionBase::ChunkSize)
                {
                    size = ConnectionBase::ChunkSize;
                }
                char* newBuffer = MsgHeaderScanner::allocateBuffer((int)size);
                memcpy(newBuffer, overHangStart, overHang);
                mBuffer = newBuffer;
                mBufferPos = 0;
                mBufferSize = size;

                DebugLog(<< "Extra bytes after message: " << overHang);
                //DebugLog(<< Data(mBuffer, overHang));

                bytesRead = overHang;
            }

            
            if (overHang > 0) 
            {
               goto start;
            }
            // .bwc. We've filled our buffer and haven't read contentLength bytes yet; go ahead and make more room.
            assert(contentLength >= mBufferSize);