prevent TcpHeader::Deserialize() from asserting; fix a few small bugs in option code
authorTom Henderson <tomh@tomh.org>
Fri, 12 Sep 2014 18:29:47 -0700
changeset 10943 e329871a431f
parent 10942 2011df8ec4f6
child 10944 93c0d8e02bf0
prevent TcpHeader::Deserialize() from asserting; fix a few small bugs in option code
src/internet/model/tcp-header.cc
src/internet/model/tcp-header.h
src/internet/model/tcp-option-ts.cc
src/internet/model/tcp-option-winscale.cc
src/internet/model/tcp-option.cc
src/internet/model/tcp-socket-base.cc
--- a/src/internet/model/tcp-header.cc	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-header.cc	Fri Sep 12 18:29:47 2014 -0700
@@ -301,7 +301,7 @@
 uint32_t
 TcpHeader::GetSerializedSize (void)  const
 {
-  return 4*GetLength ();
+  return CalculateHeaderLength () * 4;
 }
 
 void
@@ -367,7 +367,11 @@
   // Deserialize options if they exist
   m_options.clear ();
   uint32_t optionLen = (m_length - 5) * 4;
-  NS_ASSERT_MSG (optionLen <= 40, "Illegal TCP option length");
+  if (optionLen > 40)
+    {
+      NS_LOG_ERROR ("Illegal TCP option length " << optionLen << "; options discarded");
+      return 20;
+    }
   while (optionLen)
     {
       uint8_t kind = i.PeekU8 ();
@@ -383,7 +387,11 @@
           NS_LOG_WARN ("Option kind " << static_cast<int> (kind) << " unknown, skipping.");
         }
       optionSize = op->Deserialize (i);
-      NS_ASSERT_MSG (optionSize == op->GetSerializedSize (), "Option did not deserialize correctly");
+      if (optionSize != op->GetSerializedSize ())
+        {
+          NS_LOG_ERROR ("Option did not deserialize correctly");
+          break;
+        }
       if (optionLen >= optionSize)
         {
           optionLen -= optionSize;
@@ -392,12 +400,14 @@
         }
       else
         {
-          NS_ASSERT_MSG (false, "Option exceeds TCP option space");
+          NS_LOG_ERROR ("Option exceeds TCP option space; option discarded");
+          break;
         }
       if (op->GetKind () == TcpOption::END)
         {
           while (optionLen)
             {
+              // Discard padding bytes without adding to option list
               i.Next (1);
               --optionLen;
             }
@@ -406,7 +416,7 @@
 
   if (m_length != CalculateHeaderLength ())
     {
-      NS_LOG_WARN ("Mismatch between calculated length and in-header value");
+      NS_LOG_ERROR ("Mismatch between calculated length and in-header value");
     }
 
   // Do checksum
@@ -431,7 +441,11 @@
     {
       len += (*i)->GetSerializedSize ();
     }
-
+  // Option list may not include padding; need to pad up to word boundary
+  if (len % 4)
+    {
+      len += 4 - (len % 4);
+    }
   return len >> 2;
 }
 
--- a/src/internet/model/tcp-header.h	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-header.h	Fri Sep 12 18:29:47 2014 -0700
@@ -275,7 +275,7 @@
    * Given the standard size of the header, the method checks for options
    * and calculates the real length (in words).
    *
-   * \return header length
+   * \return header length in 4-byte words
    */
   uint8_t CalculateHeaderLength () const;
 
--- a/src/internet/model/tcp-option-ts.cc	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-option-ts.cc	Fri Sep 12 18:29:47 2014 -0700
@@ -88,7 +88,11 @@
     }
 
   uint8_t size = i.ReadU8 ();
-  NS_ASSERT (size == 10);
+  if (size != 10)
+    {
+      NS_LOG_WARN ("Malformed Timestamp option");
+      return 0;
+    }
   m_timestamp = i.ReadNtohU32 ();
   m_echo = i.ReadNtohU32 ();
   return GetSerializedSize ();
--- a/src/internet/model/tcp-option-winscale.cc	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-option-winscale.cc	Fri Sep 12 18:29:47 2014 -0700
@@ -86,7 +86,11 @@
       return 0;
     }
   uint8_t size = i.ReadU8 ();
-  NS_ASSERT (size == 3);
+  if (size != 3)
+    {
+      NS_LOG_WARN ("Malformed Window Scale option");
+      return 0;
+    }
   m_scale = i.ReadU8 ();
   return GetSerializedSize ();
 }
--- a/src/internet/model/tcp-option.cc	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-option.cc	Fri Sep 12 18:29:47 2014 -0700
@@ -171,7 +171,11 @@
   NS_LOG_WARN ("Trying to Deserialize an Unknown Option of Kind " << int (m_kind));
 
   m_size = i.ReadU8 ();
-  NS_ASSERT_MSG ((m_size >= 2) && (m_size < 40), "Unable to parse an Unknown Option of Kind " << int (m_kind) << " with apparent size " << int (m_size));
+  if (m_size < 2 || m_size > 40)
+    {
+      NS_LOG_WARN ("Unable to parse an unknown option of kind " << int (m_kind) << " with apparent size " << int (m_size));
+      return 0;
+    }
 
   i.Read (m_content, m_size-2);
 
--- a/src/internet/model/tcp-socket-base.cc	Fri Sep 12 18:19:47 2014 -0700
+++ b/src/internet/model/tcp-socket-base.cc	Fri Sep 12 18:29:47 2014 -0700
@@ -903,7 +903,12 @@
 
   // Peel off TCP header and do validity checking
   TcpHeader tcpHeader;
-  packet->RemoveHeader (tcpHeader);
+  uint32_t bytesRemoved = packet->RemoveHeader (tcpHeader);
+  if (bytesRemoved == 0 || bytesRemoved > 60)
+    {
+      NS_LOG_ERROR ("Bytes removed: " << bytesRemoved << " invalid");
+      return; // Discard invalid packet
+    }
 
   ReadOptions (tcpHeader);
 
@@ -988,6 +993,7 @@
     }
 }
 
+// XXX this is duplicate code with the other DoForwardUp()
 void
 TcpSocketBase::DoForwardUp (Ptr<Packet> packet, Ipv6Header header, uint16_t port)
 {
@@ -1001,7 +1007,12 @@
 
   // Peel off TCP header and do validity checking
   TcpHeader tcpHeader;
-  packet->RemoveHeader (tcpHeader);
+  uint32_t bytesRemoved = packet->RemoveHeader (tcpHeader);
+  if (bytesRemoved == 0 || bytesRemoved > 60)
+    {
+      NS_LOG_ERROR ("Bytes removed: " << bytesRemoved << " invalid");
+      return; // Discard invalid packet
+    }
 
   ReadOptions (tcpHeader);