diff --git a/remoting/protocol/negotiating_authenticator_unittest.cc b/remoting/protocol/negotiating_authenticator_unittest.cc index eaf1b6e05fa56..8a5201f0a6b83 100644 --- a/remoting/protocol/negotiating_authenticator_unittest.cc +++ b/remoting/protocol/negotiating_authenticator_unittest.cc @@ -383,6 +383,77 @@ TEST_F(NegotiatingAuthenticatorTest, ASSERT_NE(&host_->implementing_authenticator(), host_.get()); } +// Regression test: when fetch_secret_callback runs synchronously (as it does +// in the Boca Spotlight RemotingClient at remoting/client/common/ +// remoting_client.cc) and the resulting Spake2Authenticator synchronously +// REJECTs the host's session-accept (e.g. missing ), the resume +// callback chain may re-entrantly destroy the NegotiatingClientAuthenticator +// (via JingleSession::Close -> SetState(FAILED) -> WebrtcConnectionToHost +// -> RemotingClient::StopSession -> connection_.reset()) while +// CreateAuthenticatorForCurrentMethod() is still on the stack. The trailing +// `ChainStateChangeAfterAcceptedWithUnderlying(*current_authenticator_)` call +// then dereferences `*this` (and the freed Spake2Authenticator) after free. +// +// This test models exactly that production sequence: a synchronous +// fetch_secret_callback (matching Boca), a hostile session-accept with +// method=SHARED_SECRET_SPAKE2_CURVE25519 and no certificate, and a resume +// callback that — like JingleSession::ProcessAuthenticationStep on REJECTED — +// synchronously tears down the owning object graph. +TEST_F(NegotiatingAuthenticatorTest, + CreateAuthenticatorForCurrentMethod_SynchronousRejectionTeardown) { + // 1. Construct the client authenticator exactly as RemotingClient does, with + // a *synchronous* fetch_secret_callback (see remoting_client.cc:161-166). + protocol::ClientAuthenticationConfig client_auth_config; + client_auth_config.host_id = kTestHostId; + client_auth_config.fetch_secret_callback = base::BindRepeating( + [](const std::string& secret, bool pairing_supported, + const protocol::SecretFetchedCallback& secret_fetched_callback) { + secret_fetched_callback.Run(secret); + }, + std::string(kTestPin)); + auto client = std::make_unique( + kClientJid, kHostJid, client_auth_config); + + // 2. Client emits session-initiate (state -> WAITING_MESSAGE). + ASSERT_EQ(client->state(), Authenticator::MESSAGE_READY); + std::ignore = client->GetNextMessage(); + ASSERT_EQ(client->state(), Authenticator::WAITING_MESSAGE); + + // 3. Malicious host replies with session-accept choosing SHARED_SECRET and + // *omitting the certificate*. Spake2Authenticator::ProcessMessageInternal + // rejects this synchronously ("No valid host certificate.") before + // setting started_, so JingleSession would Close(INVALID_STATE) without + // transitioning to AUTHENTICATING. + JingleAuthentication hostile_accept; + hostile_accept.method = AuthenticationMethod::SHARED_SECRET_SPAKE2_CURVE25519; + // certificate left empty; spake_message left empty. + + // 4. Resume callback models JingleSession::ProcessAuthenticationStep -> + // Close -> SetState(FAILED) -> ... -> connection_.reset(), which frees + // JingleSession -> NegotiatingClientAuthenticator -> Spake2Authenticator + // while CreateAuthenticatorForCurrentMethod() is still on the stack. + bool destroyed = false; + auto resume_callback = base::BindOnce( + [](std::unique_ptr* owner, + bool* destroyed) { + if ((*owner)->state() == Authenticator::REJECTED) { + owner->reset(); + *destroyed = true; + } + }, + &client, &destroyed); + + // 5. Deliver the hostile message. Under ASAN this trips heap-use-after-free + // inside ChainStateChangeAfterAcceptedWithUnderlying() at + // negotiating_client_authenticator.cc:148. + client->ProcessMessage(hostile_accept, std::move(resume_callback)); + + // If the UAF were fixed (e.g. WeakPtr guard before line 148), control + // reaches here with the authenticator already destroyed. + EXPECT_TRUE(destroyed); + EXPECT_EQ(client, nullptr); +} + TEST_F(NegotiatingAuthenticatorTest, GetNextMessage_SynchronousTeardown) { auto auth_config = std::make_unique(host_cert_, key_pair_);