From a72d5832710b4967f61b8878ef5d73cb91c0b0a6 Mon Sep 17 00:00:00 2001
From: Sebastian Jansson <srte@webrtc.org>
Date: Thu, 25 Jul 2019 09:50:48 +0200
Subject: [PATCH] Fix for potential out of bounds reading in
 rtcp::RemoteEstimate parser.

packet_size() includes the size of padding, this means that the size
check might incorrectly not trigger even if the payload is empty. In
turn this means that the ReadBigEndian call might read out of bounds
memory.

Refactored the code to reuse the App parsing code more, eliminating
the risk of this particular kind of error.

Bug: chromium:987507
Change-Id: Id8f3e292c3d30460d3cdb551f0a45070fdf8f022
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146716
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28680}
---
 modules/rtp_rtcp/source/rtcp_packet/app.h       |  1 +
 .../source/rtcp_packet/remote_estimate.cc       | 17 ++++-------------
 .../source/rtcp_packet/remote_estimate.h        |  4 ++--
 modules/rtp_rtcp/source/rtcp_receiver.cc        | 14 +++++++++-----
 4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/modules/rtp_rtcp/source/rtcp_packet/app.h b/modules/rtp_rtcp/source/rtcp_packet/app.h
index ff5f52dbf8..990ff3645c 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/app.h
+++ b/modules/rtp_rtcp/source/rtcp_packet/app.h
@@ -25,6 +25,7 @@ class App : public RtcpPacket {
  public:
   static constexpr uint8_t kPacketType = 204;
   App();
+  App(App&&) = default;
   ~App() override;
 
   // Parse assumes header is already parsed and validated.
diff --git a/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.cc b/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.cc
index 81d1a1abfb..82b0b2f9d1 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.cc
+++ b/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.cc
@@ -12,6 +12,7 @@
 #include <algorithm>
 #include <cmath>
 #include <type_traits>
+#include <utility>
 #include <vector>
 
 #include "modules/rtp_rtcp/source/byte_io.h"
@@ -130,20 +131,10 @@ RemoteEstimate::RemoteEstimate() : serializer_(GetRemoteEstimateSerializer()) {
   SetSsrc(0);
 }
 
-bool RemoteEstimate::IsNetworkEstimate(const CommonHeader& packet) {
-  if (packet.fmt() != kSubType)
-    return false;
-  size_t kNameSize = sizeof(uint32_t);
-  if (packet.packet_size() < CommonHeader::kHeaderSizeBytes + kNameSize)
-    return false;
-  if (ByteReader<uint32_t>::ReadBigEndian(&packet.payload()[4]) != kName)
-    return false;
-  return true;
-}
+RemoteEstimate::RemoteEstimate(App&& app)
+    : App(std::move(app)), serializer_(GetRemoteEstimateSerializer()) {}
 
-bool RemoteEstimate::Parse(const CommonHeader& packet) {
-  if (!App::Parse(packet))
-    return false;
+bool RemoteEstimate::ParseData() {
   return serializer_->Parse({data(), data_size()}, &estimate_);
 }
 
diff --git a/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h b/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h
index ebf7fef676..3400274568 100644
--- a/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h
+++ b/modules/rtp_rtcp/source/rtcp_packet/remote_estimate.h
@@ -38,13 +38,13 @@ const RemoteEstimateSerializer* GetRemoteEstimateSerializer();
 class RemoteEstimate : public App {
  public:
   RemoteEstimate();
+  explicit RemoteEstimate(App&& app);
   // Note, sub type must be unique among all app messages with "goog" name.
   static constexpr uint8_t kSubType = 13;
   static constexpr uint32_t kName = NameToInt("goog");
   static TimeDelta GetTimestampPeriod();
 
-  static bool IsNetworkEstimate(const CommonHeader& packet);
-  bool Parse(const CommonHeader& packet);
+  bool ParseData();
   void SetEstimate(NetworkStateEstimate estimate);
   NetworkStateEstimate estimate() const { return estimate_; }
 
diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc
index c73c7adf3b..7754ab13e9 100644
--- a/modules/rtp_rtcp/source/rtcp_receiver.cc
+++ b/modules/rtp_rtcp/source/rtcp_receiver.cc
@@ -698,11 +698,15 @@ void RTCPReceiver::HandleNack(const CommonHeader& rtcp_block,
 
 void RTCPReceiver::HandleApp(const rtcp::CommonHeader& rtcp_block,
                              PacketInformation* packet_information) {
-  if (rtcp::RemoteEstimate::IsNetworkEstimate(rtcp_block)) {
-    rtcp::RemoteEstimate estimate;
-    if (estimate.Parse(rtcp_block)) {
-      packet_information->network_state_estimate = estimate.estimate();
-      return;
+  rtcp::App app;
+  if (app.Parse(rtcp_block)) {
+    if (app.name() == rtcp::RemoteEstimate::kName &&
+        app.sub_type() == rtcp::RemoteEstimate::kSubType) {
+      rtcp::RemoteEstimate estimate(std::move(app));
+      if (estimate.ParseData()) {
+        packet_information->network_state_estimate = estimate.estimate();
+        return;
+      }
     }
   }
   ++num_skipped_packets_;