webauthn: don't report null `authenticator` when making a credential.
There were two cases where we returned a nullptr for the authenticator
when making a credential. One of them prompted the linked bug report:
removing an authenticator when the PIN prompt was pending would trigger
a crash. The other was if biometric enrollment failed.
There aren't any cases in the assertion request handlers.
This change fixes those two cases and resolves the crash. The added test
reproed the PIN-prompt crash prior to the fix.
Fixed: 370000838
Change-Id: If406f4ab6ed58744d1c3efc8915cf63414b258dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5902556
Auto-Submit: Adam Langley <agl@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/main@{#1363581}
diff --git a/content/browser/webauth/authenticator_common_impl.cc b/content/browser/webauth/authenticator_common_impl.cc
index a05c5f3..7bb5557 100644
--- a/content/browser/webauth/authenticator_common_impl.cc
+++ b/content/browser/webauth/authenticator_common_impl.cc
@@ -2006,6 +2006,8 @@
device::MakeCredentialStatus status_code,
std::optional<device::AuthenticatorMakeCredentialResponse> response_data,
const device::FidoAuthenticator* authenticator) {
+ CHECK(authenticator);
+
if (!req_state_->request_handler) {
// Either the callback was called immediately and
// |req_state_->request_handler| has not yet been assigned (this is a bug),
@@ -2227,7 +2229,9 @@
std::optional<std::vector<device::AuthenticatorGetAssertionResponse>>
response_data,
device::FidoAuthenticator* authenticator) {
+ CHECK(authenticator);
DCHECK(!response_data || !response_data->empty()); // empty vector is invalid
+
if (!req_state_->request_handler) {
// Either the callback was called immediately and
// |req_state_->request_handler| has not yet been assigned (this is a bug),
diff --git a/content/browser/webauth/authenticator_impl_unittest.cc b/content/browser/webauth/authenticator_impl_unittest.cc
index 2f493aa9..6ad6a6f 100644
--- a/content/browser/webauth/authenticator_impl_unittest.cc
+++ b/content/browser/webauth/authenticator_impl_unittest.cc
@@ -96,6 +96,7 @@
#include "device/fido/features.h"
#include "device/fido/fido_authenticator.h"
#include "device/fido/fido_constants.h"
+#include "device/fido/fido_device_authenticator.h"
#include "device/fido/fido_discovery_base.h"
#include "device/fido/fido_parsing_utils.h"
#include "device/fido/fido_request_handler_base.h"
@@ -5369,10 +5370,12 @@
PINTestAuthenticatorRequestDelegate(
bool supports_pin,
const std::list<PINExpectation>& pins,
- std::optional<InterestingFailureReason>* failure_reason)
+ std::optional<InterestingFailureReason>* failure_reason,
+ base::RepeatingCallback<bool()> collect_pin_cb)
: supports_pin_(supports_pin),
expected_(pins),
- failure_reason_(failure_reason) {}
+ failure_reason_(failure_reason),
+ collect_pin_cb_(collect_pin_cb) {}
PINTestAuthenticatorRequestDelegate(
const PINTestAuthenticatorRequestDelegate&) = delete;
@@ -5389,6 +5392,9 @@
void CollectPIN(
CollectPINOptions options,
base::OnceCallback<void(std::u16string)> provide_pin_cb) override {
+ if (collect_pin_cb_ && !collect_pin_cb_.Run()) {
+ return;
+ }
DCHECK(supports_pin_);
DCHECK(!expected_.empty()) << "unexpected PIN request";
if (expected_.front().reason == PINReason::kChallenge) {
@@ -5418,6 +5424,9 @@
const bool supports_pin_;
std::list<PINExpectation> expected_;
const raw_ptr<std::optional<InterestingFailureReason>> failure_reason_;
+ // collect_pin_cb_ is optional. If present, it returns whether `CollectPIN`
+ // should continue and invoke its main callback.
+ base::RepeatingCallback<bool()> collect_pin_cb_;
};
class PINTestAuthenticatorContentBrowserClient : public ContentBrowserClient {
@@ -5431,7 +5440,7 @@
GetWebAuthenticationRequestDelegate(
RenderFrameHost* render_frame_host) override {
return std::make_unique<PINTestAuthenticatorRequestDelegate>(
- supports_pin, expected, &failure_reason);
+ supports_pin, expected, &failure_reason, collect_pin_cb);
}
TestWebAuthenticationDelegate web_authentication_delegate;
@@ -5439,6 +5448,7 @@
bool supports_pin = true;
std::list<PINExpectation> expected;
std::optional<InterestingFailureReason> failure_reason;
+ base::RepeatingCallback<bool()> collect_pin_cb;
};
class PINAuthenticatorImplTest : public UVAuthenticatorImplTest {
@@ -6383,6 +6393,61 @@
EXPECT_EQ(AuthenticatorMakeCredential().status, AuthenticatorStatus::SUCCESS);
}
+TEST_F(PINAuthenticatorImplTest,
+ RemoveAuthenticatorDuringRegistrationPINPrompt) {
+ // Regression test for crbug.com/370000838: removing an authenticator while
+ // the PIN prompt was showing would crash.
+ base::RepeatingCallback<void(bool)> disconnect_1;
+ device::test::MultipleVirtualFidoDeviceFactory::DeviceDetails device_1;
+ device_1.state->pin = kTestPIN;
+ device_1.config.pin_support = true;
+ std::tie(disconnect_1, device_1.disconnect_events) =
+ device::FidoDiscoveryBase::EventStream<bool>::New();
+
+ auto discovery =
+ std::make_unique<device::test::MultipleVirtualFidoDeviceFactory>();
+ discovery->AddDevice(std::move(device_1));
+ ReplaceDiscoveryFactory(std::move(discovery));
+
+ test_client_.collect_pin_cb =
+ base::BindLambdaForTesting([&disconnect_1]() -> bool {
+ disconnect_1.Run(false);
+ return false;
+ });
+
+ EXPECT_EQ(AuthenticatorMakeCredential().status,
+ AuthenticatorStatus::NOT_ALLOWED_ERROR);
+}
+
+TEST_F(PINAuthenticatorImplTest, RemoveAuthenticatorDuringAssertionPINPrompt) {
+ ASSERT_TRUE(virtual_device_factory_->mutable_state()->InjectRegistration(
+ get_credential_options()->allow_credentials[0].id, kTestRelyingPartyId));
+
+ base::RepeatingCallback<void(bool)> disconnect_1;
+ device::test::MultipleVirtualFidoDeviceFactory::DeviceDetails device_1;
+ device_1.state->pin = kTestPIN;
+ device_1.config.pin_support = true;
+ std::tie(disconnect_1, device_1.disconnect_events) =
+ device::FidoDiscoveryBase::EventStream<bool>::New();
+
+ auto discovery =
+ std::make_unique<device::test::MultipleVirtualFidoDeviceFactory>();
+ discovery->AddDevice(std::move(device_1));
+ ReplaceDiscoveryFactory(std::move(discovery));
+
+ test_client_.collect_pin_cb =
+ base::BindLambdaForTesting([&disconnect_1]() -> bool {
+ disconnect_1.Run(false);
+ return false;
+ });
+
+ PublicKeyCredentialRequestOptionsPtr options =
+ GetTestPublicKeyCredentialRequestOptions();
+ options->user_verification = device::UserVerificationRequirement::kRequired;
+ EXPECT_EQ(AuthenticatorGetAssertion(std::move(options)).status,
+ AuthenticatorStatus::NOT_ALLOWED_ERROR);
+}
+
TEST_F(PINAuthenticatorImplTest, AppIdExcludeExtensionWithPinRequiredError) {
// Some alwaysUv authenticators apply the alwaysUv logic even when up=false.
// That causes them to return `kCtap2ErrPinRequired` to appIdExclude probes
diff --git a/device/fido/make_credential_request_handler.cc b/device/fido/make_credential_request_handler.cc
index 7e727ea..c53b405 100644
--- a/device/fido/make_credential_request_handler.cc
+++ b/device/fido/make_credential_request_handler.cc
@@ -557,7 +557,7 @@
state_ = State::kFinished;
std::move(completion_callback_)
.Run(MakeCredentialStatus::kAuthenticatorRemovedDuringPINEntry,
- std::nullopt, nullptr);
+ std::nullopt, authenticator);
}
}
}
@@ -882,7 +882,7 @@
state_ = State::kFinished;
std::move(completion_callback_)
.Run(MakeCredentialStatus::kAuthenticatorResponseInvalid, std::nullopt,
- nullptr);
+ bio_enroller_->authenticator());
}
void MakeCredentialRequestHandler::OnEnrollmentDismissed() {
