Fix boundary check problems in socket_host.cc.

content/browser/renderer_host/p2p/socket_host.cc

Index: content/browser/renderer_host/p2p/socket_host.cc
diff --git a/content/browser/renderer_host/p2p/socket_host.cc b/content/browser/renderer_host/p2p/socket_host.cc
index fd26298c1b7c3397558aa9cfef2dce874aefdffd..44702ed3f916bed2dd7449be492af51434f33f49 100644
--- a/content/browser/renderer_host/p2p/socket_host.cc
+++ b/content/browser/renderer_host/p2p/socket_host.cc
@@ -19,12 +19,14 @@
 namespace {
 
 const uint32 kStunMagicCookie = 0x2112A442;
-const int kMinRtpHdrLen = 12;
-const int kRtpExtnHdrLen = 4;
-const int kDtlsRecordHeaderLen = 13;
-const int kTurnChannelHdrLen = 4;
-const int kAbsSendTimeExtnLen = 3;
-const int kOneByteHdrLen = 1;
+const size_t kMinRtpHdrLen = 12;

[Sergey Ulanov, 2014/09/23 18:38:08]

Not related to this CL (feel free to ignore): Ths nms r unrdbl. Should be
HeaderLength instead of HdrLen.

[jiayl, 2014/09/23 19:59:30]

Done.

+const size_t kMinRtcpHdrLen = 8;
+const size_t kRtpExtnHdrLen = 4;
+const size_t kDtlsRecordHeaderLen = 13;
+const size_t kTurnChannelHdrLen = 4;
+const size_t kAbsSendTimeExtnLen = 3;
+const size_t kOneByteHdrLen = 1;
+const size_t kMaxRtpPacketLen = 2048;
 
 // Fake auth tag written by the render process if external authentication is
 // enabled. HMAC in packet will be compared against this value before updating
@@ -33,36 +35,42 @@ static const unsigned char kFakeAuthTag[10] = {
     0xba, 0xdd, 0xba, 0xdd, 0xba, 0xdd, 0xba, 0xdd, 0xba, 0xdd
 };
 
-bool IsTurnChannelData(const char* data) {
-  return ((*data & 0xC0) == 0x40);
+bool IsTurnChannelData(const char* data, size_t len) {

[Sergey Ulanov, 2014/09/23 18:38:08]

nit: there is some |len| and some |length| in this file. Maybe rename them all
to |length| for consistency, or at least use |length| for the parameters you add
in this CL.

[jiayl, 2014/09/23 19:59:30]

Done.

+  return len >= kTurnChannelHdrLen && ((*data & 0xC0) == 0x40);
 }
 
-bool IsDtlsPacket(const char* data, int len) {
+bool IsDtlsPacket(const char* data, size_t len) {
   const uint8* u = reinterpret_cast<const uint8*>(data);
   return (len >= kDtlsRecordHeaderLen && (u[0] > 19 && u[0] < 64));
 }
 
-bool IsRtcpPacket(const char* data) {
+bool IsRtcpPacket(const char* data, size_t len) {
+  if (len < kMinRtcpHdrLen)

[Sergey Ulanov, 2014/09/23 18:38:08]

nit: Add {} or remove them for all other single-line ifs in this file.

[jiayl, 2014/09/23 19:59:30]

Done.

+    return false;
+
   int type = (static_cast<uint8>(data[1]) & 0x7F);
   return (type >= 64 && type < 96);
 }
 
-bool IsTurnSendIndicationPacket(const char* data) {
+bool IsTurnSendIndicationPacket(const char* data, size_t len) {
+  if (len < content::P2PSocketHost::kStunHeaderSize)
+    return false;
+
   uint16 type = rtc::GetBE16(data);
   return (type == cricket::TURN_SEND_INDICATION);
 }
 
-bool IsRtpPacket(const char* data, int len) {
+bool IsRtpPacket(const char* data, size_t len) {
   return ((*data & 0xC0) == 0x80);
 }
 
 // Verifies rtp header and message length.
-bool ValidateRtpHeader(const char* rtp, int length, size_t* header_length) {
+bool ValidateRtpHeader(const char* rtp, size_t length, size_t* header_length) {
   if (header_length)
     *header_length = 0;
 
-  int cc_count = rtp[0] & 0x0F;
-  int rtp_hdr_len_without_extn = kMinRtpHdrLen + 4 * cc_count;
+  size_t cc_count = rtp[0] & 0x0F;

[Sergey Ulanov, 2014/09/23 18:38:08]

Do you need to verify that length > 0 before reading rtp[0]?

[palmer, 2014/09/23 18:57:32]

Yes, and probably in |IsRtpPacket|, too.

I almost wonder if

    rtp[0] != rtp[0] & 0x0F

indicates a malformed, damaged, or even malicious packet?

Ohh, I see, you need to check the 0x10 bit, too (line 80). Maybe check that no
other bits are set, e.g. rtp[0] & 0xe0 == 0? I don't know...

[jiayl, 2014/09/23 19:59:30]

Done length check.

I don't think we should check other bits.

+  size_t rtp_hdr_len_without_extn = kMinRtpHdrLen + 4 * cc_count;
   if (rtp_hdr_len_without_extn > length) {
     return false;
   }
@@ -78,21 +86,29 @@ bool ValidateRtpHeader(const char* rtp, int length, size_t* header_length) {
 
   rtp += rtp_hdr_len_without_extn;
 
+  if (rtp_hdr_len_without_extn + kRtpExtnHdrLen > length) {
+    return false;
+  }
+
   // Getting extension profile length.
   // Length is in 32 bit words.
-  uint16 extn_length = rtc::GetBE16(rtp + 2) * 4;
+  uint16 extn_len_32bit_words = rtc::GetBE16(rtp + 2);

[palmer, 2014/09/23 18:57:31]

Nit: Again, standardize on |...length| rather than mixing |...len| and
|...length|.

[jiayl, 2014/09/23 19:59:31]

Done.

+  size_t extn_length = extn_len_32bit_words * 4;
+
+  size_t rtp_header_length =
+      extn_length + rtp_hdr_len_without_extn + kRtpExtnHdrLen;
 
   // Verify input length against total header size.
-  if (rtp_hdr_len_without_extn + kRtpExtnHdrLen + extn_length > length) {
+  if (rtp_header_length > length) {
     return false;
   }
 
   if (header_length)

[Sergey Ulanov, 2014/09/23 18:38:08]

nit: add {}

[jiayl, 2014/09/23 19:59:31]

Done.

-    *header_length = rtp_hdr_len_without_extn + kRtpExtnHdrLen + extn_length;
+    *header_length = rtp_header_length;
   return true;
 }
 
-void UpdateAbsSendTimeExtnValue(char* extn_data, int len,
+void UpdateAbsSendTimeExtnValue(char* extn_data, size_t len,
                                 uint32 abs_send_time) {
   // Absolute send time in RTP streams.
   //
@@ -109,7 +125,11 @@ void UpdateAbsSendTimeExtnValue(char* extn_data, int len,
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   //   |  ID   | len=2 |              absolute send time               |
   //   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-  DCHECK_EQ(len, kAbsSendTimeExtnLen);
+  if (len != kAbsSendTimeExtnLen) {
+    NOTREACHED();
+    return;
+  }
+
   // Now() has resolution ~1-15ms, using HighResNow(). But it is warned not to
   // use it unless necessary, as it is expensive than Now().
   uint32 now_second = abs_send_time;
@@ -128,17 +148,19 @@ void UpdateAbsSendTimeExtnValue(char* extn_data, int len,
 
 // Assumes |len| is actual packet length + tag length. Updates HMAC at end of
 // the RTP packet.
-void UpdateRtpAuthTag(char* rtp, int len,
+void UpdateRtpAuthTag(char* rtp, size_t len,
                       const rtc::PacketOptions& options) {
   // If there is no key, return.
   if (options.packet_time_params.srtp_auth_key.empty())
     return;
 
   size_t tag_length = options.packet_time_params.srtp_auth_tag_len;
-  char* auth_tag = rtp + (len - tag_length);
 
-  // We should have a fake HMAC value @ auth_tag.
-  DCHECK_EQ(0, memcmp(auth_tag, kFakeAuthTag, tag_length));
+  const size_t kRocLength = 4;

[Sergey Ulanov, 2014/09/23 18:38:08]

What is Roc?

[jiayl, 2014/09/23 19:59:31]

Added a comment.

+  if (tag_length < kRocLength || tag_length > len) {
+    NOTREACHED();
+    return;
+  }
 
   crypto::HMAC hmac(crypto::HMAC::SHA1);
   if (!hmac.Init(reinterpret_cast<const unsigned char*>(
@@ -148,15 +170,20 @@ void UpdateRtpAuthTag(char* rtp, int len,
     return;
   }
 
-  if (hmac.DigestLength() < tag_length) {
+  if (tag_length > hmac.DigestLength()) {
     NOTREACHED();
     return;
   }
 
+  char* auth_tag = rtp + (len - tag_length);
+
+  // We should have a fake HMAC value @ auth_tag.
+  DCHECK_EQ(0, memcmp(auth_tag, kFakeAuthTag, tag_length));
+
   // Copy ROC after end of rtp packet.
-  memcpy(auth_tag, &options.packet_time_params.srtp_packet_index, 4);
+  memcpy(auth_tag, &options.packet_time_params.srtp_packet_index, kRocLength);
   // Authentication of a RTP packet will have RTP packet + ROC size.
-  int auth_required_length = len - tag_length + 4;
+  int auth_required_length = len - tag_length + kRocLength;
 
   unsigned char output[64];
   if (!hmac.Sign(base::StringPiece(rtp, auth_required_length),
@@ -175,7 +202,7 @@ namespace content {
 
 namespace packet_processing_helpers {
 
-bool ApplyPacketOptions(char* data, int length,
+bool ApplyPacketOptions(char* data, size_t length,
                         const rtc::PacketOptions& options,
                         uint32 abs_send_time) {
   DCHECK(data != NULL);
@@ -188,13 +215,13 @@ bool ApplyPacketOptions(char* data, int length,
   }
 
   DCHECK(!IsDtlsPacket(data, length));
-  DCHECK(!IsRtcpPacket(data));
+  DCHECK(!IsRtcpPacket(data, length));
 
   // If there is a srtp auth key present then packet must be a RTP packet.
   // RTP packet may have been wrapped in a TURN Channel Data or
   // TURN send indication.
-  int rtp_start_pos;
-  int rtp_length;
+  size_t rtp_start_pos;
+  size_t rtp_length;
   if (!GetRtpPacketStartPositionAndLength(
       data, length, &rtp_start_pos, &rtp_length)) {
     // This method should never return false.
@@ -218,12 +245,15 @@ bool ApplyPacketOptions(char* data, int length,
 }
 
 bool GetRtpPacketStartPositionAndLength(const char* packet,
-                                        int length,
-                                        int* rtp_start_pos,
-                                        int* rtp_packet_length) {
-  int rtp_begin;
-  int rtp_length = 0;
-  if (IsTurnChannelData(packet)) {
+                                        size_t length,
+                                        size_t* rtp_start_pos,
+                                        size_t* rtp_packet_length) {
+  if (length < kMinRtpHdrLen || length > kMaxRtpPacketLen)
+    return false;
+
+  size_t rtp_begin;
+  size_t rtp_length = 0;
+  if (IsTurnChannelData(packet, length)) {
     // Turn Channel Message header format.
     //   0                   1                   2                   3
     //  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
@@ -234,22 +264,14 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
     // /                       Application Data                        /
     // /                                                               /
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    if (length < kTurnChannelHdrLen) {
-      return false;
-    }
-
     rtp_begin = kTurnChannelHdrLen;
     rtp_length = rtc::GetBE16(&packet[2]);
     if (length < rtp_length + kTurnChannelHdrLen) {
       return false;
     }
-  } else if (IsTurnSendIndicationPacket(packet)) {
-    if (length <= P2PSocketHost::kStunHeaderSize) {
-      // Message must be greater than 20 bytes, if it's carrying any payload.
-      return false;
-    }
+  } else if (IsTurnSendIndicationPacket(packet, length)) {
     // Validate STUN message length.
-    int stun_msg_len = rtc::GetBE16(&packet[2]);
+    size_t stun_msg_len = rtc::GetBE16(&packet[2]);
     if (stun_msg_len + P2PSocketHost::kStunHeaderSize != length) {
       return false;
     }
@@ -259,7 +281,7 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
     // Loop through STUN attributes until we find STUN DATA attribute.
     const char* start = packet + rtp_begin;
     bool data_attr_present = false;
-    while ((packet + rtp_begin) - start < stun_msg_len) {
+    while (packet + rtp_begin < start + stun_msg_len) {
       // Keep reading STUN attributes until we hit DATA attribute.
       // Attribute will be a TLV structure.
       // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
@@ -274,17 +296,24 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
       // padding so that its value contains a multiple of 4 bytes.  The
       // padding bits are ignored, and may be any value.
       uint16 attr_type, attr_length;
+      const int kAttrHeaderLength = sizeof(attr_type) + sizeof(attr_length);

[Sergey Ulanov, 2014/09/23 18:38:08]

nit: it's better to write =4 instead of sum the of the sizes.

[palmer, 2014/09/23 18:57:32]

I disagree; it's nice documentation of the intent.

Also, the value of sizeof(foo) is of type size_t, so |kAttrHeaderLength| should
be declared as a size_t too.

[jiayl, 2014/09/23 19:59:30]

Acknowledged.

+
+      if (length < rtp_begin + kAttrHeaderLength) {
+        return false;
+      }
+
       // Getting attribute type and length.
       attr_type = rtc::GetBE16(&packet[rtp_begin]);
       attr_length = rtc::GetBE16(
           &packet[rtp_begin + sizeof(attr_type)]);
+
       // Checking for bogus attribute length.
-      if (length < attr_length + rtp_begin) {
+      if (length < rtp_begin + kAttrHeaderLength + attr_length) {
         return false;
       }
 
       if (attr_type != cricket::STUN_ATTR_DATA) {
-        rtp_begin += sizeof(attr_type) + sizeof(attr_length) + attr_length;
+        rtp_begin += kAttrHeaderLength + attr_length;
         if ((attr_length % 4) != 0) {
           rtp_begin += (4 - (attr_length % 4));
         }
@@ -292,12 +321,9 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
       }
 
       data_attr_present = true;
-      rtp_begin += 4;  // Skip STUN_DATA_ATTR header.
+      rtp_begin += kAttrHeaderLength;  // Skip STUN_DATA_ATTR header.
       rtp_length = attr_length;
-      // One final check of length before exiting.
-      if (length < rtp_length + rtp_begin) {
-        return false;
-      }
+
       // We found STUN_DATA_ATTR. We can skip parsing rest of the packet.
       break;
     }
@@ -327,7 +353,7 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
 
 // ValidateRtpHeader must be called before this method to make sure, we have
 // a sane rtp packet.
-bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
+bool UpdateRtpAbsSendTimeExtn(char* rtp, size_t length,
                               int extension_id, uint32 abs_send_time) {
   //  0                   1                   2                   3
   //  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
@@ -347,15 +373,16 @@ bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
     return true;
   }
 
-  int cc_count = rtp[0] & 0x0F;
-  int rtp_hdr_len_without_extn = kMinRtpHdrLen + 4 * cc_count;
+  size_t cc_count = rtp[0] & 0x0F;
+  size_t rtp_hdr_len_without_extn = kMinRtpHdrLen + 4 * cc_count;
 
   rtp += rtp_hdr_len_without_extn;
 
   // Getting extension profile ID and length.
   uint16 profile_id = rtc::GetBE16(rtp);
   // Length is in 32 bit words.
-  uint16 extn_length = rtc::GetBE16(rtp + 2) * 4;
+  uint16 extn_len_32bit_words = rtc::GetBE16(rtp + 2);
+  size_t extn_length = extn_len_32bit_words * 4;
 
   rtp += kRtpExtnHdrLen;  // Moving past extn header.
 
@@ -380,10 +407,15 @@ bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     // |                          data                                 |
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    char* extn_start = rtp;
-    while (rtp - extn_start < extn_length) {
+    const char* extn_start = rtp;
+    const char* extn_end = extn_start + extn_length;
+
+    while (rtp < extn_end) {
       const int id = (*rtp & 0xF0) >> 4;
-      const int len = (*rtp & 0x0F) + 1;
+      const size_t len = (*rtp & 0x0F) + 1;
+      if (rtp + kOneByteHdrLen + len > extn_end) {
+        return false;
+      }
       // The 4-bit length is the number minus one of data bytes of this header
       // extension element following the one-byte header.
       if (id == extension_id) {
@@ -393,7 +425,7 @@ bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
       }
       rtp += kOneByteHdrLen + len;
       // Counting padding bytes.
-      while ((*rtp == 0) && (rtp - extn_start < extn_length)) {
+      while ((rtp < extn_end) && (*rtp == 0)) {
         ++rtp;
       }
     }
@@ -528,11 +560,11 @@ void P2PSocketHost::StopRtpDump(bool incoming, bool outgoing) {
 void P2PSocketHost::DumpRtpPacket(const char* packet,
                                   size_t length,
                                   bool incoming) {
-  if (IsDtlsPacket(packet, length) || IsRtcpPacket(packet))
+  if (IsDtlsPacket(packet, length) || IsRtcpPacket(packet, length))
     return;
 
-  int rtp_packet_pos = 0;
-  int rtp_packet_length = length;
+  size_t rtp_packet_pos = 0;
+  size_t rtp_packet_length = length;
   if (!packet_processing_helpers::GetRtpPacketStartPositionAndLength(
           packet, length, &rtp_packet_pos, &rtp_packet_length))
     return;