diff --git a/api/transport/stun.cc b/api/transport/stun.cc index 0bd2d24fa5..1098c6720e 100644 --- a/api/transport/stun.cc +++ b/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(integrity_); + integrity_ = IntegrityStatus::kNotSet; + return ValidateMessageIntegrity(password); +} + bool StunMessage::ValidateMessageIntegrityForTesting( const char* data, size_t size, diff --git a/api/transport/stun.h b/api/transport/stun.h index 72a77990b4..c2c9ad4b9c 100644 --- a/api/transport/stun.h +++ b/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_; } diff --git a/api/transport/stun_unittest.cc b/api/transport/stun_unittest.cc index 218520bda4..96ad45843b 100644 --- a/api/transport/stun_unittest.cc +++ b/api/transport/stun_unittest.cc @@ -1870,7 +1870,7 @@ TEST_F(StunTest, ValidateMessageIntegrityWithParser) { "WebRTC.Stun.Integrity.Request", static_cast(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", diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index e59de3de9d..44dc982c84 100644 --- a/p2p/base/connection.cc +++ b/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 " diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 1228e4e148..d15a3e65e2 100644 --- a/p2p/base/stun_request.cc +++ b/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;