commit 02ce5887b67387d670cd905e40c400ffd1e749d4
author Jonas Oreland <jonaso@webrtc.org> Wed Nov 15 09:53:40 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Nov 15 11:31:45 2023
tree 63458e54556a0436d9109a68ee925afc6374006f
parent 4ac371883e83c264eea96beeb14f95f9b69d72a4

Allow list the places which send STUN_REQUEST w/o password

Bug: chromium:1177125
Change-Id: Ia58a596871c8f15b9638d026a336a30c15f89f90
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/327441
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41165}

p2p/base/stun_port.cc

diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc
index 373b21a..44776d7 100644
--- a/p2p/base/stun_port.cc
+++ b/p2p/base/stun_port.cc
@@ -46,7 +46,9 @@
                     std::make_unique<StunMessage>(STUN_BINDING_REQUEST)),
         port_(port),
         server_addr_(addr),
-        start_time_(start_time) {}
+        start_time_(start_time) {
+    SetAuthenticationRequired(false);
+  }
 
   const rtc::SocketAddress& server_addr() const { return server_addr_; }
 

p2p/base/stun_request.cc

diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc
index 15c1e6a..5bf7dfe 100644
--- a/p2p/base/stun_request.cc
+++ b/p2p/base/stun_request.cc
@@ -57,6 +57,10 @@
 void StunRequestManager::SendDelayed(StunRequest* request, int delay) {
   RTC_DCHECK_RUN_ON(thread_);
   RTC_DCHECK_EQ(this, request->manager());
+  RTC_DCHECK(!request->AuthenticationRequired() ||
+             request->msg()->integrity() !=
+                 StunMessage::IntegrityStatus::kNotSet)
+      << "Sending request w/o integrity!";
   auto [iter, was_inserted] =
       requests_.emplace(request->id(), absl::WrapUnique(request));
   RTC_DCHECK(was_inserted);
@@ -104,15 +108,23 @@
   StunRequest* request = iter->second.get();
 
   // Now that we know the request, we can see if the response is
-  // integrity-protected or not.
-  // For some tests, the message integrity is not set in the request.
-  // Complain, and then don't check.
+  // integrity-protected or not. Some requests explicitly disables
+  // integrity checks using SetAuthenticationRequired.
+  // TODO(chromium:1177125): Remove below!
+  // And we suspect that for some tests, the message integrity is not set in the
+  // request. Complain, and then don't check.
   bool skip_integrity_checking =
       (request->msg()->integrity() == StunMessage::IntegrityStatus::kNotSet);
-  if (skip_integrity_checking) {
+  if (!request->AuthenticationRequired()) {
+    // This is a STUN_BINDING to from stun_port.cc or
+    // the initial (unauthenticated) TURN_ALLOCATE_REQUEST.
+  } else if (skip_integrity_checking) {
+    // TODO(chromium:1177125): Remove below!
     // This indicates lazy test writing (not adding integrity attribute).
     // Complain, but only in debug mode (while developing).
-    RTC_DLOG(LS_ERROR)
+    RTC_LOG(LS_ERROR)
+        << "CheckResponse called on a passwordless request. Fix test!";
+    RTC_DCHECK(false)
         << "CheckResponse called on a passwordless request. Fix test!";
   } else {
     if (msg->integrity() == StunMessage::IntegrityStatus::kNotSet) {

p2p/base/stun_request.h

diff --git a/p2p/base/stun_request.h b/p2p/base/stun_request.h
index 6e83be38..f2fd7e5 100644
--- a/p2p/base/stun_request.h
+++ b/p2p/base/stun_request.h
@@ -115,6 +115,12 @@
   // Time elapsed since last send (in ms)
   int Elapsed() const;
 
+  // Add method to explitly allow requests w/o password.
+  // - STUN_BINDINGs from StunPort to a stun server
+  // - The initial TURN_ALLOCATE_REQUEST
+  void SetAuthenticationRequired(bool val) { authentication_required_ = val; }
+  bool AuthenticationRequired() const { return authentication_required_; }
+
  protected:
   friend class StunRequestManager;
 
@@ -155,6 +161,7 @@
   bool timeout_ RTC_GUARDED_BY(network_thread());
   webrtc::ScopedTaskSafety task_safety_{
       webrtc::PendingTaskSafetyFlag::CreateDetachedInactive()};
+  bool authentication_required_ = true;
 };
 
 }  // namespace cricket

p2p/base/stun_request_unittest.cc

diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc
index 2f88ab1..8ce89f8 100644
--- a/p2p/base/stun_request_unittest.cc
+++ b/p2p/base/stun_request_unittest.cc
@@ -79,7 +79,9 @@
  public:
   StunRequestThunker(StunRequestManager& manager, StunRequestTest* test)
       : StunRequest(manager, CreateStunMessage(STUN_BINDING_REQUEST)),
-        test_(test) {}
+        test_(test) {
+    SetAuthenticationRequired(false);
+  }
 
   std::unique_ptr<StunMessage> CreateResponseMessage(StunMessageType type) {
     return CreateStunMessage(type, msg());

p2p/base/turn_port.cc

diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc
index 6cd6c45..042727f 100644
--- a/p2p/base/turn_port.cc
+++ b/p2p/base/turn_port.cc
@@ -1314,6 +1314,8 @@
   message->AddAttribute(std::move(transport_attr));
   if (!port_->hash().empty()) {
     port_->AddRequestAuthInfo(message);
+  } else {
+    SetAuthenticationRequired(false);
   }
   port_->MaybeAddTurnLoggingId(message);
   port_->TurnCustomizerMaybeModifyOutgoingStunMessage(message);