[reSIProcate] Unsafe use of Content-Length in ConnectionBase

[Byron Campen]

	I have taken another look at this, and added some stuff to resip/ 
stack/testTcp.cxx that triggers the bug. Bad Content-Length doesn't  
cause us to core or assert, it appears, but we do leak lots of  
memory. (run this test under valgrind to see) I have some code that  
fixes the leak in a working copy, here is the patch:

Index: ConnectionBase.cxx
===================================================================
--- ConnectionBase.cxx  (revision 6562)
+++ ConnectionBase.cxx  (working copy)
@@ -170,9 +170,28 @@
           }
           else
           {
-            // The message header is complete.
-            size_t contentLength = mMessage->header 
(h_ContentLength).value();
+            size_t contentLength = 0;

+            try
+            {
+               // The message header is complete.
+               contentLength=mMessage->header(h_ContentLength).value();
+            }
+            catch(resip::ParseBuffer::Exception& e)
+            {
+               WarningLog(<<"Malformed Content-Length in connection- 
based transport"
+                           ". Not much we can do to fix this.");
+               // !bwc! Bad Content-Length. We are hosed.
+               delete mMessage;
+               mMessage = 0;
+               mBuffer = 0;
+               // !bwc! mMessage just took ownership of mBuffer, so  
we don't
+               // delete it here.
+               //.jacob. Shouldn't the state also be set here?
+               delete this;
+               return;
+            }
+
              if (numUnprocessedChars < contentLength)
              {
                 // The message body is incomplete.
@@ -234,7 +253,26 @@
        }
        case PartialBody:
        {
-         size_t contentLength = mMessage->header 
(h_ContentLength).value();
+         size_t contentLength = 0;
+
+         try
+         {
+             contentLength = mMessage->header(h_ContentLength).value();
+         }
+         catch(resip::ParseBuffer::Exception& e)
+         {
+            WarningLog(<<"Malformed Content-Length in connection- 
based transport"
+                        ". Not much we can do to fix this.");
+            // !bwc! Bad Content-Length. We are hosed.
+            delete [] mBuffer;
+            mBuffer = 0;
+            delete mMessage;
+            mMessage = 0;
+            //.jacob. Shouldn't the state also be set here?
+            delete this;
+            return;
+         }
+
           mBufferPos += bytesRead;
           if (mBufferPos == contentLength)
           {

Comments?

Best regards,
Byron Campen


> 	ConnectionBase uses header(h_ContentLength) without a try block in  
> two places. If Content-Length is malformed, we could end up with  
> strange behavior (I haven't dug very deep into precisely how  
> strange). Recommend we wrap these in try, and if something goes  
> wrong, scrap the connection.
>
> Best regards,
> Byron Campen
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at list.sipfoundry.org
> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060911/bec12882/attachment.bin>