From d1831cb4f8db1f19b343d4d1c48dc774bfeb5f42 Mon Sep 17 00:00:00 2001 From: Dor Hen Date: Thu, 2 Feb 2023 17:59:28 -0800 Subject: [PATCH] Treat non DTLS/SCTP Protocol Based Data Channels as Unsupported Media In current state, the SDP parser in webrtc is not backward compatible with clients that might still be using RTP data channels. Obviously, this isn't there is no such usecase in webrtc since the code is deleted, but in Meta we still use it and would like to be able to negotiate between clients that offer RTP data channels. Instead of erroring the parsing procedure, we can parse it as unsupported media in the client that no longer supports RTP data channels. Replaced the existing test that expects parsing failures with a test that validates that the content was parsed as unsupported media. Bug: webrtc:14872 Change-Id: I4c105cf55e33b8c19b2849e16148b8175053c40c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/291190 Reviewed-by: Harald Alvestrand Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#39252} --- pc/webrtc_sdp.cc | 53 ++++++++++++++++++--------------------- pc/webrtc_sdp_unittest.cc | 50 +++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 46 deletions(-) diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 7a63ae41ea..e91743f186 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -2772,36 +2772,31 @@ bool ParseMediaDescription( message, cricket::MEDIA_TYPE_AUDIO, mline_index, protocol, payload_types, pos, &content_name, &bundle_only, §ion_msid_signaling, &transport, candidates, error); - } else if (media_type == kMediaTypeData) { - if (cricket::IsDtlsSctp(protocol)) { - // The draft-03 format is: - // m=application DTLS/SCTP ... - // use_sctpmap should be false. - // The draft-26 format is: - // m=application UDP/DTLS/SCTP webrtc-datachannel - // use_sctpmap should be false. - auto data_desc = std::make_unique(); - // Default max message size is 64K - // according to draft-ietf-mmusic-sctp-sdp-26 - data_desc->set_max_message_size(kDefaultSctpMaxMessageSize); - int p; - if (rtc::FromString(fields[3], &p)) { - data_desc->set_port(p); - } else if (fields[3] == kDefaultSctpmapProtocol) { - data_desc->set_use_sctpmap(false); - } - if (!ParseContent(message, cricket::MEDIA_TYPE_DATA, mline_index, - protocol, payload_types, pos, &content_name, - &bundle_only, §ion_msid_signaling, - data_desc.get(), &transport, candidates, error)) { - return false; - } - data_desc->set_protocol(protocol); - content = std::move(data_desc); - } else { - return ParseFailed(*mline, "Unsupported protocol for media type", - error); + } else if (media_type == kMediaTypeData && cricket::IsDtlsSctp(protocol)) { + // The draft-03 format is: + // m=application DTLS/SCTP ... + // use_sctpmap should be false. + // The draft-26 format is: + // m=application UDP/DTLS/SCTP webrtc-datachannel + // use_sctpmap should be false. + auto data_desc = std::make_unique(); + // Default max message size is 64K + // according to draft-ietf-mmusic-sctp-sdp-26 + data_desc->set_max_message_size(kDefaultSctpMaxMessageSize); + int p; + if (rtc::FromString(fields[3], &p)) { + data_desc->set_port(p); + } else if (fields[3] == kDefaultSctpmapProtocol) { + data_desc->set_use_sctpmap(false); } + if (!ParseContent(message, cricket::MEDIA_TYPE_DATA, mline_index, + protocol, payload_types, pos, &content_name, + &bundle_only, §ion_msid_signaling, data_desc.get(), + &transport, candidates, error)) { + return false; + } + data_desc->set_protocol(protocol); + content = std::move(data_desc); } else { RTC_LOG(LS_WARNING) << "Unsupported media type: " << *mline; auto unsupported_desc = diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index 3e5e9a0d08..368d311009 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -2943,21 +2943,6 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithRtpmapAttribute) { EXPECT_TRUE(SdpDeserialize(sdp_with_data, &jdesc_output)); } -TEST_F(WebRtcSdpTest, DeserializeSdpWithStrangeApplicationProtocolNames) { - static const char* bad_strings[] = { - "DTLS/SCTPRTP/", "obviously-bogus", "UDP/TL/RTSP/SAVPF", - "UDP/TL/RTSP/S", "DTLS/SCTP/RTP/FOO", "obviously-bogus/RTP/"}; - for (auto proto : bad_strings) { - std::string sdp_with_data = kSdpString; - sdp_with_data.append("m=application 9 "); - sdp_with_data.append(proto); - sdp_with_data.append(" 47\r\n"); - JsepSessionDescription jdesc_output(kDummyType); - EXPECT_FALSE(SdpDeserialize(sdp_with_data, &jdesc_output)) - << "Parsing should have failed on " << proto; - } -} - // For crbug/344475. TEST_F(WebRtcSdpTest, DeserializeSdpWithCorruptedSctpDataChannels) { std::string sdp_with_data = kSdpString; @@ -4866,6 +4851,39 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutCname) { EXPECT_TRUE(CompareSessionDescription(jdesc_, new_jdesc)); } +TEST_F(WebRtcSdpTest, + DeserializeSdpWithUnrecognizedApplicationProtocolRejectsSection) { + const char* unsupported_application_protocols[] = { + "bogus/RTP/", "RTP/SAVPF", "DTLS/SCTP/RTP/", "DTLS/SCTPRTP/", + "obviously-bogus", "UDP/TL/RTSP/SAVPF", "UDP/TL/RTSP/S"}; + + for (auto proto : unsupported_application_protocols) { + JsepSessionDescription jdesc_output(kDummyType); + std::string sdp = kSdpSessionString; + sdp.append("m=application 9 "); + sdp.append(proto); + sdp.append(" 101\r\n"); + + EXPECT_TRUE(SdpDeserialize(sdp, &jdesc_output)); + + // Make sure we actually parsed a single media section + ASSERT_EQ(1u, jdesc_output.description()->contents().size()); + + // Content is not getting parsed as sctp but instead unsupported. + EXPECT_EQ(nullptr, jdesc_output.description() + ->contents()[0] + .media_description() + ->as_sctp()); + EXPECT_NE(nullptr, jdesc_output.description() + ->contents()[0] + .media_description() + ->as_unsupported()); + + // Reject the content + EXPECT_TRUE(jdesc_output.description()->contents()[0].rejected); + } +} + TEST_F(WebRtcSdpTest, DeserializeSdpWithUnsupportedMediaType) { std::string sdp = kSdpSessionString; sdp += @@ -4910,8 +4928,6 @@ TEST_F(WebRtcSdpTest, MediaTypeProtocolMismatch) { "m=video"); ExpectParseFailure(std::string(sdp + "m=video 9 SOMETHING 120\r\n"), "m=video"); - ExpectParseFailure(std::string(sdp + "m=application 9 SOMETHING 120\r\n"), - "m=application"); } // Regression test for: