From 58563b4fba5b0fbe8e206ad849c8ad70ff84534f Mon Sep 17 00:00:00 2001 From: chenyang Date: Thu, 21 Mar 2024 18:00:58 +0800 Subject: [PATCH 1/2] =?UTF-8?q?=E4=BF=AE=E5=A4=8DCVE-2023-4076?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: chenyang --- pc/peer_connection_bundle_unittest.cc | 51 +++++++++++++++++++-------- pc/sdp_offer_answer.cc | 19 +++++++++- pc/sdp_offer_answer_unittest.cc | 17 +++++++++ 3 files changed, 72 insertions(+), 15 deletions(-) diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index a71b5138..875a77cb 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -730,16 +730,17 @@ TEST_P(PeerConnectionBundleTest, BundleOnFirstMidInAnswer) { } // This tests that applying description with conflicted RTP demuxing criteria -// will fail. -TEST_P(PeerConnectionBundleTest, - ApplyDescriptionWithConflictedDemuxCriteriaFail) { +// will fail when using BUNDLE. +TEST_P(PeerConnectionBundleTest, ApplyDescriptionWithSameSsrcsBundledFails) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); RTCOfferAnswerOptions options; - options.use_rtp_mux = false; + options.use_rtp_mux = true; auto offer = caller->CreateOffer(options); - // Modified the SDP to make two m= sections have the same SSRC. + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + // Modify the remote SDP to make two m= sections have the same SSRC. ASSERT_GE(offer->description()->contents().size(), 2U); offer->description() ->contents()[0] @@ -751,20 +752,42 @@ TEST_P(PeerConnectionBundleTest, .media_description() ->mutable_streams()[0] .ssrcs[0] = 1111222; + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); + + // When BUNDLE is enabled, applying the description is expected to fail + // because the demuxing criteria can not be satisfied. + auto answer = callee->CreateAnswer(options); + EXPECT_FALSE(callee->SetLocalDescription(std::move(answer))); +} + +// A variant of the above, without BUNDLE duplicate SSRCs are allowed. +TEST_P(PeerConnectionBundleTest, + ApplyDescriptionWithSameSsrcsUnbundledSucceeds) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + RTCOfferAnswerOptions options; + options.use_rtp_mux = false; + auto offer = caller->CreateOffer(options); EXPECT_TRUE( caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + // Modify the remote SDP to make two m= sections have the same SSRC. + ASSERT_GE(offer->description()->contents().size(), 2U); + offer->description() + ->contents()[0] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = 1111222; + offer->description() + ->contents()[1] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = 1111222; EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); - EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal(options)); - // Enable BUNDLE in subsequent offer/answer exchange and two m= sections are - // expectd to use one RtpTransport underneath. - options.use_rtp_mux = true; - EXPECT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(options))); + // Without BUNDLE, demuxing is done per-transport. auto answer = callee->CreateAnswer(options); - // When BUNDLE is enabled, applying the description is expected to fail - // because the demuxing criteria is conflicted. - EXPECT_FALSE(callee->SetLocalDescription(std::move(answer))); + EXPECT_TRUE(callee->SetLocalDescription(std::move(answer))); } // This tests that changing the pre-negotiated BUNDLE tag is not supported. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 0cb10b7d..a314778b 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1738,7 +1738,7 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( if (type == SdpType::kOffer) { // TODO(bugs.webrtc.org/4676) - Handle CreateChannel failure, as new local // description is applied. Restore back to old description. - RTCError error = CreateChannels(*local_description()->description()); + error = CreateChannels(*local_description()->description()); if (!error.ok()) { RTC_LOG(LS_ERROR) << error.message() << " (" << SdpTypeToString(type) << ")"; @@ -1770,6 +1770,23 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( // SCTP sids. AllocateSctpSids(); + // Validate SSRCs, we do not allow duplicates. + if (ConfiguredForMedia()) { + std::set used_ssrcs; + for (const auto& content : local_description()->description()->contents()) { + for (const auto& stream : content.media_description()->streams()) { + for (uint32_t ssrc : stream.ssrcs) { + auto result = used_ssrcs.insert(ssrc); + if (!result.second) { + LOG_AND_RETURN_ERROR( + RTCErrorType::INVALID_PARAMETER, + "Duplicate ssrc " + rtc::ToString(ssrc) + " is not allowed"); + } + } + } + } + } + if (IsUnifiedPlan()) { if (ConfiguredForMedia()) { // We must use List and not ListInternal here because diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 4c4554f1..77689049 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -12,6 +12,7 @@ #include #include +#include "absl/strings/str_replace.h" #include "api/audio/audio_mixer.h" #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" @@ -547,4 +548,20 @@ TEST_F(SdpOfferAnswerTest, RejectedDataChannelsDoGetReofferedWhenActive) { #endif // WEBRTC_HAVE_SCTP +TEST_F(SdpOfferAnswerTest, DuplicateSsrcsDisallowedInLocalDescription) { + auto pc = CreatePeerConnection(); + pc->AddAudioTrack("audio_track", {}); + pc->AddVideoTrack("video_track", {}); + auto offer = pc->CreateOffer(); + auto& offer_contents = offer->description()->contents(); + ASSERT_EQ(offer_contents.size(), 2u); + uint32_t second_ssrc = offer_contents[1].media_description()->first_ssrc(); + offer->description() + ->contents()[0] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = second_ssrc; + EXPECT_FALSE(pc->SetLocalDescription(std::move(offer))); +} + } // namespace webrtc -- Gitee From 0c5afa8a9fb4a4e34a3214681938b94dce42e8a5 Mon Sep 17 00:00:00 2001 From: chenyang Date: Thu, 21 Mar 2024 18:06:01 +0800 Subject: [PATCH 2/2] =?UTF-8?q?=E4=BF=AE=E5=A4=8DCVE-2023-4076?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: chenyang --- pc/sdp_offer_answer_unittest.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 77689049..4f3ce60c 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -556,6 +556,7 @@ TEST_F(SdpOfferAnswerTest, DuplicateSsrcsDisallowedInLocalDescription) { auto& offer_contents = offer->description()->contents(); ASSERT_EQ(offer_contents.size(), 2u); uint32_t second_ssrc = offer_contents[1].media_description()->first_ssrc(); + offer->description() ->contents()[0] .media_description() -- Gitee