From ee6e4272a4cb28ee6cf38bc73585b82f316b0682 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 19 Apr 2016 12:15:10 +0200 Subject: [PATCH] Fixed undefined shift in parsing Tmmbr, Tmmbn and Remb BUG=chromium:603483 R=asapersson@webrtc.org Review URL: https://codereview.webrtc.org/1888793003 . Cr-Commit-Position: refs/heads/master@{#12423} --- .../rtp_rtcp/source/rtcp_receiver_unittest.cc | 16 ++++- .../modules/rtp_rtcp/source/rtcp_utility.cc | 64 ++++++++++++++----- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc index 96b5198ed3..4924a658a2 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc @@ -1244,6 +1244,20 @@ TEST_F(RtcpReceiverTest, ReceivesTransportFeedback) { EXPECT_TRUE(rtcp_packet_info_.transport_feedback_.get() != nullptr); } +TEST_F(RtcpReceiverTest, ReceivesRemb) { + const uint32_t kSenderSsrc = 0x123456; + const uint32_t kBitrateBps = 500000; + rtcp::Remb remb; + remb.From(kSenderSsrc); + remb.WithBitrateBps(kBitrateBps); + rtc::Buffer built_packet = remb.Build(); + + EXPECT_EQ(0, InjectRtcpPacket(built_packet.data(), built_packet.size())); + + EXPECT_EQ(kRtcpRemb, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpRemb); + EXPECT_EQ(kBitrateBps, rtcp_packet_info_.receiverEstimatedMaxBitrate); +} + TEST_F(RtcpReceiverTest, HandlesInvalidTransportFeedback) { const uint32_t kSenderSsrc = 0x10203; const uint32_t kSourceSsrc = 0x123456; @@ -1261,7 +1275,7 @@ TEST_F(RtcpReceiverTest, HandlesInvalidTransportFeedback) { static uint32_t kBitrateBps = 50000; rtcp::Remb remb; - remb.From(kSourceSsrc); + remb.From(kSenderSsrc); remb.WithBitrateBps(kBitrateBps); rtcp::CompoundPacket compound; compound.Append(&packet); diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc index 9b5a83515f..c4f688aac4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc @@ -14,12 +14,17 @@ #include // ceil #include // memcpy +#include + #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" #include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" namespace webrtc { +namespace { +constexpr uint64_t kMaxBitrateBps = std::numeric_limits::max(); +} // namespace namespace RTCPUtility { @@ -1440,14 +1445,23 @@ RTCPUtility::RTCPParserV2::ParsePsfbREMBItem() } _packet.REMBItem.NumberOfSSRCs = *_ptrRTCPData++; - const uint8_t brExp = (_ptrRTCPData[0] >> 2) & 0x3F; + const uint8_t exp = (_ptrRTCPData[0] >> 2) & 0x3F; - uint32_t brMantissa = (_ptrRTCPData[0] & 0x03) << 16; - brMantissa += (_ptrRTCPData[1] << 8); - brMantissa += (_ptrRTCPData[2]); + uint64_t mantissa = (_ptrRTCPData[0] & 0x03) << 16; + mantissa += (_ptrRTCPData[1] << 8); + mantissa += (_ptrRTCPData[2]); _ptrRTCPData += 3; // Fwd read data - _packet.REMBItem.BitRate = (brMantissa << brExp); + uint64_t bitrate_bps = (mantissa << exp); + bool shift_overflow = exp > 0 && (mantissa >> (64 - exp)) != 0; + if (shift_overflow || bitrate_bps > kMaxBitrateBps) { + LOG(LS_ERROR) << "Unhandled remb bitrate value : " << mantissa + << "*2^" << static_cast(exp); + _state = ParseState::State_TopLevel; + EndCurrentBlock(); + return false; + } + _packet.REMBItem.BitRate = bitrate_bps; const ptrdiff_t length_ssrcs = _ptrRTCPBlockEnd - _ptrRTCPData; if (length_ssrcs < 4 * _packet.REMBItem.NumberOfSSRCs) @@ -1492,18 +1506,28 @@ RTCPUtility::RTCPParserV2::ParseTMMBRItem() _packet.TMMBRItem.SSRC += *_ptrRTCPData++ << 8; _packet.TMMBRItem.SSRC += *_ptrRTCPData++; - uint8_t mxtbrExp = (_ptrRTCPData[0] >> 2) & 0x3F; + uint8_t exp = (_ptrRTCPData[0] >> 2) & 0x3F; - uint32_t mxtbrMantissa = (_ptrRTCPData[0] & 0x03) << 15; - mxtbrMantissa += (_ptrRTCPData[1] << 7); - mxtbrMantissa += (_ptrRTCPData[2] >> 1) & 0x7F; + uint64_t mantissa = (_ptrRTCPData[0] & 0x03) << 15; + mantissa += (_ptrRTCPData[1] << 7); + mantissa += (_ptrRTCPData[2] >> 1) & 0x7F; uint32_t measuredOH = (_ptrRTCPData[2] & 0x01) << 8; measuredOH += _ptrRTCPData[3]; _ptrRTCPData += 4; // Fwd read data - _packet.TMMBRItem.MaxTotalMediaBitRate = ((mxtbrMantissa << mxtbrExp) / 1000); + uint64_t bitrate_bps = (mantissa << exp); + bool shift_overflow = exp > 0 && (mantissa >> (64 - exp)) != 0; + if (shift_overflow || bitrate_bps > kMaxBitrateBps) { + LOG(LS_ERROR) << "Unhandled tmmbr bitrate value : " << mantissa + << "*2^" << static_cast(exp); + _state = ParseState::State_TopLevel; + EndCurrentBlock(); + return false; + } + + _packet.TMMBRItem.MaxTotalMediaBitRate = bitrate_bps / 1000; _packet.TMMBRItem.MeasuredOverhead = measuredOH; return true; @@ -1531,18 +1555,28 @@ RTCPUtility::RTCPParserV2::ParseTMMBNItem() _packet.TMMBNItem.SSRC += *_ptrRTCPData++ << 8; _packet.TMMBNItem.SSRC += *_ptrRTCPData++; - uint8_t mxtbrExp = (_ptrRTCPData[0] >> 2) & 0x3F; + uint8_t exp = (_ptrRTCPData[0] >> 2) & 0x3F; - uint32_t mxtbrMantissa = (_ptrRTCPData[0] & 0x03) << 15; - mxtbrMantissa += (_ptrRTCPData[1] << 7); - mxtbrMantissa += (_ptrRTCPData[2] >> 1) & 0x7F; + uint64_t mantissa = (_ptrRTCPData[0] & 0x03) << 15; + mantissa += (_ptrRTCPData[1] << 7); + mantissa += (_ptrRTCPData[2] >> 1) & 0x7F; uint32_t measuredOH = (_ptrRTCPData[2] & 0x01) << 8; measuredOH += _ptrRTCPData[3]; _ptrRTCPData += 4; // Fwd read data - _packet.TMMBNItem.MaxTotalMediaBitRate = ((mxtbrMantissa << mxtbrExp) / 1000); + uint64_t bitrate_bps = (mantissa << exp); + bool shift_overflow = exp > 0 && (mantissa >> (64 - exp)) != 0; + if (shift_overflow || bitrate_bps > kMaxBitrateBps) { + LOG(LS_ERROR) << "Unhandled tmmbn bitrate value : " << mantissa + << "*2^" << static_cast(exp); + _state = ParseState::State_TopLevel; + EndCurrentBlock(); + return false; + } + + _packet.TMMBNItem.MaxTotalMediaBitRate = bitrate_bps / 1000; _packet.TMMBNItem.MeasuredOverhead = measuredOH; return true;