From bf633673a200d6d22785aa48ed35f5c1cebc538f Mon Sep 17 00:00:00 2001
From: Harald Alvestrand <hta@webrtc.org>
Date: Tue, 21 Apr 2026 10:21:04 +0000
Subject: [PATCH] Fix Data Channel encryption downgrade via BUNDLE desync in kPrAnswer.

Update BundleManager::Update to establish BUNDLE groups when receiving
a prAnswer. This fixes a desync where VerifyCrypto (which uses the
BUNDLE group from the SDP) assumed sections were bundled and skipped
fingerprint checks, while JsepTransportController (which uses
BundleManager) did not bundle them, leading to separate unencrypted
transports if a fingerprint was missing in the prAnswer.

Includes a regression test that MUNGEs a prAnswer to trigger this
condition and verifies that no unencrypted packets are received.

Bug: webrtc:504579798
Change-Id: I8b1b3a20f6c82f6be5c79d636be230fbbc83ca08
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/465400
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Auto-Submit: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#47493}
---

diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc
index 2f10c6a..6d8dbf0 100644
--- a/pc/jsep_transport_collection.cc
+++ b/pc/jsep_transport_collection.cc
@@ -37,12 +37,11 @@
   // Rollbacks should call Rollback, not Update.
   RTC_DCHECK(type != SdpType::kRollback);
   bool bundle_groups_changed = false;
-  // TODO(bugs.webrtc.org/3349): Do this for kPrAnswer as well. To make this
-  // work, we also need to make sure PRANSWERs don't call
-  // MaybeDestroyJsepTransport, because the final answer may need the destroyed
-  // transport if it changes the BUNDLE group.
+  // TODO: bugs.webrtc.org/42228228 - Evaluate whether a PR-Answer can establish
+  // a bundle and a final Answer can remove it again. If this happens, PRAnswer
+  // should not destroy the unused transport.
   if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle ||
-      type == SdpType::kAnswer) {
+      type == SdpType::kAnswer || type == SdpType::kPrAnswer) {
     // If our policy is "max-bundle" or this is an answer, update all bundle
     // groups.
     bundle_groups_changed = true;
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index b108ba9..3a5803a 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -30,6 +30,7 @@
 #include "absl/strings/string_view.h"
 #include "api/candidate.h"
 #include "api/crypto/crypto_options.h"
+#include "api/data_channel_interface.h"
 #include "api/dtmf_sender_interface.h"
 #include "api/jsep.h"
 #include "api/make_ref_counted.h"
@@ -61,14 +62,17 @@
 #include "media/base/codec.h"
 #include "media/base/media_constants.h"
 #include "media/base/stream_params.h"
+#include "p2p/base/p2p_constants.h"
 #include "p2p/base/port.h"
 #include "p2p/base/port_allocator.h"
 #include "p2p/base/transport_description.h"
 #include "p2p/base/transport_info.h"
+#include "p2p/dtls/dtls_transport_internal.h"
 #include "p2p/test/test_turn_server.h"
 #include "pc/media_session.h"
 #include "pc/peer_connection.h"
 #include "pc/peer_connection_factory.h"
+#include "pc/sctp_transport.h"
 #include "pc/session_description.h"
 #include "pc/test/fake_periodic_video_source.h"
 #include "pc/test/integration_test_helpers.h"
@@ -86,6 +90,7 @@
 #include "rtc_base/system/plan_b_only.h"
 #include "rtc_base/task_queue_for_test.h"
 #include "rtc_base/test_certificate_verifier.h"
+#include "rtc_base/thread.h"
 #include "rtc_base/virtual_socket_server.h"
 #include "system_wrappers/include/metrics.h"
 #include "test/gmock.h"
@@ -109,6 +114,7 @@
 using ::testing::Not;
 using ::testing::NotNull;
 using ::testing::Return;
+using ::testing::SizeIs;
 using ::testing::WithParamInterface;
 
 class PeerConnectionIntegrationTest : public PeerConnectionIntegrationBaseTest,
@@ -5125,6 +5131,230 @@
   }
 }
 
+#ifdef WEBRTC_HAVE_SCTP
+
+class DowngradeLogSink : public LogSink {
+ public:
+  void OnLogMessage(const std::string& message) override {
+    if (message.find("Received unexpected non-DTLS packet") !=
+        std::string::npos) {
+      found_ = true;
+    }
+  }
+  bool found() const { return found_; }
+
+ private:
+  bool found_ = false;
+};
+
+// This test reproduces the vulnerability where a kPrAnswer with a BUNDLE group
+// but no fingerprint for a media section can cause that media section to use
+// an unencrypted transport.
+TEST_P(PeerConnectionIntegrationTest,
+       DataChannelEncryptionDowngradeViaBundleDesyncInPrAnswer) {
+  RTCConfiguration config;
+  config.bundle_policy = PeerConnectionInterface::kBundlePolicyBalanced;
+  ASSERT_TRUE(CreatePeerConnectionWrappersWithConfig(config, config));
+  ConnectFakeSignaling();
+
+  // 1. Initial negotiation without BUNDLE.
+  // We munge the answer to remove BUNDLE so that separate transports are
+  // created.
+  callee()->SetGeneratedSdpMunger(
+      [](std::unique_ptr<SessionDescriptionInterface>& desc) {
+        desc->description()->RemoveGroupByName(GROUP_TYPE_BUNDLE);
+      });
+
+  caller()->AddVideoTrack();
+  caller()->CreateDataChannel();
+  caller()->CreateAndSetAndSignalOffer();
+  ASSERT_TRUE(WaitUntil([&] { return SignalingStateStable(); }));
+  ASSERT_TRUE(WaitUntil([&] { return DtlsConnected(); }));
+
+  // 2. Renegotiation with BUNDLE in offer, and malicious prAnswer.
+  // The malicious prAnswer will have a BUNDLE group containing mid 0 and mid 1,
+  // but mid 1 (Data Channel) will have no fingerprint.
+  callee()->SetGeneratedSdpMunger(nullptr);
+
+  // Create an offer.
+  std::unique_ptr<SessionDescriptionInterface> offer =
+      caller()->CreateOfferAndWait();
+  ASSERT_THAT(offer, NotNull());
+
+  // Set local offer on caller.
+  auto observer = make_ref_counted<FakeSetLocalDescriptionObserver>();
+  caller()->pc()->SetLocalDescription(std::move(offer), observer);
+  ASSERT_TRUE(WaitUntil([&] { return observer->called(); }));
+
+  // Set remote offer on callee.
+  std::unique_ptr<SessionDescriptionInterface> offer_for_callee =
+      caller()->CreateOfferAndWait();
+  ASSERT_THAT(offer_for_callee, NotNull());
+  ASSERT_TRUE(callee()->SetRemoteDescription(std::move(offer_for_callee)));
+
+  // Callee creates a normal answer first.
+  auto answer_observer =
+      make_ref_counted<MockCreateSessionDescriptionObserver>();
+  callee()->pc()->CreateAnswer(
+      answer_observer.get(), PeerConnectionInterface::RTCOfferAnswerOptions());
+  ASSERT_TRUE(WaitUntil([&] { return answer_observer->called(); }));
+  std::unique_ptr<SessionDescriptionInterface> answer =
+      answer_observer->MoveDescription();
+  ASSERT_THAT(answer, NotNull());
+
+  // Convert the answer to a prAnswer by serializing and re-parsing.
+  std::string sdp = answer->ToString();
+  ASSERT_FALSE(sdp.empty());
+  std::unique_ptr<SessionDescriptionInterface> pr_answer =
+      CreateSessionDescription(SdpType::kPrAnswer, sdp);
+  ASSERT_THAT(pr_answer, NotNull());
+
+  // Munge the prAnswer:
+  // - Ensure BUNDLE group is present.
+  // - Remove fingerprint from the second media section (mid 1).
+  SessionDescription* desc = pr_answer->description();
+  const ContentGroup* bundle_group = desc->GetGroupByName(GROUP_TYPE_BUNDLE);
+  ASSERT_THAT(bundle_group, NotNull());
+  ASSERT_THAT(bundle_group->content_names(), SizeIs(2));
+
+  // Remove fingerprint from the second content.
+  const ContentInfos& contents = desc->contents();
+  ASSERT_GE(contents.size(), 2u);
+  TransportInfo* transport_info =
+      desc->GetTransportInfoByName(contents[1].mid());
+  ASSERT_THAT(transport_info, NotNull());
+  transport_info->description.identity_fingerprint.reset();
+
+  // Set the remote prAnswer on the caller.
+  // This succeeds because VerifyCrypto skips the fingerprint check for
+  // bundled sections (except the first).
+  ASSERT_TRUE(caller()->SetRemoteDescription(std::move(pr_answer)));
+
+  // With the fix, the Data Channel's transport will now be BUNDLED into mid 0.
+  // Since mid 0 has DTLS active, mid 1's data will also be DTLS-wrapped.
+  // We can verify this by checking that the SCTP transport's internal DTLS
+  // transport is active.
+  EXPECT_TRUE(network_thread()->BlockingCall([&] {
+    return static_cast<SctpTransport*>(caller()->pc()->GetSctpTransport().get())