From 1f1b0b31e7c7f688a2d9d774d1d375ae3c56fbd1 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 11 Aug 2023 09:32:50 +0200 Subject: [PATCH] sdp: add validation for the number of ssrcs in the ssrc group for the known standard semantics FID (used by rtx) and FEC-FR (used byFlexFEC) they should match the expected two SSRCs. For the nonstandard SIM group this should be limited by the maximum number of simulcast layers supported. BUG=chromium:1459124 Change-Id: I7cc2417a3ab207658ec80e8d7e9984c1ae631f53 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/315323 Reviewed-by: Florent Castelli Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#40652} --- pc/peer_connection_rtp_unittest.cc | 2 +- pc/sdp_offer_answer.cc | 34 +++++++++ pc/sdp_offer_answer_unittest.cc | 116 ++++++++++++++++++++++++++++- 3 files changed, 149 insertions(+), 3 deletions(-) diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index 1ff48cd0a5..4bdb7f1ea6 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -826,7 +826,7 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, TracksDoNotEndWhenSsrcChanges) { for (size_t i = 0; i < contents.size(); ++i) { auto& mutable_streams = contents[i].media_description()->mutable_streams(); ASSERT_EQ(mutable_streams.size(), 1u); - mutable_streams[0].ssrcs = {kFirstMungedSsrc + static_cast(i)}; + mutable_streams[0].ssrcs[0] = kFirstMungedSsrc + static_cast(i); } ASSERT_TRUE( callee->SetLocalDescription(CloneSessionDescription(answer.get()))); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 7d7edd2207..06303de441 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -535,6 +535,33 @@ RTCError ValidateRtpHeaderExtensionsForSpecSimulcast( return RTCError::OK(); } +RTCError ValidateSsrcGroups(const cricket::SessionDescription& description) { + for (const ContentInfo& content : description.contents()) { + if (content.type != MediaProtocolType::kRtp) { + continue; + } + for (const StreamParams& stream : content.media_description()->streams()) { + for (const cricket::SsrcGroup& group : stream.ssrc_groups) { + // Validate the number of SSRCs for standard SSRC group semantics such + // as FID and FEC-FR and the non-standard SIM group. + if ((group.semantics == cricket::kFidSsrcGroupSemantics && + group.ssrcs.size() != 2) || + (group.semantics == cricket::kFecFrSsrcGroupSemantics && + group.ssrcs.size() != 2) || + (group.semantics == cricket::kSimSsrcGroupSemantics && + group.ssrcs.size() > kMaxSimulcastStreams)) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, + "The media section with MID='" + content.mid() + + "' has a ssrc-group with semantics " + + group.semantics + + " and an unexpected number of SSRCs."); + } + } + } + } + return RTCError::OK(); +} + bool IsValidOfferToReceiveMedia(int value) { typedef PeerConnectionInterface::RTCOfferAnswerOptions Options; return (value >= Options::kUndefined) && @@ -3547,6 +3574,13 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription( return error; } + // TODO(crbug.com/1459124): remove killswitch after rollout. + error = ValidateSsrcGroups(*sdesc->description()); + if (!error.ok() && + !pc_->trials().IsDisabled("WebRTC-PreventSsrcGroupsWithUnexpectedSize")) { + return error; + } + if (!pc_->ValidateBundleSettings(sdesc->description(), bundle_groups_by_mid)) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 5cbf79bd50..c03c08f3b8 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -646,7 +646,10 @@ TEST_F(SdpOfferAnswerTest, ExpectAllSsrcsSpecifiedInSsrcGroupFid) { "a=ssrc-group:FID 1 2\r\n" "a=ssrc:1 cname:test\r\n"; auto offer = CreateSessionDescription(SdpType::kOffer, sdp); - EXPECT_FALSE(pc->SetRemoteDescription(std::move(offer))); + RTCError error; + pc->SetRemoteDescription(std::move(offer), &error); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); } TEST_F(SdpOfferAnswerTest, ExpectAllSsrcsSpecifiedInSsrcGroupFecFr) { @@ -676,7 +679,116 @@ TEST_F(SdpOfferAnswerTest, ExpectAllSsrcsSpecifiedInSsrcGroupFecFr) { "a=ssrc-group:FEC-FR 1 2\r\n" "a=ssrc:1 cname:test\r\n"; auto offer = CreateSessionDescription(SdpType::kOffer, sdp); - EXPECT_FALSE(pc->SetRemoteDescription(std::move(offer))); + RTCError error; + pc->SetRemoteDescription(std::move(offer), &error); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); +} + +TEST_F(SdpOfferAnswerTest, ExpectTwoSsrcsInSsrcGroupFid) { + auto pc = CreatePeerConnection(); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96 97\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + "a=rtpmap:96 H264/90000\r\n" + "a=fmtp:96 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\r\n" + "a=rtpmap:97 rtx/90000\r\n" + "a=fmtp:97 apt=96\r\n" + "a=ssrc-group:FID 1 2 3\r\n" + "a=ssrc:1 cname:test\r\n" + "a=ssrc:2 cname:test\r\n" + "a=ssrc:3 cname:test\r\n"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + RTCError error; + pc->SetRemoteDescription(std::move(offer), &error); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); +} + +TEST_F(SdpOfferAnswerTest, ExpectTwoSsrcsInSsrcGroupFecFr) { + auto pc = CreatePeerConnection(); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96 98\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + "a=rtpmap:96 H264/90000\r\n" + "a=fmtp:96 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\r\n" + "a=rtpmap:98 flexfec-03/90000\r\n" + "a=fmtp:98 repair-window=10000000\r\n" + "a=ssrc-group:FEC-FR 1 2 3\r\n" + "a=ssrc:1 cname:test\r\n" + "a=ssrc:2 cname:test\r\n" + "a=ssrc:3 cname:test\r\n"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + RTCError error; + pc->SetRemoteDescription(std::move(offer), &error); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); +} + +TEST_F(SdpOfferAnswerTest, ExpectAtMostFourSsrcsInSsrcGroupSIM) { + auto pc = CreatePeerConnection(); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96 97\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + "a=rtpmap:96 H264/90000\r\n" + "a=fmtp:96 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\r\n" + "a=rtpmap:97 rtx/90000\r\n" + "a=fmtp:97 apt=96\r\n" + "a=ssrc-group:SIM 1 2 3 4\r\n" + "a=ssrc:1 cname:test\r\n" + "a=ssrc:2 cname:test\r\n" + "a=ssrc:3 cname:test\r\n" + "a=ssrc:4 cname:test\r\n"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + RTCError error; + pc->SetRemoteDescription(std::move(offer), &error); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); } TEST_F(SdpOfferAnswerTest, DuplicateSsrcsDisallowedInLocalDescription) {