From 666c33362568ee00c95ee428c541d4ec7e306d4b Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Tue, 18 Oct 2022 12:32:40 +0000 Subject: [PATCH] Stop revalidating STUN packets with the wrong password Investigation showed that a function is revalidating STUN packets against the wrong password. This CL also allows absl/strings/escape.h as #include. Bug: chromium:1177125 Change-Id: Ie068d4c076a5462f2922a012f5e1de23aa6c0b06 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/279560 Commit-Queue: Harald Alvestrand Reviewed-by: Jonas Oreland Cr-Commit-Position: refs/heads/main@{#38438} --- DEPS | 1 + g3doc/abseil-in-webrtc.md | 1 + p2p/base/connection.cc | 17 ++++++++--------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/DEPS b/DEPS index 0ae460afe2..dca70324e9 100644 --- a/DEPS +++ b/DEPS @@ -2678,6 +2678,7 @@ include_rules = [ "+absl/meta/type_traits.h", "+absl/numeric/bits.h", "+absl/strings/ascii.h", + "+absl/strings/escaping.h", "+absl/strings/match.h", "+absl/strings/str_replace.h", "+absl/strings/string_view.h", diff --git a/g3doc/abseil-in-webrtc.md b/g3doc/abseil-in-webrtc.md index 8561975340..80572a3245 100644 --- a/g3doc/abseil-in-webrtc.md +++ b/g3doc/abseil-in-webrtc.md @@ -34,6 +34,7 @@ will generate a shared library. * `absl::string_view` * The functions in `absl/strings/ascii.h`, `absl/strings/match.h`, and `absl/strings/str_replace.h`. +* The functions in `absl/strings/escaping.h`. * `absl::is_trivially_copy_constructible`, `absl::is_trivially_copy_assignable`, and `absl::is_trivially_destructible` from `absl/meta/type_traits.h`. diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 44dc982c84..28761b5921 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -18,6 +18,7 @@ #include #include "absl/algorithm/container.h" +#include "absl/strings/escaping.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" #include "p2p/base/port_allocator.h" @@ -474,22 +475,20 @@ void Connection::OnReadPacket(const char* data, rtc::LoggingSeverity sev = (!writable() ? rtc::LS_INFO : rtc::LS_VERBOSE); switch (msg->integrity()) { case StunMessage::IntegrityStatus::kNotSet: - // Late computation of integrity status, but not an error. + // This packet did not come through Port processing? + // TODO(bugs.webrtc.org/14578): Clean up this situation. 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()); + // TODO(bugs.webrtc.org/14578): Do a better thing + RTC_LOG(LS_INFO) << "STUN code error - Different passwords, old = " + << absl::CHexEscape(msg->password()) << ", new " + << absl::CHexEscape(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: + // kIntegrityBad and kNoIntegrity. // This shouldn't happen. RTC_DCHECK_NOTREACHED(); break;