From 02bd649092f62e8416f02eca484578c20e88ba7d Mon Sep 17 00:00:00 2001
From: Joe Downing <[email protected]>
Date: Fri, 14 Aug 2026 10:28:51 -0700
Subject: [PATCH] Fix potential UAF in NegotiatingHostAuthenticator::CreateAuthenticator

During authentication setup, synchronous execution of resume_callback in NegotiatingHostAuthenticator::CreateAuthenticator can result in session closure and destruction of the NegotiatingHostAuthenticator instance. Upon returning from the callback, accessing current_authenticator_ causes a Use-After-Free.

This CL captures a WeakPtr before invoking the callback and returns early if this was destroyed.

TAG=agy
CONV=ec6a5028-dd53-4091-8c6b-7a88a373cc3a

Bug: 522819252
Change-Id: I9aa68827d17dfbf6a0a9ae972c9bb8e3acbe0b11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8261393
Commit-Queue: Joe Downing <[email protected]>
Reviewed-by: Jamie Walch <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1679658}
---

diff --git a/remoting/protocol/negotiating_authenticator_unittest.cc b/remoting/protocol/negotiating_authenticator_unittest.cc
index 47e2313..f7f62c2 100644
--- a/remoting/protocol/negotiating_authenticator_unittest.cc
+++ b/remoting/protocol/negotiating_authenticator_unittest.cc
@@ -9,9 +9,12 @@
 #include "base/functional/bind.h"
 #include "base/memory/raw_ptr.h"
 #include "base/run_loop.h"
+#include "base/test/bind.h"
 #include "base/test/mock_callback.h"
 #include "net/base/net_errors.h"
+#include "remoting/base/mock_session_authz_service_client.h"
 #include "remoting/base/rsa_key_pair.h"
+#include "remoting/base/session_authz_service_client_factory.h"
 #include "remoting/protocol/auth_util.h"
 #include "remoting/protocol/authenticator.h"
 #include "remoting/protocol/authenticator_test_base.h"
@@ -104,6 +107,28 @@
   raw_ptr<Authenticator> authenticator_;
 };
 
+class FakeSessionAuthzServiceClientFactory
+    : public SessionAuthzServiceClientFactory {
+ public:
+  FakeSessionAuthzServiceClientFactory(
+      AuthenticationMethod method,
+      std::unique_ptr<SessionAuthzServiceClient> client)
+      : method_(method), client_(std::move(client)) {}
+
+  std::unique_ptr<SessionAuthzServiceClient> Create() override {
+    return std::move(client_);
+  }
+
+  AuthenticationMethod method() override { return method_; }
+
+ protected:
+  ~FakeSessionAuthzServiceClientFactory() override = default;
+
+ private:
+  AuthenticationMethod method_;
+  std::unique_ptr<SessionAuthzServiceClient> client_;
+};
+
 }  // namespace
 
 class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
@@ -467,4 +492,74 @@
   EXPECT_EQ(client, nullptr);
 }
 
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_SharedSecret_SynchronousTeardown) {
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddSharedSecretAuth("hash");
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method = AuthenticationMethod::SHARED_SECRET_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_Pairing_SynchronousTeardown) {
+  auto pairing_registry = base::MakeRefCounted<SynchronousPairingRegistry>(
+      std::make_unique<MockPairingRegistryDelegate>());
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddPairingAuth(pairing_registry);
+  auth_config->AddSharedSecretAuth("hash");
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method = AuthenticationMethod::PAIRED_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_SessionAuthz_SynchronousTeardown) {
+  auto mock_client = std::make_unique<MockSessionAuthzServiceClient>();
+  EXPECT_CALL(*mock_client, GenerateHostToken(_))
+      .WillOnce([](MockSessionAuthzServiceClient::GenerateHostTokenCallback
+                       callback) {
+        std::move(callback).Run(
+            HttpStatus(HttpStatus::Code::PERMISSION_DENIED, "denied"), nullptr);
+      });
+
+  auto factory = base::MakeRefCounted<FakeSessionAuthzServiceClientFactory>(
+      AuthenticationMethod::CLOUD_SESSION_AUTHZ_SPAKE2_CURVE25519,
+      std::move(mock_client));
+
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddSessionAuthzAuth(factory);
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method =
+      AuthenticationMethod::CLOUD_SESSION_AUTHZ_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
 }  // namespace remoting::protocol
diff --git a/remoting/protocol/negotiating_host_authenticator.cc b/remoting/protocol/negotiating_host_authenticator.cc
index 69dbe5a..0d461033 100644
--- a/remoting/protocol/negotiating_host_authenticator.cc
+++ b/remoting/protocol/negotiating_host_authenticator.cc
@@ -134,6 +134,8 @@
     base::OnceClosure resume_callback) {
   DCHECK(current_method_ != AuthenticationMethod::INVALID);
 
+  auto weak_this = weak_factory_.GetWeakPtr();
+
   switch (current_method_) {
     case AuthenticationMethod::INVALID:
       NOTREACHED();
@@ -147,8 +149,9 @@
           base::BindRepeating(&Spake2Authenticator::CreateForHost, local_id_,
                               remote_id_, config_->local_cert,
                               config_->key_pair));
-      authenticator->Start(std::move(resume_callback));
+      SessionAuthzAuthenticator* auth_ptr = authenticator.get();
       current_authenticator_ = std::move(authenticator);
+      auth_ptr->Start(std::move(resume_callback));
       break;
     }
 
@@ -161,22 +164,23 @@
           base::BindRepeating(&Spake2Authenticator::CreateForHost, local_id_,
                               remote_id_, config_->local_cert,
                               config_->key_pair));
-      authenticator->Start(std::move(resume_callback));
+      SessionAuthzAuthenticator* auth_ptr = authenticator.get();
       current_authenticator_ = std::move(authenticator);
+      auth_ptr->Start(std::move(resume_callback));
       break;
     }
 
     case AuthenticationMethod::PAIRED_SPAKE2_CURVE25519: {
-      PairingHostAuthenticator* pairing_authenticator =
-          new PairingHostAuthenticator(
-              config_->pairing_registry,
-              base::BindRepeating(&Spake2Authenticator::CreateForHost,
-                                  local_id_, remote_id_, config_->local_cert,
-                                  config_->key_pair),
-              config_->shared_secret_hash);
-      current_authenticator_.reset(pairing_authenticator);
-      pairing_authenticator->Initialize(client_id_, preferred_initial_state,
-                                        std::move(resume_callback));
+      auto pairing_authenticator = std::make_unique<PairingHostAuthenticator>(
+          config_->pairing_registry,
+          base::BindRepeating(&Spake2Authenticator::CreateForHost, local_id_,
+                              remote_id_, config_->local_cert,
+                              config_->key_pair),
+          config_->shared_secret_hash);
+      PairingHostAuthenticator* auth_ptr = pairing_authenticator.get();
+      current_authenticator_ = std::move(pairing_authenticator);
+      auth_ptr->Initialize(client_id_, preferred_initial_state,
+                           std::move(resume_callback));
       break;
     }
 

diff --git a/remoting/protocol/negotiating_authenticator_unittest.cc b/remoting/protocol/negotiating_authenticator_unittest.cc
index 47e2313..f7f62c2 100644
--- a/remoting/protocol/negotiating_authenticator_unittest.cc
+++ b/remoting/protocol/negotiating_authenticator_unittest.cc
@@ -9,9 +9,12 @@
 #include "base/functional/bind.h"
 #include "base/memory/raw_ptr.h"
 #include "base/run_loop.h"
+#include "base/test/bind.h"
 #include "base/test/mock_callback.h"
 #include "net/base/net_errors.h"
+#include "remoting/base/mock_session_authz_service_client.h"
 #include "remoting/base/rsa_key_pair.h"
+#include "remoting/base/session_authz_service_client_factory.h"
 #include "remoting/protocol/auth_util.h"
 #include "remoting/protocol/authenticator.h"
 #include "remoting/protocol/authenticator_test_base.h"
@@ -104,6 +107,28 @@
   raw_ptr<Authenticator> authenticator_;
 };
 
+class FakeSessionAuthzServiceClientFactory
+    : public SessionAuthzServiceClientFactory {
+ public:
+  FakeSessionAuthzServiceClientFactory(
+      AuthenticationMethod method,
+      std::unique_ptr<SessionAuthzServiceClient> client)
+      : method_(method), client_(std::move(client)) {}
+
+  std::unique_ptr<SessionAuthzServiceClient> Create() override {
+    return std::move(client_);
+  }
+
+  AuthenticationMethod method() override { return method_; }
+
+ protected:
+  ~FakeSessionAuthzServiceClientFactory() override = default;
+
+ private:
+  AuthenticationMethod method_;
+  std::unique_ptr<SessionAuthzServiceClient> client_;
+};
+
 }  // namespace
 
 class NegotiatingAuthenticatorTest : public AuthenticatorTestBase {
@@ -467,4 +492,74 @@
   EXPECT_EQ(client, nullptr);
 }
 
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_SharedSecret_SynchronousTeardown) {
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddSharedSecretAuth("hash");
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method = AuthenticationMethod::SHARED_SECRET_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_Pairing_SynchronousTeardown) {
+  auto pairing_registry = base::MakeRefCounted<SynchronousPairingRegistry>(
+      std::make_unique<MockPairingRegistryDelegate>());
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddPairingAuth(pairing_registry);
+  auth_config->AddSharedSecretAuth("hash");
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method = AuthenticationMethod::PAIRED_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
+TEST_F(NegotiatingAuthenticatorTest,
+       CreateHostAuthenticator_SessionAuthz_SynchronousTeardown) {
+  auto mock_client = std::make_unique<MockSessionAuthzServiceClient>();
+  EXPECT_CALL(*mock_client, GenerateHostToken(_))
+      .WillOnce([](MockSessionAuthzServiceClient::GenerateHostTokenCallback
+                       callback) {
+        std::move(callback).Run(
+            HttpStatus(HttpStatus::Code::PERMISSION_DENIED, "denied"), nullptr);
+      });
+
+  auto factory = base::MakeRefCounted<FakeSessionAuthzServiceClientFactory>(
+      AuthenticationMethod::CLOUD_SESSION_AUTHZ_SPAKE2_CURVE25519,
+      std::move(mock_client));
+
+  auto auth_config =
+      std::make_unique<HostAuthenticationConfig>(host_cert_, key_pair_);
+  auth_config->AddSessionAuthzAuth(factory);
+  auto host = std::make_unique<NegotiatingHostAuthenticator>(
+      kHostJid, kClientJid, std::move(auth_config));
+
+  JingleAuthentication client_message;
+  client_message.method =
+      AuthenticationMethod::CLOUD_SESSION_AUTHZ_SPAKE2_CURVE25519;
+
+  // This should not crash.
+  host->ProcessMessage(client_message,
+                       base::BindLambdaForTesting([&]() { host.reset(); }));
+
+  EXPECT_EQ(host, nullptr);
+}
+
 }  // namespace remoting::protocol