From 2bad88d164754f1f0694e9fea1051e71b3cb5347 Mon Sep 17 00:00:00 2001 From: pbos Date: Mon, 6 Jul 2015 03:09:08 -0700 Subject: [PATCH] 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} --- .../rtp_rtcp/source/fec_receiver_unittest.cc | 43 +++++++++++++++- .../source/forward_error_correction.cc | 49 ++++++++++++++----- .../source/forward_error_correction.h | 6 +-- 3 files changed, 82 insertions(+), 16 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc index e67ec4e53..f64b537a5 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc +++ b/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::iterator it = media_rtp_packets.begin(); - std::list::iterator media_it = media_rtp_packets.begin(); - BuildAndAddRedMediaPacket(*media_it); + BuildAndAddRedMediaPacket(*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 media_rtp_packets; + std::list media_packets; + GenerateFrame(2, 0, &media_rtp_packets, &media_packets); + std::list fec_packets; + GenerateFEC(&media_packets, &fec_packets, kNumFecPackets); + ByteWriter::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 media_rtp_packets; diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index a9e88a5bc..48126cec5 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/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(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]; - // Copy the protection length from the ULP header. - memcpy(protection_length, &fec_packet->pkt->data[10], 2); + uint16_t protection_length = + ByteReader::ReadBigEndian(&fec_packet->pkt->data[10]); + 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::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::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::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( - const FecPacket* fec_packet, RecoveredPacket* rec_packet_to_insert) { - InitRecovery(fec_packet, rec_packet_to_insert); +bool ForwardErrorCorrection::RecoverPacket( + const FecPacket* fec_packet, + 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. diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h b/webrtc/modules/rtp_rtcp/source/forward_error_correction.h index c6f273848..dbb3fa93e 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.h +++ b/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