From 6c27d56a2aeb2cff10a216d714552f4970d99d32 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 16 Dec 2022 12:45:57 +0100 Subject: [PATCH] sdp: measure rtp header extension collisions since extension ids are required to be unique in a BUNDLE group: https://www.rfc-editor.org/rfc/rfc8843#name-rtp-header-extensions-consi Measure how much enforcing this would break in UMA first. BUG=webrtc:14782 Change-Id: Ieaf7a436feea677032499e11ca14973eebda322e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/288362 Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Reviewed-by: Johannes Kron Cr-Commit-Position: refs/heads/main@{#38914} --- pc/sdp_offer_answer.cc | 54 +++++++++++++- pc/sdp_offer_answer_unittest.cc | 124 ++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 84a075630e..d0bd3c88af 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -491,6 +491,52 @@ RTCError ValidateBundledPayloadTypes( return RTCError::OK(); } +RTCError FindDuplicateHeaderExtensionIds( + const RtpExtension extension, + std::map& id_to_extension) { + auto existing_extension = id_to_extension.find(extension.id); + if (existing_extension != id_to_extension.end() && + extension != existing_extension->second) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "A BUNDLE group contains a codec collision for " + "header extension id='" + + rtc::ToString(extension.id) + + ". The id must be the same across all bundled media descriptions"); + } + id_to_extension.insert(std::make_pair(extension.id, extension)); + return RTCError::OK(); +} + +RTCError ValidateBundledRtpHeaderExtensions( + const cricket::SessionDescription& description) { + // https://www.rfc-editor.org/rfc/rfc8843#name-rtp-header-extensions-consi + // ... the identifier used for a given extension MUST identify the same + // extension across all the bundled media descriptions. + std::vector bundle_groups = + description.GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + for (const cricket::ContentGroup* bundle_group : bundle_groups) { + std::map id_to_extension; + for (const std::string& content_name : bundle_group->content_names()) { + const cricket::MediaContentDescription* media_description = + description.GetContentDescriptionByName(content_name); + if (!media_description) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "A BUNDLE group contains a MID='" + content_name + + "' matching no m= section."); + } + for (const auto& extension : media_description->rtp_header_extensions()) { + auto error = + FindDuplicateHeaderExtensionIds(extension, id_to_extension); + if (!error.ok()) { + return error; + } + } + } + } + return RTCError::OK(); +} + bool IsValidOfferToReceiveMedia(int value) { typedef PeerConnectionInterface::RTCOfferAnswerOptions Options; return (value >= Options::kUndefined) && @@ -3371,12 +3417,18 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription( return RTCError(RTCErrorType::INVALID_PARAMETER, kSdpWithoutIceUfragPwd); } - // Validate bundle, payload types and that there are no collisions. + // Validate that there are no collisions of bundled payload types. error = ValidateBundledPayloadTypes(*sdesc->description()); // TODO(bugs.webrtc.org/14420): actually reject. RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledPayloadTypes", error.ok()); + // Validate that there are no collisions of bundled header extensions ids. + error = ValidateBundledRtpHeaderExtensions(*sdesc->description()); + // TODO(bugs.webrtc.org/14782): actually reject. + RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledExtensionIds", + error.ok()); + if (!pc_->ValidateBundleSettings(sdesc->description(), bundle_groups_by_mid)) { return RTCError(RTCErrorType::INVALID_PARAMETER, kBundleWithoutRtcpMux); diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index ecac7f8de8..bdd3301060 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -238,6 +238,130 @@ TEST_F(SdpOfferAnswerTest, BundleCodecCollisionInDifferentBundlesAllowed) { "WebRTC.PeerConnection.ValidBundledPayloadTypes", false)); } +TEST_F(SdpOfferAnswerTest, BundleMeasuresHeaderExtensionIdCollision) { + 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 1\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=audio 9 UDP/TLS/RTP/SAVPF 111\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:111 opus/48000/2\r\n" + "a=extmap:3 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:1\r\n" + "a=rtpmap:112 VP8/90000\r\n" + "a=extmap:3 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n"; + auto desc = CreateSessionDescription(SdpType::kOffer, sdp); + ASSERT_NE(desc, nullptr); + RTCError error; + pc->SetRemoteDescription(std::move(desc), &error); + EXPECT_TRUE(error.ok()); + EXPECT_METRIC_EQ(1, + webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.ValidBundledExtensionIds", true)); +} + +// extmap:3 is used with two different URIs which is not allowed. +TEST_F(SdpOfferAnswerTest, BundleRejectsHeaderExtensionIdCollision) { + 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 1\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=audio 9 UDP/TLS/RTP/SAVPF 111\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:111 opus/48000/2\r\n" + "a=extmap:3 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:1\r\n" + "a=rtpmap:112 VP8/90000\r\n" + "a=extmap:3 urn:3gpp:video-orientation\r\n"; + auto desc = CreateSessionDescription(SdpType::kOffer, sdp); + ASSERT_NE(desc, nullptr); + RTCError error; + pc->SetRemoteDescription(std::move(desc), &error); + EXPECT_TRUE(error.ok()); + EXPECT_METRIC_EQ( + 1, webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.ValidBundledExtensionIds", false)); +} + +// transport-wide cc is negotiated with two different ids 3 and 4. +// This is not a good idea but tolerable. +TEST_F(SdpOfferAnswerTest, BundleAcceptsDifferentIdsForSameExtension) { + 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 1\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=audio 9 UDP/TLS/RTP/SAVPF 111\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:111 opus/48000/2\r\n" + "a=extmap:3 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 112\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:1\r\n" + "a=rtpmap:112 VP8/90000\r\n" + "a=extmap:4 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n"; + auto desc = CreateSessionDescription(SdpType::kOffer, sdp); + ASSERT_NE(desc, nullptr); + RTCError error; + pc->SetRemoteDescription(std::move(desc), &error); + EXPECT_TRUE(error.ok()); + EXPECT_METRIC_EQ(1, + webrtc::metrics::NumEvents( + "WebRTC.PeerConnection.ValidBundledExtensionIds", true)); +} + TEST_F(SdpOfferAnswerTest, LargeMidsAreRejected) { auto pc = CreatePeerConnection(); std::string sdp =