Prevent heap overflows for incorrect FEC packet lengths.

Bugs found by manual inspection of code, not by fuzzing or packet
replays. At least one of them confirmed by local fuzzing.

BUG=chromium:496094, webrtc:4771
R=stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1182793002

Cr-Commit-Position: refs/heads/master@{#9542}

webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc

@@ -18,6 +18,7 @@
 #include "webrtc/modules/rtp_rtcp/interface/fec_receiver.h"
 #include "webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h"
 #include "webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/fec_test_helper.h"
 #include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
 
@@ -82,6 +83,7 @@ class ReceiverFecTest : public ::testing::Test {
     delete red_packet;
   }
 
+  void InjectGarbagePacketLength(size_t fec_garbage_offset);
   static void SurvivesMaliciousPacket(const uint8_t* data,
                                       size_t length,
                                       uint8_t ulpfec_payload_type);
@@ -109,8 +111,7 @@ TEST_F(ReceiverFecTest, TwoMediaOneFec) {
 
   // Recovery
   std::list<RtpPacket*>::iterator it = media_rtp_packets.begin();
-  std::list<RtpPacket*>::iterator media_it = media_rtp_packets.begin();
+  BuildAndAddRedMediaPacket(*it);
-  BuildAndAddRedMediaPacket(*media_it);
   VerifyReconstructedMediaPacket(*it, 1);
   EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec());
   // Drop one media packet.
@@ -128,6 +129,44 @@ TEST_F(ReceiverFecTest, TwoMediaOneFec) {
   DeletePackets(&media_packets);
 }
 
+void ReceiverFecTest::InjectGarbagePacketLength(size_t fec_garbage_offset) {
+  EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _))
+      .WillRepeatedly(Return(true));
+
+  const unsigned int kNumFecPackets = 1u;
+  std::list<RtpPacket*> media_rtp_packets;
+  std::list<Packet*> media_packets;
+  GenerateFrame(2, 0, &media_rtp_packets, &media_packets);
+  std::list<Packet*> fec_packets;
+  GenerateFEC(&media_packets, &fec_packets, kNumFecPackets);
+  ByteWriter<uint16_t>::WriteBigEndian(
+      &fec_packets.front()->data[fec_garbage_offset], 0x4711);
+
+  // Inject first media packet, then first FEC packet, skipping the second media
+  // packet to cause a recovery from the FEC packet.
+  BuildAndAddRedMediaPacket(media_rtp_packets.front());
+  BuildAndAddRedFecPacket(fec_packets.front());
+  EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec());
+
+  FecPacketCounter counter = receiver_fec_->GetPacketCounter();
+  EXPECT_EQ(2u, counter.num_packets);
+  EXPECT_EQ(1u, counter.num_fec_packets);
+  EXPECT_EQ(0u, counter.num_recovered_packets);
+
+  DeletePackets(&media_packets);
+}
+
+TEST_F(ReceiverFecTest, InjectGarbageFecHeaderLengthRecovery) {
+  // Byte offset 8 is the 'length recovery' field of the FEC header.
+  InjectGarbagePacketLength(8);
+}
+
+TEST_F(ReceiverFecTest, InjectGarbageFecLevelHeaderProtectionLength) {
+  // Byte offset 10 is the 'protection length' field in the first FEC level
+  // header.
+  InjectGarbagePacketLength(10);
+}
+
 TEST_F(ReceiverFecTest, TwoMediaTwoFec) {
   const unsigned int kNumFecPackets = 2u;
   std::list<RtpPacket*> media_rtp_packets;

webrtc/modules/rtp_rtcp/source/forward_error_correction.cc

@@ -634,23 +634,35 @@ void ForwardErrorCorrection::InsertPackets(
   DiscardOldPackets(recovered_packet_list);
 }
 
-void ForwardErrorCorrection::InitRecovery(const FecPacket* fec_packet,
+bool ForwardErrorCorrection::InitRecovery(const FecPacket* fec_packet,
                                           RecoveredPacket* recovered) {
   // This is the first packet which we try to recover with.
   const uint16_t ulp_header_size =
       fec_packet->pkt->data[0] & 0x40 ? kUlpHeaderSizeLBitSet
                                       : kUlpHeaderSizeLBitClear;  // L bit set?
+  if (fec_packet->pkt->length <
+      static_cast<size_t>(kFecHeaderSize + ulp_header_size)) {
+    LOG(LS_WARNING)
+        << "Truncated FEC packet doesn't contain room for ULP header.";
+    return false;
+  }
   recovered->pkt = new Packet;
   memset(recovered->pkt->data, 0, IP_PACKET_SIZE);
   recovered->returned = false;
   recovered->was_recovered = true;
-  uint8_t protection_length[2];
+  uint16_t protection_length =
-  // Copy the protection length from the ULP header.
+      ByteReader<uint16_t>::ReadBigEndian(&fec_packet->pkt->data[10]);
-  memcpy(protection_length, &fec_packet->pkt->data[10], 2);
+  if (protection_length >
+      std::min(
+          sizeof(recovered->pkt->data) - kRtpHeaderSize,
+          sizeof(fec_packet->pkt->data) - kFecHeaderSize - ulp_header_size)) {
+    LOG(LS_WARNING) << "Incorrect FEC protection length, dropping.";
+    return false;
+  }
   // Copy FEC payload, skipping the ULP header.
   memcpy(&recovered->pkt->data[kRtpHeaderSize],
          &fec_packet->pkt->data[kFecHeaderSize + ulp_header_size],
-         ByteReader<uint16_t>::ReadBigEndian(protection_length));
+         protection_length);
   // Copy the length recovery field.
   memcpy(recovered->length_recovery, &fec_packet->pkt->data[8], 2);
   // Copy the first 2 bytes of the FEC header.
@@ -660,9 +672,10 @@ void ForwardErrorCorrection::InitRecovery(const FecPacket* fec_packet,
   // Set the SSRC field.
   ByteWriter<uint32_t>::WriteBigEndian(&recovered->pkt->data[8],
                                        fec_packet->ssrc);
+  return true;
 }
 
-void ForwardErrorCorrection::FinishRecovery(RecoveredPacket* recovered) {
+bool ForwardErrorCorrection::FinishRecovery(RecoveredPacket* recovered) {
   // Set the RTP version to 2.
   recovered->pkt->data[0] |= 0x80;  // Set the 1st bit.
   recovered->pkt->data[0] &= 0xbf;  // Clear the 2nd bit.
@@ -674,6 +687,10 @@ void ForwardErrorCorrection::FinishRecovery(RecoveredPacket* recovered) {
   recovered->pkt->length =
       ByteReader<uint16_t>::ReadBigEndian(recovered->length_recovery) +
       kRtpHeaderSize;
+  if (recovered->pkt->length > sizeof(recovered->pkt->data) - kRtpHeaderSize)
+    return false;
+
+  return true;
 }
 
 void ForwardErrorCorrection::XorPackets(const Packet* src_packet,
@@ -700,9 +717,11 @@ void ForwardErrorCorrection::XorPackets(const Packet* src_packet,
   }
 }
 
-void ForwardErrorCorrection::RecoverPacket(
+bool ForwardErrorCorrection::RecoverPacket(
-    const FecPacket* fec_packet, RecoveredPacket* rec_packet_to_insert) {
+    const FecPacket* fec_packet,
-  InitRecovery(fec_packet, rec_packet_to_insert);
+    RecoveredPacket* rec_packet_to_insert) {
+  if (!InitRecovery(fec_packet, rec_packet_to_insert))
+    return false;
   ProtectedPacketList::const_iterator protected_it =
       fec_packet->protected_pkt_list.begin();
   while (protected_it != fec_packet->protected_pkt_list.end()) {
@@ -714,7 +733,9 @@ void ForwardErrorCorrection::RecoverPacket(
     }
     ++protected_it;
   }
-  FinishRecovery(rec_packet_to_insert);
+  if (!FinishRecovery(rec_packet_to_insert))
+    return false;
+  return true;
 }
 
 void ForwardErrorCorrection::AttemptRecover(
@@ -729,7 +750,13 @@ void ForwardErrorCorrection::AttemptRecover(
       // Recovery possible.
       RecoveredPacket* packet_to_insert = new RecoveredPacket;
       packet_to_insert->pkt = NULL;
-      RecoverPacket(*fec_packet_list_it, packet_to_insert);
+      if (!RecoverPacket(*fec_packet_list_it, packet_to_insert)) {
+        // Can't recover using this packet, drop it.
+        DiscardFECPacket(*fec_packet_list_it);
+        fec_packet_list_it = fec_packet_list_.erase(fec_packet_list_it);
+        delete packet_to_insert;
+        continue;
+      }
 
       // Add recovered packet to the list of recovered packets and update any
       // FEC packets covering this packet with a pointer to the data.

webrtc/modules/rtp_rtcp/source/forward_error_correction.h

@@ -279,7 +279,7 @@ class ForwardErrorCorrection {
   void AttemptRecover(RecoveredPacketList* recovered_packet_list);
 
   // Initializes the packet recovery using the FEC packet.
-  static void InitRecovery(const FecPacket* fec_packet,
+  static bool InitRecovery(const FecPacket* fec_packet,
                            RecoveredPacket* recovered);
 
   // Performs XOR between |src_packet| and |dst_packet| and stores the result
@@ -287,10 +287,10 @@ class ForwardErrorCorrection {
   static void XorPackets(const Packet* src_packet, RecoveredPacket* dst_packet);
 
   // Finish up the recovery of a packet.
-  static void FinishRecovery(RecoveredPacket* recovered);
+  static bool FinishRecovery(RecoveredPacket* recovered);
 
   // Recover a missing packet.
-  void RecoverPacket(const FecPacket* fec_packet,
+  bool RecoverPacket(const FecPacket* fec_packet,
                      RecoveredPacket* rec_packet_to_insert);
 
   // Get the number of missing media packets which are covered by this