Disallow SDP munging if it modifies the ICE ufrag.
Gated behind default-disabled field trial WebRTC-NoSdpMangleUfrag
Bug: b/409713509
Change-Id: I48e307ef3ca65b463dde8bcb24ae5252cd6fdf58
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/385721
Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#44360}
diff --git a/experiments/field_trials.py b/experiments/field_trials.py
index 3ca7029..1c9f8c0 100755
--- a/experiments/field_trials.py
+++ b/experiments/field_trials.py
@@ -116,6 +116,9 @@
FieldTrial('WebRTC-MixedCodecSimulcast',
362277533,
date(2025, 9, 1)),
+ FieldTrial('WebRTC-NoSdpMangleUfrag',
+ 375571816,
+ date(2025, 10, 11)),
FieldTrial('WebRTC-Pacer-FastRetransmissions',
40235589,
date(2024, 4, 1)),
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index e2f6bf9..c5254e0 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -458,6 +458,15 @@
PayloadTypePicker& payload_type_picker() override {
return payload_type_picker_;
}
+ void DisableSdpMungingChecksForTesting() {
+ if (!signaling_thread()->IsCurrent()) {
+ signaling_thread()->BlockingCall(
+ [&]() { DisableSdpMungingChecksForTesting(); });
+ return;
+ }
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sdp_handler_->DisableSdpMungingChecksForTesting();
+ }
protected:
// Available for rtc::scoped_refptr creation
diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc
index 4ba1709..20ee339 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -654,6 +654,9 @@
auto set_local_description_with_ufrag_pwd_length = [this](int ufrag_len,
int pwd_len) {
auto pc = CreatePeerConnectionWithAudioVideo();
+ // Because local munging is forbidden by spec, we have to disable the
+ // check for it.
+ pc->GetInternalPeerConnection()->DisableSdpMungingChecksForTesting();
auto offer = pc->CreateOffer();
SetIceUfragPwd(offer.get(), std::string(ufrag_len, 'x'),
std::string(pwd_len, 'x'));
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 95b396d5..59604bc 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -2370,6 +2370,33 @@
UpdateEndedRemoteMediaStreams();
}
+void SdpOfferAnswerHandler::ReportInitialSdpMunging(bool had_local_description,
+ SdpType type) {
+ // Report SDP munging of the initial call to setLocalDescription separately.
+ if (!had_local_description) {
+ switch (type) {
+ case SdpType::kOffer:
+ RTC_HISTOGRAM_ENUMERATION(
+ "WebRTC.PeerConnection.SdpMunging.Offer.Initial",
+ last_sdp_munging_type_, SdpMungingType::kMaxValue);
+ break;
+ case SdpType::kAnswer:
+ RTC_HISTOGRAM_ENUMERATION(
+ "WebRTC.PeerConnection.SdpMunging.Answer.Initial",
+ last_sdp_munging_type_, SdpMungingType::kMaxValue);
+ break;
+ case SdpType::kPrAnswer:
+ RTC_HISTOGRAM_ENUMERATION(
+ "WebRTC.PeerConnection.SdpMunging.PrAnswer.Initial",
+ last_sdp_munging_type_, SdpMungingType::kMaxValue);
+ break;
+ case SdpType::kRollback:
+ // Rollback does not have SDP so can not be munged.
+ break;
+ }
+ }
+}
+
void SdpOfferAnswerHandler::DoSetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
@@ -2429,6 +2456,19 @@
? last_created_offer_.get()
: last_created_answer_.get());
+ if (!disable_sdp_munging_checks_ &&
+ pc_->trials().IsEnabled("WebRTC-NoSdpMangleUfrag")) {
+ if (sdp_munging_type == kIceUfrag || sdp_munging_type == kIcePwd) {
+ RTC_LOG(LS_ERROR) << "Rejecting SDP because of ufrag modification";
+ observer->OnSetLocalDescriptionComplete(
+ RTCError(RTCErrorType::INVALID_PARAMETER,
+ "SDP is modified in a non-acceptable way"));
+ last_sdp_munging_type_ = sdp_munging_type;
+ ReportInitialSdpMunging(had_local_description, desc->GetType());
+ return;
+ }
+ }
+
// Grab the description type before moving ownership to
// ApplyLocalDescription, which may destroy it before returning.
const SdpType type = desc->GetType();
@@ -2463,29 +2503,10 @@
last_created_offer_.reset(nullptr);
last_created_answer_.reset(nullptr);
last_sdp_munging_type_ = sdp_munging_type;
+
// Report SDP munging of the initial call to setLocalDescription separately.
- if (!had_local_description) {
- switch (local_description()->GetType()) {
- case SdpType::kOffer:
- RTC_HISTOGRAM_ENUMERATION(
- "WebRTC.PeerConnection.SdpMunging.Offer.Initial",
- last_sdp_munging_type_, SdpMungingType::kMaxValue);
- break;
- case SdpType::kAnswer:
- RTC_HISTOGRAM_ENUMERATION(
- "WebRTC.PeerConnection.SdpMunging.Answer.Initial",
- last_sdp_munging_type_, SdpMungingType::kMaxValue);
- break;
- case SdpType::kPrAnswer:
- RTC_HISTOGRAM_ENUMERATION(
- "WebRTC.PeerConnection.SdpMunging.PrAnswer.Initial",
- last_sdp_munging_type_, SdpMungingType::kMaxValue);
- break;
- case SdpType::kRollback:
- // Rollback does not have SDP so can not be munged.
- break;
- }
- }
+ ReportInitialSdpMunging(had_local_description,
+ local_description()->GetType());
observer->OnSetLocalDescriptionComplete(RTCError::OK());
pc_->NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h
index f3eff28..453e4bc 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -39,6 +39,7 @@
#include "api/uma_metrics.h"
#include "api/video/video_bitrate_allocator_factory.h"
#include "media/base/media_channel.h"
+#include "media/base/media_engine.h"
#include "media/base/stream_params.h"
#include "p2p/base/port_allocator.h"
#include "pc/codec_vendor.h"
@@ -183,6 +184,9 @@
}
SdpMungingType sdp_munging_type() const { return last_sdp_munging_type_; }
+ void DisableSdpMungingChecksForTesting() {
+ disable_sdp_munging_checks_ = true;
+ }
private:
class RemoteDescriptionOperation;
@@ -559,6 +563,8 @@
AddIceCandidateResult AddIceCandidateInternal(
const IceCandidateInterface* candidate);
+ void ReportInitialSdpMunging(bool had_local_description, SdpType type);
+
// ==================================================================
// Access to pc_ variables
MediaEngineInterface* media_engine() const;
@@ -682,6 +688,11 @@
// determines the SSL role.
std::optional<bool> initial_offerer_ RTC_GUARDED_BY(signaling_thread());
+ // Whether SDP munging checks are enabled or not.
+ // Some tests will be detected as SDP munging, so offer the option
+ // to disable.
+ bool disable_sdp_munging_checks_ = false;
+
WeakPtrFactory<SdpOfferAnswerHandler> weak_ptr_factory_
RTC_GUARDED_BY(signaling_thread());
};
diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
index a765355..8038556 100644
--- a/pc/sdp_offer_answer_unittest.cc
+++ b/pc/sdp_offer_answer_unittest.cc
@@ -1867,7 +1867,44 @@
}
TEST_F(SdpOfferAnswerMungingTest, IceUfrag) {
+ auto pc = CreatePeerConnection(
+ FieldTrials::CreateNoGlobal("WebRTC-NoSdpMangleUfrag/Enabled/"));
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& transport_infos = offer->description()->transport_infos();
+ ASSERT_EQ(transport_infos.size(), 1u);
+ transport_infos[0].description.ice_ufrag =
+ "amungediceufragthisshouldberejected";
+ RTCError error;
+ // Ufrag is rejected.
+ EXPECT_FALSE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kIceUfrag, 1)));
+}
+
+TEST_F(SdpOfferAnswerMungingTest, IceUfragCheckDisabledByFieldTrial) {
+ auto pc = CreatePeerConnection(
+ FieldTrials::CreateNoGlobal("WebRTC-NoSdpMangleUfrag/Disabled/"));
+ pc->AddAudioTrack("audio_track", {});
+
+ auto offer = pc->CreateOffer();
+ auto& transport_infos = offer->description()->transport_infos();
+ ASSERT_EQ(transport_infos.size(), 1u);
+ transport_infos[0].description.ice_ufrag =
+ "amungediceufragthisshouldberejected";
+ RTCError error;
+ // Ufrag is not rejected.
+ EXPECT_TRUE(pc->SetLocalDescription(std::move(offer), &error));
+ EXPECT_THAT(
+ metrics::Samples("WebRTC.PeerConnection.SdpMunging.Offer.Initial"),
+ ElementsAre(Pair(SdpMungingType::kIceUfrag, 1)));
+}
+
+TEST_F(SdpOfferAnswerMungingTest, IceUfragWithCheckDisabledForTesting) {
auto pc = CreatePeerConnection();
+ pc->GetInternalPeerConnection()->DisableSdpMungingChecksForTesting();
pc->AddAudioTrack("audio_track", {});
auto offer = pc->CreateOffer();
