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

@@ -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.

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) {
+RemoteEstimate::RemoteEstimate(App&& app)
-  if (packet.fmt() != kSubType)
+    : App(std::move(app)), serializer_(GetRemoteEstimateSerializer()) {}
-    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;
-}
 
-bool RemoteEstimate::Parse(const CommonHeader& packet) {
+bool RemoteEstimate::ParseData() {
-  if (!App::Parse(packet))
-    return false;
   return serializer_->Parse({data(), data_size()}, &estimate_);
 }
 

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 ParseData();
-  bool Parse(const CommonHeader& packet);
   void SetEstimate(NetworkStateEstimate estimate);
   NetworkStateEstimate estimate() const { return estimate_; }
 

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::App app;
-    rtcp::RemoteEstimate estimate;
+  if (app.Parse(rtcp_block)) {
-    if (estimate.Parse(rtcp_block)) {
+    if (app.name() == rtcp::RemoteEstimate::kName &&
-      packet_information->network_state_estimate = estimate.estimate();
+        app.sub_type() == rtcp::RemoteEstimate::kSubType) {
-      return;
+      rtcp::RemoteEstimate estimate(std::move(app));
+      if (estimate.ParseData()) {
+        packet_information->network_state_estimate = estimate.estimate();
+        return;
+      }
     }
   }
   ++num_skipped_packets_;