Re: [reSIProcate] Unsafe use of Content-Length in ConnectionBase

[Byron Campen <bcampen@xxxxxxxxxxxx>]

	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@xxxxxxxxxxxxxxxxxxx
> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel

Attachment: smime.p7s
Description: S/MIME cryptographic signature