STUN: Avoid ICE message revalidation wherever possible.

Also call out the places where it happens explicitly - these are places
that need to be redesigned.

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

api/transport/stun.cc

@@ -239,6 +239,8 @@ const StunUInt16ListAttribute* StunMessage::GetUnknownAttributes() const {
 
 StunMessage::IntegrityStatus StunMessage::ValidateMessageIntegrity(
     const std::string& password) {
+  RTC_DCHECK(integrity_ == IntegrityStatus::kNotSet)
+      << "Usage error: Verification should only be done once";
   password_ = password;
   if (GetByteString(STUN_ATTR_MESSAGE_INTEGRITY)) {
     if (ValidateMessageIntegrityOfType(
@@ -327,6 +329,14 @@ StunMessage::IntegrityStatus StunMessage::ValidateMessageIntegrity(
   return integrity_;
 }
 
+StunMessage::IntegrityStatus StunMessage::RevalidateMessageIntegrity(
+    const std::string& password) {
+  RTC_LOG(LS_INFO) << "Message revalidation, old status was "
+                   << static_cast<int>(integrity_);
+  integrity_ = IntegrityStatus::kNotSet;
+  return ValidateMessageIntegrity(password);
+}
+
 bool StunMessage::ValidateMessageIntegrityForTesting(
     const char* data,
     size_t size,

api/transport/stun.h

@@ -220,6 +220,11 @@ class StunMessage {
   // This uses the buffered raw-format message stored by Read().
   IntegrityStatus ValidateMessageIntegrity(const std::string& password);
 
+  // Revalidates the STUN message with (possibly) a new password.
+  // Indicates that calling logic needs review - probably previous call
+  // was checking with the wrong password.
+  IntegrityStatus RevalidateMessageIntegrity(const std::string& password);
+
   // Returns the current integrity status of the message.
   IntegrityStatus integrity() const { return integrity_; }
 

api/transport/stun_unittest.cc

@@ -1870,7 +1870,7 @@ TEST_F(StunTest, ValidateMessageIntegrityWithParser) {
                 "WebRTC.Stun.Integrity.Request",
                 static_cast<int>(StunMessage::IntegrityStatus::kIntegrityOk)),
             1);
-  EXPECT_EQ(message.ValidateMessageIntegrity("Invalid password"),
+  EXPECT_EQ(message.RevalidateMessageIntegrity("Invalid password"),
             StunMessage::IntegrityStatus::kIntegrityBad);
   EXPECT_EQ(webrtc::metrics::NumEvents(
                 "WebRTC.Stun.Integrity.Request",

p2p/base/connection.cc

@@ -472,7 +472,28 @@ void Connection::OnReadPacket(const char* data,
     // If this is a STUN response, then update the writable bit.
     // Log at LS_INFO if we receive a ping on an unwritable connection.
     rtc::LoggingSeverity sev = (!writable() ? rtc::LS_INFO : rtc::LS_VERBOSE);
-    msg->ValidateMessageIntegrity(remote_candidate().password());
+    switch (msg->integrity()) {
+      case StunMessage::IntegrityStatus::kNotSet:
+        // Late computation of integrity status, but not an error.
+        msg->ValidateMessageIntegrity(remote_candidate().password());
+        break;
+      case StunMessage::IntegrityStatus::kIntegrityOk:
+        if (remote_candidate().password() != msg->password()) {
+          // Password has changed. Recheck message.
+          // TODO(crbug.com/1177125): Redesign logic to check only once.
+          msg->RevalidateMessageIntegrity(remote_candidate().password());
+        }
+        break;
+      case StunMessage::IntegrityStatus::kIntegrityBad:
+        // Possibly we have a new password to try.
+        // TODO(crbug.com/1177125): Redesign logic to check only once.
+        msg->RevalidateMessageIntegrity(remote_candidate().password());
+        break;
+      default:
+        // This shouldn't happen.
+        RTC_DCHECK_NOTREACHED();
+        break;
+    }
     switch (msg->type()) {
       case STUN_BINDING_REQUEST:
         RTC_LOG_V(sev) << ToString() << ": Received "

p2p/base/stun_request.cc

@@ -107,11 +107,30 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
   // integrity-protected or not.
   // For some tests, the message integrity is not set in the request.
   // Complain, and then don't check.
-  bool skip_integrity_checking = false;
-  if (request->msg()->integrity() == StunMessage::IntegrityStatus::kNotSet) {
-    skip_integrity_checking = true;
+  bool skip_integrity_checking =
+      (request->msg()->integrity() == StunMessage::IntegrityStatus::kNotSet);
+  if (skip_integrity_checking) {
+    // This indicates lazy test writing (not adding integrity attribute).
+    // Complain, but only in debug mode (while developing).
+    RTC_DLOG(LS_ERROR)
+        << "CheckResponse called on a passwordless request. Fix test!";
   } else {
-    msg->ValidateMessageIntegrity(request->msg()->password());
+    if (msg->integrity() == StunMessage::IntegrityStatus::kNotSet) {
+      // Checking status for the first time. Normal.
+      msg->ValidateMessageIntegrity(request->msg()->password());
+    } else if (msg->integrity() == StunMessage::IntegrityStatus::kIntegrityOk &&
+               msg->password() == request->msg()->password()) {
+      // Status is already checked, with the same password. This is the case
+      // we would want to see happen.
+    } else if (msg->integrity() ==
+               StunMessage::IntegrityStatus::kIntegrityBad) {
+      // This indicates that the original check had the wrong password.
+      // Bad design, needs revisiting.
+      // TODO(crbug.com/1177125): Fix this.
+      msg->RevalidateMessageIntegrity(request->msg()->password());
+    } else {
+      RTC_CHECK_NOTREACHED();
+    }
   }
 
   bool success = true;