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..9a80fd097f8c0043a4057be09470dc108e5f009a 100644
--- a/content/browser/renderer_host/p2p/socket_host.cc
+++ b/content/browser/renderer_host/p2p/socket_host.cc
@@ -33,8 +33,8 @@ 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, int len) {

[palmer, 2014/09/22 19:34:46]

size_t, or explicitly-sized unsigned type.

http://www.chromium.org/Home/chromium-security/education/security-tips-for-ip....

[jiayl, 2014/09/22 21:39:02]

Done.

+  return len >= 1 && ((*data & 0xC0) == 0x40);

[palmer, 2014/09/22 19:34:46]

Is there a maximum sane size we should check for, too?

[jiayl, 2014/09/22 21:39:03]

I don't think there is a maximum.

[juberti2, 2014/09/22 22:43:56]

We have a max packet size of 2048 that we use elsewhere. If checking that
provides some extra safety, we should probably do it when the packet is first
received.

[jiayl, 2014/09/22 23:04:13]

Done.

 }
 
 bool IsDtlsPacket(const char* data, int len) {
@@ -47,7 +47,10 @@ bool IsRtcpPacket(const char* data) {
   return (type >= 64 && type < 96);
 }
 
-bool IsTurnSendIndicationPacket(const char* data) {
+bool IsTurnSendIndicationPacket(const char* data, int len) {
+  if (len < 2)

[palmer, 2014/09/22 19:34:46]

I'd suggest using kMinimumWhateverLength and kMaximumWhateverLength, here and in
|IsTurnChannelData|.

[jiayl, 2014/09/22 21:39:03]

Used the minimum header length for these methods. 

+    return false;
+
   uint16 type = rtc::GetBE16(data);
   return (type == cricket::TURN_SEND_INDICATION);
 }
@@ -78,6 +81,10 @@ 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) {

[palmer, 2014/09/22 19:34:46]

Integer overflow potential? Consider using checked_numeric<foo> from base/.

[jiayl, 2014/09/22 21:39:02]

The max of the sum is 12 + 4 * 31 + 4 * UINT16_MAX, which should not overflow
size_t.

+    return false;
+  }
+
   // Getting extension profile length.
   // Length is in 32 bit words.
   uint16 extn_length = rtc::GetBE16(rtp + 2) * 4;

[aedla, 2014/09/22 20:06:47]

Would be nice to fix the integer overflow here.

[jiayl, 2014/09/22 21:39:03]

Changed the result to size_t to avoid overflow.

@@ -109,7 +116,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;
@@ -148,15 +159,17 @@ void UpdateRtpAuthTag(char* rtp, int len,
     return;
   }
 
-  if (hmac.DigestLength() < tag_length) {
+  const size_t kRocLength = 4;
+
+  if (tag_length > hmac.DigestLength() || tag_length < kRocLength) {

[aedla, 2014/09/22 20:06:47]

That won't cut it. Say tag_length == hmac.DigestLength() == 20 and len == 16.
Then auth_tag == rtp - 4 and second memcpy will OOB write 4 bytes.

Failing with (tag_length > len || tag_length < kRocLength) should be ok from a
security standpoint. Not sure about semantics. I'll think about it a bit more
though.

[jiayl, 2014/09/22 21:39:03]

Done.

     NOTREACHED();
     return;
   }
 
   // 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;

[palmer, 2014/09/22 19:34:46]

Integer overflow potential?

[jiayl, 2014/09/22 21:39:03]

tag_length is guaranteed to be >= kRocLength, so no overflow risk.

 
   unsigned char output[64];
   if (!hmac.Sign(base::StringPiece(rtp, auth_required_length),
@@ -221,9 +234,12 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
                                         int length,
                                         int* rtp_start_pos,
                                         int* rtp_packet_length) {
+  *rtp_start_pos = 0;
+  *rtp_packet_length = 0;
+
   int rtp_begin;
   int rtp_length = 0;
-  if (IsTurnChannelData(packet)) {
+  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
@@ -243,7 +259,7 @@ bool GetRtpPacketStartPositionAndLength(const char* packet,
     if (length < rtp_length + kTurnChannelHdrLen) {
       return false;
     }
-  } else if (IsTurnSendIndicationPacket(packet)) {
+  } else if (IsTurnSendIndicationPacket(packet, length)) {
     if (length <= P2PSocketHost::kStunHeaderSize) {
       // Message must be greater than 20 bytes, if it's carrying any payload.
       return false;
@@ -274,12 +290,19 @@ 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);
+
+      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;
       }
 
@@ -380,10 +403,13 @@ bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     // |                          data                                 |
     // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    char* extn_start = rtp;
+    const char* extn_start = rtp;

[aedla, 2014/09/22 20:06:46]

Having
const char* extn_end = extn_start + extn_length;
as well would simplify length checks below.

[jiayl, 2014/09/22 21:39:03]

Done.

     while (rtp - extn_start < extn_length) {
       const int id = (*rtp & 0xF0) >> 4;
       const int len = (*rtp & 0x0F) + 1;
+      if (rtp + kOneByteHdrLen + len - extn_start >= extn_length) {

[aedla, 2014/09/22 20:06:48]

I think this should be > extn_length. Or
if (rtp + kOneByteHdrLen + len > extn_end) {

[jiayl, 2014/09/22 21:39:03]

You are right. Done.

+        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 +419,7 @@ bool UpdateRtpAbsSendTimeExtn(char* rtp, int length,
       }
       rtp += kOneByteHdrLen + len;
       // Counting padding bytes.
-      while ((*rtp == 0) && (rtp - extn_start < extn_length)) {
+      while ((rtp - extn_start < extn_length) && (*rtp == 0)) {
         ++rtp;
       }
     }