Detect and reject illegal RTP header extension modifications.

This is somewhat klugey, because it does the same checks at two
different layers in the stack, in different functions, which runs
the risk of making them out of sync. But it solves the immediate
problem.

Bug: chromium:1249753
Change-Id: I2ad96f0cc9499c15540ff6946a409b40df3e3925
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/235826
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35259}

media/engine/webrtc_media_engine.cc

@@ -10,6 +10,7 @@
 
 #include "media/engine/webrtc_media_engine.h"
 
+#include <map>
 #include <memory>
 #include <utility>
 
@@ -74,7 +75,8 @@ void DiscardRedundantExtensions(
 }  // namespace
 
 bool ValidateRtpExtensions(
-    const std::vector<webrtc::RtpExtension>& extensions) {
+    rtc::ArrayView<const webrtc::RtpExtension> extensions,
+    rtc::ArrayView<const webrtc::RtpExtension> old_extensions) {
   bool id_used[1 + webrtc::RtpExtension::kMaxId] = {false};
   for (const auto& extension : extensions) {
     if (extension.id < webrtc::RtpExtension::kMinId ||
@@ -89,6 +91,45 @@ bool ValidateRtpExtensions(
     }
     id_used[extension.id] = true;
   }
+  // Validate the extension list against the already negotiated extensions.
+  // Re-registering is OK, re-mapping (either same URL at new ID or same
+  // ID used with new URL) is an illegal remap.
+
+  // This is required in order to avoid a crash when registering an
+  // extension. A better structure would use the registered extensions
+  // in the RTPSender. This requires spinning through:
+  //
+  // WebRtcVoiceMediaChannel::::WebRtcAudioSendStream::stream_ (pointer)
+  // AudioSendStream::rtp_rtcp_module_ (pointer)
+  // ModuleRtpRtcpImpl2::rtp_sender_ (pointer)
+  // RtpSenderContext::packet_generator (struct member)
+  // RTPSender::rtp_header_extension_map_ (class member)
+  //
+  // Getting at this seems like a hard slog.
+  if (!old_extensions.empty()) {
+    absl::string_view urimap[1 + webrtc::RtpExtension::kMaxId];
+    std::map<absl::string_view, int> idmap;
+    for (const auto& old_extension : old_extensions) {
+      urimap[old_extension.id] = old_extension.uri;
+      idmap[old_extension.uri] = old_extension.id;
+    }
+    for (const auto& extension : extensions) {
+      if (!urimap[extension.id].empty() &&
+          urimap[extension.id] != extension.uri) {
+        RTC_LOG(LS_ERROR) << "Extension negotiation failure: " << extension.id
+                          << " was mapped to " << urimap[extension.id]
+                          << " but is proposed changed to " << extension.uri;
+        return false;
+      }
+      const auto& it = idmap.find(extension.uri);
+      if (it != idmap.end() && it->second != extension.id) {
+        RTC_LOG(LS_ERROR) << "Extension negotation failure: " << extension.uri
+                          << " was identified by " << it->second
+                          << " but is proposed changed to " << extension.id;
+        return false;
+      }
+    }
+  }
   return true;
 }
 
@@ -97,7 +138,8 @@ std::vector<webrtc::RtpExtension> FilterRtpExtensions(
     bool (*supported)(absl::string_view),
     bool filter_redundant_extensions,
     const webrtc::WebRtcKeyValueConfig& trials) {
-  RTC_DCHECK(ValidateRtpExtensions(extensions));
+  // Don't check against old parameters; this should have been done earlier.
+  RTC_DCHECK(ValidateRtpExtensions(extensions, {}));
   RTC_DCHECK(supported);
   std::vector<webrtc::RtpExtension> result;
 

media/engine/webrtc_media_engine.h

@@ -63,8 +63,11 @@ RTC_EXPORT std::unique_ptr<MediaEngineInterface> CreateMediaEngine(
     MediaEngineDependencies dependencies);
 
 // Verify that extension IDs are within 1-byte extension range and are not
-// overlapping.
+// overlapping, and that they form a legal change from previously registerd
-bool ValidateRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions);
+// extensions (if any).
+bool ValidateRtpExtensions(
+    rtc::ArrayView<const webrtc::RtpExtension> extennsions,
+    rtc::ArrayView<const webrtc::RtpExtension> old_extensions);
 
 // Discard any extensions not validated by the 'supported' predicate. Duplicate
 // extensions are removed if 'filter_redundant_extensions' is set, and also any

media/engine/webrtc_media_engine_unittest.cc

@@ -66,41 +66,68 @@ bool IsSorted(const std::vector<webrtc::RtpExtension>& extensions) {
 }
 }  // namespace
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_EmptyList) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsEmptyList) {
   std::vector<RtpExtension> extensions;
-  EXPECT_TRUE(ValidateRtpExtensions(extensions));
+  EXPECT_TRUE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_AllGood) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsAllGood) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
-  EXPECT_TRUE(ValidateRtpExtensions(extensions));
+  EXPECT_TRUE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OutOfRangeId_Low) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOutOfRangeId_Low) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   extensions.push_back(RtpExtension("foo", 0));
-  EXPECT_FALSE(ValidateRtpExtensions(extensions));
+  EXPECT_FALSE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OutOfRangeId_High) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOutOfRangeIdHigh) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   extensions.push_back(RtpExtension("foo", 256));
-  EXPECT_FALSE(ValidateRtpExtensions(extensions));
+  EXPECT_FALSE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_StartOfSet) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOverlappingIdsStartOfSet) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   extensions.push_back(RtpExtension("foo", 1));
-  EXPECT_FALSE(ValidateRtpExtensions(extensions));
+  EXPECT_FALSE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_EndOfSet) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOverlappingIdsEndOfSet) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   extensions.push_back(RtpExtension("foo", 255));
-  EXPECT_FALSE(ValidateRtpExtensions(extensions));
+  EXPECT_FALSE(ValidateRtpExtensions(extensions, {}));
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_EmptyList) {
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsEmptyToEmpty) {
+  std::vector<RtpExtension> extensions;
+  EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions));
+}
+
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsNoChange) {
+  std::vector<RtpExtension> extensions = MakeUniqueExtensions();
+  EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions));
+}
+
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsChangeIdNotUrl) {
+  std::vector<RtpExtension> old_extensions = MakeUniqueExtensions();
+  std::vector<RtpExtension> new_extensions = old_extensions;
+  std::swap(new_extensions[0].id, new_extensions[1].id);
+
+  EXPECT_FALSE(ValidateRtpExtensions(new_extensions, old_extensions));
+}
+
+TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsChangeIdForUrl) {
+  std::vector<RtpExtension> old_extensions = MakeUniqueExtensions();
+  std::vector<RtpExtension> new_extensions = old_extensions;
+  // Change first extension to something not generated by MakeUniqueExtensions
+  new_extensions[0].id = 123;
+
+  EXPECT_FALSE(ValidateRtpExtensions(new_extensions, old_extensions));
+}
+
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsEmptyList) {
   std::vector<RtpExtension> extensions;
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -108,7 +135,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_EmptyList) {
   EXPECT_EQ(0u, filtered.size());
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_IncludeOnlySupported) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsIncludeOnlySupported) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -118,7 +145,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_IncludeOnlySupported) {
   EXPECT_EQ("i", filtered[1].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_1) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsSortedByName1) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -127,7 +154,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_1) {
   EXPECT_TRUE(IsSorted(filtered));
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsSortedByName2) {
   std::vector<RtpExtension> extensions = MakeUniqueExtensions();
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -136,7 +163,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) {
   EXPECT_TRUE(IsSorted(filtered));
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_DontRemoveRedundant) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsDontRemoveRedundant) {
   std::vector<RtpExtension> extensions = MakeRedundantExtensions();
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -146,7 +173,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_DontRemoveRedundant) {
   EXPECT_EQ(filtered[0].uri, filtered[1].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundant) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundant) {
   std::vector<RtpExtension> extensions = MakeRedundantExtensions();
   webrtc::FieldTrialBasedConfig trials;
   std::vector<webrtc::RtpExtension> filtered =
@@ -156,7 +183,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundant) {
   EXPECT_NE(filtered[0].uri, filtered[1].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_1) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantEncrypted1) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(webrtc::RtpExtension("b", 1));
   extensions.push_back(webrtc::RtpExtension("b", 2, true));
@@ -173,7 +200,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_1) {
   EXPECT_NE(filtered[1].uri, filtered[2].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_2) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantEncrypted2) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(webrtc::RtpExtension("b", 1, true));
   extensions.push_back(webrtc::RtpExtension("b", 2));
@@ -190,7 +217,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantEncrypted_2) {
   EXPECT_NE(filtered[1].uri, filtered[2].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_1) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe1) {
   webrtc::test::ScopedFieldTrials override_field_trials_(
       "WebRTC-FilterAbsSendTimeExtension/Enabled/");
   webrtc::FieldTrialBasedConfig trials;
@@ -209,7 +236,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_1) {
 }
 
 TEST(WebRtcMediaEngineTest,
-     FilterRtpExtensions_RemoveRedundantBwe_1_KeepAbsSendTime) {
+     FilterRtpExtensionsRemoveRedundantBwe1KeepAbsSendTime) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(
       RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3));
@@ -226,7 +253,7 @@ TEST(WebRtcMediaEngineTest,
   EXPECT_EQ(RtpExtension::kAbsSendTimeUri, filtered[1].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBweEncrypted_1) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBweEncrypted1) {
   webrtc::test::ScopedFieldTrials override_field_trials_(
       "WebRTC-FilterAbsSendTimeExtension/Enabled/");
   webrtc::FieldTrialBasedConfig trials;
@@ -251,7 +278,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBweEncrypted_1) {
 }
 
 TEST(WebRtcMediaEngineTest,
-     FilterRtpExtensions_RemoveRedundantBweEncrypted_1_KeepAbsSendTime) {
+     FilterRtpExtensionsRemoveRedundantBweEncrypted1KeepAbsSendTime) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(
       RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3));
@@ -274,7 +301,7 @@ TEST(WebRtcMediaEngineTest,
   EXPECT_NE(filtered[0].encrypt, filtered[1].encrypt);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_2) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe2) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 1));
   extensions.push_back(RtpExtension(RtpExtension::kAbsSendTimeUri, 14));
@@ -286,7 +313,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_2) {
   EXPECT_EQ(RtpExtension::kAbsSendTimeUri, filtered[0].uri);
 }
 
-TEST(WebRtcMediaEngineTest, FilterRtpExtensions_RemoveRedundantBwe_3) {
+TEST(WebRtcMediaEngineTest, FilterRtpExtensionsRemoveRedundantBwe3) {
   std::vector<RtpExtension> extensions;
   extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 2));
   extensions.push_back(RtpExtension(RtpExtension::kTimestampOffsetUri, 14));

media/engine/webrtc_video_engine.cc

@@ -827,7 +827,7 @@ bool WebRtcVideoChannel::GetChangedSendParameters(
     const VideoSendParameters& params,
     ChangedSendParameters* changed_params) const {
   if (!ValidateCodecFormats(params.codecs) ||
-      !ValidateRtpExtensions(params.extensions)) {
+      !ValidateRtpExtensions(params.extensions, send_rtp_extensions_)) {
     return false;
   }
 
@@ -862,7 +862,7 @@ bool WebRtcVideoChannel::GetChangedSendParameters(
   std::vector<webrtc::RtpExtension> filtered_extensions = FilterRtpExtensions(
       params.extensions, webrtc::RtpExtension::IsSupportedForVideo, true,
       call_->trials());
-  if (!send_rtp_extensions_ || (*send_rtp_extensions_ != filtered_extensions)) {
+  if (send_rtp_extensions_ != filtered_extensions) {
     changed_params->rtp_header_extensions =
         absl::optional<std::vector<webrtc::RtpExtension>>(filtered_extensions);
   }
@@ -1009,7 +1009,7 @@ bool WebRtcVideoChannel::ApplyChangedParams(
     SetExtmapAllowMixed(*changed_params.extmap_allow_mixed);
   }
   if (changed_params.rtp_header_extensions) {
-    send_rtp_extensions_ = changed_params.rtp_header_extensions;
+    send_rtp_extensions_ = *changed_params.rtp_header_extensions;
   }
 
   if (changed_params.send_codec || changed_params.max_bandwidth_bps) {
@@ -1186,7 +1186,7 @@ bool WebRtcVideoChannel::GetChangedRecvParameters(
     const VideoRecvParameters& params,
     ChangedRecvParameters* changed_params) const {
   if (!ValidateCodecFormats(params.codecs) ||
-      !ValidateRtpExtensions(params.extensions)) {
+      !ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) {
     return false;
   }
 

media/engine/webrtc_video_engine.h

@@ -598,7 +598,7 @@ class WebRtcVideoChannel : public VideoMediaChannel,
   std::vector<VideoCodecSettings> negotiated_codecs_
       RTC_GUARDED_BY(thread_checker_);
 
-  absl::optional<std::vector<webrtc::RtpExtension>> send_rtp_extensions_
+  std::vector<webrtc::RtpExtension> send_rtp_extensions_
       RTC_GUARDED_BY(thread_checker_);
 
   webrtc::VideoEncoderFactory* const encoder_factory_

media/engine/webrtc_voice_engine.cc

@@ -1384,7 +1384,7 @@ bool WebRtcVoiceMediaChannel::SetSendParameters(
     return false;
   }
 
-  if (!ValidateRtpExtensions(params.extensions)) {
+  if (!ValidateRtpExtensions(params.extensions, send_rtp_extensions_)) {
     return false;
   }
 
@@ -1430,7 +1430,7 @@ bool WebRtcVoiceMediaChannel::SetRecvParameters(
     return false;
   }
 
-  if (!ValidateRtpExtensions(params.extensions)) {
+  if (!ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) {
     return false;
   }
   std::vector<webrtc::RtpExtension> filtered_extensions = FilterRtpExtensions(

modules/rtp_rtcp/source/rtp_header_extension_map.cc

@@ -154,7 +154,12 @@ bool RtpHeaderExtensionMap::Register(int id,
                         << static_cast<int>(registered_type);
     return false;
   }
-  RTC_DCHECK(!IsRegistered(type));
+  if (IsRegistered(type)) {
+    RTC_LOG(LS_WARNING) << "Illegal reregistration for uri: " << uri
+                        << " is previously registered with id " << GetId(type)
+                        << " and cannot be reregistered with id " << id;
+    return false;
+  }
 
   // There is a run-time check above id fits into uint8_t.
   ids_[type] = static_cast<uint8_t>(id);

modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc

@@ -106,4 +106,10 @@ TEST(RtpHeaderExtensionTest, GetId) {
   EXPECT_EQ(3, map.GetId(TransmissionOffset::kId));
 }
 
+TEST(RtpHeaderExtensionTest, RemapFails) {
+  RtpHeaderExtensionMap map;
+  EXPECT_TRUE(map.Register<TransmissionOffset>(3));
+  EXPECT_FALSE(map.Register<TransmissionOffset>(4));
+}
+
 }  // namespace webrtc