From f1828e8ed96ae1aa3ea9dc1eb96e2e703d2e78cf Mon Sep 17 00:00:00 2001 From: pbos Date: Tue, 28 Jul 2015 08:20:59 -0700 Subject: [PATCH] Prevent OOB reads for truncated H264 STAP-A packets. BUG=webrtc:4771, webrtc:4834 R=stefan@webrtc.org Review URL: https://codereview.webrtc.org/1238033003 Cr-Commit-Position: refs/heads/master@{#9650} --- .../rtp_rtcp/source/rtp_format_h264.cc | 22 +++++ .../video_coding/main/source/session_info.cc | 2 + webrtc/test/call_test.cc | 1 + webrtc/video/packet_injection_tests.cc | 91 +++++++++++++++++++ webrtc/webrtc_tests.gypi | 1 + 5 files changed, 117 insertions(+) create mode 100644 webrtc/video/packet_injection_tests.cc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc b/webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc index ba41c620c5..8849732157 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc @@ -40,6 +40,23 @@ enum NalDefs { kFBit = 0x80, kNriMask = 0x60, kTypeMask = 0x1F }; // Bit masks for FU (A and B) headers. enum FuDefs { kSBit = 0x80, kEBit = 0x40, kRBit = 0x20 }; +// TODO(pbos): Avoid parsing this here as well as inside the jitter buffer. +bool VerifyStapANaluLengths(const uint8_t* nalu_ptr, size_t length_remaining) { + while (length_remaining > 0) { + // Buffer doesn't contain room for additional nalu length. + if (length_remaining < sizeof(uint16_t)) + return false; + uint16_t nalu_size = nalu_ptr[0] << 8 | nalu_ptr[1]; + nalu_ptr += sizeof(uint16_t); + length_remaining -= sizeof(uint16_t); + if (nalu_size > length_remaining) + return false; + nalu_ptr += nalu_size; + length_remaining -= nalu_size; + } + return true; +} + bool ParseSingleNalu(RtpDepacketizer::ParsedPayload* parsed_payload, const uint8_t* payload_data, size_t payload_data_length) { @@ -59,6 +76,11 @@ bool ParseSingleNalu(RtpDepacketizer::ParsedPayload* parsed_payload, LOG(LS_ERROR) << "StapA header truncated."; return false; } + if (!VerifyStapANaluLengths(nalu_start, nalu_length)) { + LOG(LS_ERROR) << "StapA packet with incorrect NALU packet lengths."; + return false; + } + nal_type = payload_data[kStapAHeaderSize] & kTypeMask; nalu_start += kStapAHeaderSize; nalu_length -= kStapAHeaderSize; diff --git a/webrtc/modules/video_coding/main/source/session_info.cc b/webrtc/modules/video_coding/main/source/session_info.cc index 8eba432643..49839e5dff 100644 --- a/webrtc/modules/video_coding/main/source/session_info.cc +++ b/webrtc/modules/video_coding/main/source/session_info.cc @@ -133,6 +133,8 @@ size_t VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer, // We handle H.264 STAP-A packets in a special way as we need to remove the // two length bytes between each NAL unit, and potentially add start codes. + // TODO(pbos): Remove H264 parsing from this step and use a fragmentation + // header supplied by the H264 depacketizer. const size_t kH264NALHeaderLengthInBytes = 1; const size_t kLengthFieldLength = 2; if (packet.codecSpecificHeader.codec == kRtpVideoH264 && diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 2548659ef5..185ed2661b 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -132,6 +132,7 @@ void CallTest::CreateFrameGeneratorCapturer() { stream.max_framerate, clock_)); } + void CallTest::CreateStreams() { assert(send_stream_ == NULL); assert(receive_streams_.empty()); diff --git a/webrtc/video/packet_injection_tests.cc b/webrtc/video/packet_injection_tests.cc new file mode 100644 index 0000000000..7e1d98f6b4 --- /dev/null +++ b/webrtc/video/packet_injection_tests.cc @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "testing/gtest/include/gtest/gtest.h" + +#include "webrtc/test/call_test.h" +#include "webrtc/test/null_transport.h" + +namespace webrtc { + +class PacketInjectionTest : public test::CallTest { + protected: + enum class CodecType { + kVp8, + kH264, + }; + + PacketInjectionTest() : rtp_header_parser_(RtpHeaderParser::Create()) {} + + void InjectIncorrectPacket(CodecType codec_type, + uint8_t packet_type, + const uint8_t* packet, + size_t length); + + rtc::scoped_ptr rtp_header_parser_; +}; + +void PacketInjectionTest::InjectIncorrectPacket(CodecType codec_type, + uint8_t payload_type, + const uint8_t* packet, + size_t length) { + test::NullTransport transport; + CreateSenderCall(Call::Config(&transport)); + CreateReceiverCall(Call::Config(&transport)); + + CreateSendConfig(1); + CreateMatchingReceiveConfigs(); + receive_configs_[0].decoders[0].payload_type = payload_type; + switch (codec_type) { + case CodecType::kVp8: + receive_configs_[0].decoders[0].payload_name = "VP8"; + break; + case CodecType::kH264: + receive_configs_[0].decoders[0].payload_name = "H264"; + break; + } + CreateStreams(); + + RTPHeader header; + EXPECT_TRUE(rtp_header_parser_->Parse(packet, length, &header)); + EXPECT_EQ(kSendSsrcs[0], header.ssrc) + << "Packet should have configured SSRC to not be dropped early."; + EXPECT_EQ(payload_type, header.payloadType); + Start(); + EXPECT_EQ(PacketReceiver::DELIVERY_PACKET_ERROR, + receiver_call_->Receiver()->DeliverPacket(MediaType::VIDEO, packet, + length)); + Stop(); + + DestroyStreams(); +} + +TEST_F(PacketInjectionTest, StapAPacketWithTruncatedNalUnits) { + const uint8_t kPacket[] = {0x80, + 0xE5, + 0xE6, + 0x0, + 0x0, + 0xED, + 0x23, + 0x4, + 0x00, + 0xC0, + 0xFF, + 0xED, + 0x58, + 0xCB, + 0xED, + 0xDF}; + + InjectIncorrectPacket(CodecType::kH264, 101, kPacket, sizeof(kPacket)); +} + +} // namespace webrtc diff --git a/webrtc/webrtc_tests.gypi b/webrtc/webrtc_tests.gypi index 2de51ce05e..9d302f18ad 100644 --- a/webrtc/webrtc_tests.gypi +++ b/webrtc/webrtc_tests.gypi @@ -149,6 +149,7 @@ 'tools/agc/agc_manager_unittest.cc', 'video/bitrate_estimator_tests.cc', 'video/end_to_end_tests.cc', + 'video/packet_injection_tests.cc', 'video/send_statistics_proxy_unittest.cc', 'video/video_capture_input_unittest.cc', 'video/video_decoder_unittest.cc',