From b62ee8ce94e5f10e0a94d6f112e715cc4d0cd9dc Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 21 Oct 2021 22:37:50 +0000 Subject: [PATCH] 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 Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#35259} --- media/engine/webrtc_media_engine.cc | 46 ++++++++++- media/engine/webrtc_media_engine.h | 7 +- media/engine/webrtc_media_engine_unittest.cc | 79 +++++++++++++------ media/engine/webrtc_video_engine.cc | 8 +- media/engine/webrtc_video_engine.h | 2 +- media/engine/webrtc_voice_engine.cc | 4 +- .../source/rtp_header_extension_map.cc | 7 +- .../rtp_header_extension_map_unittest.cc | 6 ++ 8 files changed, 121 insertions(+), 38 deletions(-) diff --git a/media/engine/webrtc_media_engine.cc b/media/engine/webrtc_media_engine.cc index 6ce52e4c8e..f083b9c9ca 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 #include #include @@ -74,7 +75,8 @@ void DiscardRedundantExtensions( } // namespace bool ValidateRtpExtensions( - const std::vector& extensions) { + rtc::ArrayView extensions, + rtc::ArrayView 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 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 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 result; diff --git a/media/engine/webrtc_media_engine.h b/media/engine/webrtc_media_engine.h index 34ec4cdc9c..ff977609b2 100644 --- a/media/engine/webrtc_media_engine.h +++ b/media/engine/webrtc_media_engine.h @@ -63,8 +63,11 @@ RTC_EXPORT std::unique_ptr CreateMediaEngine( MediaEngineDependencies dependencies); // Verify that extension IDs are within 1-byte extension range and are not -// overlapping. -bool ValidateRtpExtensions(const std::vector& extensions); +// overlapping, and that they form a legal change from previously registerd +// extensions (if any). +bool ValidateRtpExtensions( + rtc::ArrayView extennsions, + rtc::ArrayView 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 78d13fc297..81982fae2b 100644 --- a/media/engine/webrtc_media_engine_unittest.cc +++ b/media/engine/webrtc_media_engine_unittest.cc @@ -66,41 +66,68 @@ bool IsSorted(const std::vector& extensions) { } } // namespace -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_EmptyList) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsEmptyList) { std::vector extensions; - EXPECT_TRUE(ValidateRtpExtensions(extensions)); + EXPECT_TRUE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_AllGood) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsAllGood) { std::vector extensions = MakeUniqueExtensions(); - EXPECT_TRUE(ValidateRtpExtensions(extensions)); + EXPECT_TRUE(ValidateRtpExtensions(extensions, {})); } -TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OutOfRangeId_Low) { +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsOutOfRangeId_Low) { std::vector 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 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 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 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 extensions; + EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions)); +} + +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsNoChange) { + std::vector extensions = MakeUniqueExtensions(); + EXPECT_TRUE(ValidateRtpExtensions(extensions, extensions)); +} + +TEST(WebRtcMediaEngineTest, ValidateRtpExtensionsChangeIdNotUrl) { + std::vector old_extensions = MakeUniqueExtensions(); + std::vector 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 old_extensions = MakeUniqueExtensions(); + std::vector 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 extensions; webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -108,7 +135,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_EmptyList) { EXPECT_EQ(0u, filtered.size()); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_IncludeOnlySupported) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsIncludeOnlySupported) { std::vector extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector 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 extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -127,7 +154,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_1) { EXPECT_TRUE(IsSorted(filtered)); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsSortedByName2) { std::vector extensions = MakeUniqueExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector filtered = @@ -136,7 +163,7 @@ TEST(WebRtcMediaEngineTest, FilterRtpExtensions_SortedByName_2) { EXPECT_TRUE(IsSorted(filtered)); } -TEST(WebRtcMediaEngineTest, FilterRtpExtensions_DontRemoveRedundant) { +TEST(WebRtcMediaEngineTest, FilterRtpExtensionsDontRemoveRedundant) { std::vector extensions = MakeRedundantExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector 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 extensions = MakeRedundantExtensions(); webrtc::FieldTrialBasedConfig trials; std::vector 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 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 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 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 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 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 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 5de6752d64..bdce2ba7b6 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/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 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>(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; } diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 8b3a7f42c6..90d824a55b 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -598,7 +598,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, std::vector negotiated_codecs_ RTC_GUARDED_BY(thread_checker_); - absl::optional> send_rtp_extensions_ + std::vector 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 448ad3522a..7ce5063cfe 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/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 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 8fcef1de49..3c9659343e 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 @@ bool RtpHeaderExtensionMap::Register(int id, << static_cast(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(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 19a17a5ecc..42842cc876 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 @@ TEST(RtpHeaderExtensionTest, GetId) { EXPECT_EQ(3, map.GetId(TransmissionOffset::kId)); } +TEST(RtpHeaderExtensionTest, RemapFails) { + RtpHeaderExtensionMap map; + EXPECT_TRUE(map.Register(3)); + EXPECT_FALSE(map.Register(4)); +} + } // namespace webrtc