[webauthn] Do not skip UV after iCloud recovery
When a user recovers their security domain secret through MagicArch,
they had to enter some form of user verification, so we don't prompt
them for a second UV to satisfy the user verification requirement.
This logic was erroneously applying to iCloud keychain recovery keys.
Fixed: 367771121
Change-Id: I4dc03f8568285cdc2ca14dbc65df0f4724cdac10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5873366
Reviewed-by: Adam Langley <agl@chromium.org>
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1358217}
diff --git a/chrome/browser/webauthn/enclave_authenticator_browsertest.cc b/chrome/browser/webauthn/enclave_authenticator_browsertest.cc
index cbe7d6fd..bf8e4d7 100644
--- a/chrome/browser/webauthn/enclave_authenticator_browsertest.cc
+++ b/chrome/browser/webauthn/enclave_authenticator_browsertest.cc
@@ -2952,11 +2952,19 @@
// factor that should have been added.
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult
registration_state_result;
+ const auto pin_member = std::ranges::find_if(
+ security_domain_service_->members(),
+ [](const trusted_vault_pb::SecurityDomainMember& member) {
+ return member.member_type() ==
+ trusted_vault_pb::SecurityDomainMember::
+ MEMBER_TYPE_GOOGLE_PASSWORD_MANAGER_PIN;
+ });
+ const auto& pin_metadata =
+ pin_member->member_metadata().google_password_manager_pin_metadata();
registration_state_result.gpm_pin_metadata = trusted_vault::GpmPinMetadata(
- "public key",
- EnclaveManager::MakeWrappedPINForTesting(kSecurityDomainSecret,
- "123456"),
- /*expiry=*/base::Time::Now() + base::Seconds(10000));
+ pin_member->public_key(), pin_metadata.encrypted_pin_hash(),
+ base::Time::FromSecondsSinceUnixEpoch(
+ pin_metadata.expiration_time().seconds()));
registration_state_result.state =
trusted_vault::DownloadAuthenticationFactorsRegistrationStateResult::
State::kRecoverable;
@@ -2990,7 +2998,7 @@
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
content::DOMMessageQueue message_queue(web_contents);
- content::ExecuteScriptAsync(web_contents, kMakeCredentialUvDiscouraged);
+ content::ExecuteScriptAsync(web_contents, kMakeCredentialUvRequired);
delegate_observer()->WaitForUI();
EXPECT_EQ(
@@ -3000,11 +3008,17 @@
->enclave_controller_for_testing()
->account_state_for_testing(),
GPMEnclaveController::AccountState::kRecoverable);
+
+ // User verification must not be skipped when recovering from an iCloud key.
+ model_observer()->SetStepToObserve(
+ AuthenticatorRequestDialogModel::Step::kGPMEnterPin);
dialog_model()->OnTrustThisComputer();
+ model_observer()->WaitForStep();
+ dialog_model()->OnGPMPinEntered(u"123456");
std::string script_result;
ASSERT_TRUE(message_queue.WaitForMessage(&script_result));
- EXPECT_EQ(script_result, "\"webauthn: OK\"");
+ EXPECT_EQ(script_result, "\"webauthn: uv=true\"");
delegate_observer()->WaitForDelegateDestruction();
diff --git a/chrome/browser/webauthn/gpm_enclave_controller.cc b/chrome/browser/webauthn/gpm_enclave_controller.cc
index f4736f9..61b4870 100644
--- a/chrome/browser/webauthn/gpm_enclave_controller.cc
+++ b/chrome/browser/webauthn/gpm_enclave_controller.cc
@@ -313,7 +313,7 @@
// Pick an enclave user verification method for a specific request.
EnclaveUserVerificationMethod PickEnclaveUserVerificationMethod(
device::UserVerificationRequirement uv,
- bool have_added_device,
+ bool have_entered_pin_for_recovery,
bool has_pin,
EnclaveManager::UvKeyState uv_key_state,
bool platform_has_biometrics,
@@ -324,7 +324,7 @@
constexpr bool kIsMac = false;
#endif
- if (have_added_device) {
+ if (have_entered_pin_for_recovery) {
return EnclaveUserVerificationMethod::kImplicit;
}
@@ -560,7 +560,8 @@
if (request_type_ == device::FidoRequestType::kGetAssertion) {
if (enclave_manager_->is_ready()) {
switch (PickEnclaveUserVerificationMethod(
- user_verification_requirement_, /*have_added_device=*/false,
+ user_verification_requirement_,
+ /*have_entered_pin_for_recovery=*/false,
enclave_manager_->has_wrapped_pin(),
enclave_manager_->uv_key_state(/*platform_has_biometrics=*/false),
/*platform_has_biometrics=*/false, BrowserIsApp())) {
@@ -781,7 +782,6 @@
// iCloud keychain recovery.
device::enclave::RecordEvent(
device::enclave::Event::kICloudRecoverySuccessful);
- recovered_with_icloud_keychain_ = false;
} else if (model_->step() == Step::kRecoverSecurityDomain) {
// MagicArch recovery.
webauthn::user_actions::RecordRecoverySucceeded();
@@ -941,16 +941,34 @@
SetFailedPINAttemptCount(0);
uv_method_ = PickEnclaveUserVerificationMethod(
- user_verification_requirement_, have_added_device_,
+ user_verification_requirement_,
+ have_added_device_ && !recovered_with_icloud_keychain_,
enclave_manager_->has_wrapped_pin(),
enclave_manager_->uv_key_state(*model_->platform_has_biometrics),
*model_->platform_has_biometrics, BrowserIsApp());
- // `have_added_device_` is set, which satisfies UV, so we must have picked
- // "implicit" UV.
- CHECK_EQ(*uv_method_, EnclaveUserVerificationMethod::kImplicit);
+ switch (*uv_method_) {
+ case EnclaveUserVerificationMethod::kUVKeyWithSystemUI:
+ case EnclaveUserVerificationMethod::kDeferredUVKeyWithSystemUI:
+ case EnclaveUserVerificationMethod::kNone:
+ case EnclaveUserVerificationMethod::kImplicit:
+ model_->DisableUiOrShowLoadingDialog();
+ StartTransaction();
+ break;
- model_->DisableUiOrShowLoadingDialog();
- StartTransaction();
+ case EnclaveUserVerificationMethod::kUVKeyWithChromeUI:
+ model_->SetStep(Step::kGPMTouchID);
+ break;
+
+ case EnclaveUserVerificationMethod::kUnsatisfiable:
+ // TODO(crbug.com/367985619): it's possible to get to this state if a user
+ // recovers from iCloud keychain and does not have a usable PIN.
+ model_->SetStep(Step::kGPMError);
+ break;
+
+ case EnclaveUserVerificationMethod::kPIN:
+ PromptForPin();
+ break;
+ }
}
void GPMEnclaveController::SetAccountStateReady() {
@@ -972,7 +990,8 @@
case AccountState::kReady:
uv_method_ = PickEnclaveUserVerificationMethod(
- user_verification_requirement_, have_added_device_,
+ user_verification_requirement_,
+ have_added_device_ && !recovered_with_icloud_keychain_,
enclave_manager_->has_wrapped_pin(),
enclave_manager_->uv_key_state(*model_->platform_has_biometrics),
*model_->platform_has_biometrics, BrowserIsApp());
@@ -1025,7 +1044,8 @@
switch (account_state_) {
case AccountState::kReady:
uv_method_ = PickEnclaveUserVerificationMethod(
- user_verification_requirement_, have_added_device_,
+ user_verification_requirement_,
+ have_added_device_ && !recovered_with_icloud_keychain_,
enclave_manager_->has_wrapped_pin(),
enclave_manager_->uv_key_state(*model_->platform_has_biometrics),
*model_->platform_has_biometrics, BrowserIsApp());
