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}
diff --git a/media/engine/webrtc_media_engine.cc b/media/engine/webrtc_media_engine.cc
index 6ce52e4..f083b9c 100644
--- a/media/engine/webrtc_media_engine.cc
+++ b/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 @@
} // 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 @@
}
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 @@
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;
diff --git a/media/engine/webrtc_media_engine.h b/media/engine/webrtc_media_engine.h
index 34ec4cd..ff97760 100644
--- a/media/engine/webrtc_media_engine.h
+++ b/media/engine/webrtc_media_engine.h
@@ -63,8 +63,11 @@
MediaEngineDependencies dependencies);
// Verify that extension IDs are within 1-byte extension range and are not
-// overlapping.
-bool ValidateRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions);
+// overlapping, and that they form a legal change from previously registerd
+// 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
diff --git a/media/engine/webrtc_media_engine_unittest.cc b/media/engine/webrtc_media_engine_unittest.cc
index 78d13fc..81982fa 100644
--- a/media/engine/webrtc_media_engine_unittest.cc
+++ b/media/engine/webrtc_media_engine_unittest.cc
@@ -66,41 +66,68 @@
}
} // 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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_KeepAbsSendTime) {
+ FilterRtpExtensionsRemoveRedundantBwe1KeepAbsSendTime) {
std::vector<RtpExtension> extensions;
extensions.push_back(
RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3));
@@ -226,7 +253,7 @@
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_KeepAbsSendTime) {
+ FilterRtpExtensionsRemoveRedundantBweEncrypted1KeepAbsSendTime) {
std::vector<RtpExtension> extensions;
extensions.push_back(
RtpExtension(RtpExtension::kTransportSequenceNumberUri, 3));
@@ -274,7 +301,7 @@
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 @@
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));
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 5de6752..bdce2ba 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -827,7 +827,7 @@
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 @@
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 @@
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 @@
const VideoRecvParameters& params,
ChangedRecvParameters* changed_params) const {
if (!ValidateCodecFormats(params.codecs) ||
- !ValidateRtpExtensions(params.extensions)) {
+ !ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) {
return false;
}
diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h
index 8b3a7f4..90d824a 100644
--- a/media/engine/webrtc_video_engine.h
+++ b/media/engine/webrtc_video_engine.h
@@ -598,7 +598,7 @@
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_
diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc
index 448ad35..7ce5063 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -1384,7 +1384,7 @@
return false;
}
- if (!ValidateRtpExtensions(params.extensions)) {
+ if (!ValidateRtpExtensions(params.extensions, send_rtp_extensions_)) {
return false;
}
@@ -1430,7 +1430,7 @@
return false;
}
- if (!ValidateRtpExtensions(params.extensions)) {
+ if (!ValidateRtpExtensions(params.extensions, recv_rtp_extensions_)) {
return false;
}
std::vector<webrtc::RtpExtension> filtered_extensions = FilterRtpExtensions(
diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map.cc b/modules/rtp_rtcp/source/rtp_header_extension_map.cc
index 8fcef1d..3c96593 100644
--- a/modules/rtp_rtcp/source/rtp_header_extension_map.cc
+++ b/modules/rtp_rtcp/source/rtp_header_extension_map.cc
@@ -154,7 +154,12 @@
<< 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);
diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc b/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc
index 19a17a5..42842cc 100644
--- a/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_header_extension_map_unittest.cc
@@ -106,4 +106,10 @@
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
