Skip to content

Commit

Permalink
[WebAuthn] Unregister device when deferred UV key submission fails
Browse files Browse the repository at this point in the history 
This identifies when an attempt to submit a UV key after registration
fails, and causes the device to unregister itself in that situation.

Bug: 40274370
Change-Id: Ie1d4ff03d652ff345ad8ba22fad49e80a7fbf7ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5570160
Reviewed-by: Adam Langley <[email protected]>
Commit-Queue: Ken Buchanan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1307126}
  • Loading branch information
@kenrb
kenrb authored and Chromium LUCI CQ committed May 28, 2024
1 parent 3184b5f commit 0e7f401
Show file tree
Hide file tree
Showing 11 changed files with 280 additions and 104 deletions.
15 changes: 15 additions & 0 deletions chrome/browser/webauthn/enclave_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ struct EnclaveManager::PendingAction {

namespace {

// Used so the EnclaveManager can be forced into invalid states for testing.
static bool g_invariant_override_ = false;

#if BUILDFLAG(IS_MAC)
constexpr char kUserVerifyingKeyKeychainAccessGroup[] =
MAC_TEAM_IDENTIFIER_STRING "." MAC_BUNDLE_IDENTIFIER_STRING ".webauthn-uvk";
Expand Down Expand Up @@ -253,6 +256,9 @@ std::optional<int> CheckPINInvariants(
// CheckInvariants checks all the invariants of `user`, returning either a
// line-number for the failing check, or else `nullopt` to indicate success.
std::optional<int> CheckInvariants(const EnclaveLocalState::User& user) {
if (g_invariant_override_) {
return std::nullopt;
}
if (user.wrapped_hardware_private_key().empty() !=
user.hardware_public_key().empty()) {
return __LINE__;
Expand Down Expand Up @@ -3192,6 +3198,11 @@ void EnclaveManager::ClearRegistrationForTesting() {
ClearRegistration();
}

// static
void EnclaveManager::EnableInvariantChecksForTesting(bool enabled) {
g_invariant_override_ = !enabled;
}

// static
std::string EnclaveManager::MakeWrappedPINForTesting(
base::span<const uint8_t> security_domain_secret,
Expand Down Expand Up @@ -3555,3 +3566,7 @@ void EnclaveManager::SetSecret(int32_t key_version,
secret_version_ = key_version;
secret_ = std::vector<uint8_t>(secret.begin(), secret.end());
}

base::WeakPtr<EnclaveManager> EnclaveManager::GetWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}
5 changes: 5 additions & 0 deletions chrome/browser/webauthn/enclave_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,17 @@ class EnclaveManager : public EnclaveManagerInterface {
// Clears the registration as if we were starting from scratch.
void ClearRegistrationForTesting();

// Toggle invariant checks.
static void EnableInvariantChecksForTesting(bool enable);

// Create a wrapped PIN, suitable for putting into a simulated security domain
// member.
static std::string MakeWrappedPINForTesting(
base::span<const uint8_t> security_domain_secret,
std::string_view pin);

base::WeakPtr<EnclaveManager> GetWeakPtr();

private:
class StateMachine;
class IdentityObserver;
Expand Down
122 changes: 103 additions & 19 deletions chrome/browser/webauthn/enclave_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "base/functional/callback.h"
#include "base/json/json_reader.h"
#include "base/process/process.h"
#include "base/run_loop.h"
#include "base/strings/string_number_conversions.h"
#include "base/test/bind.h"
#include "base/test/task_environment.h"
Expand Down Expand Up @@ -339,18 +340,30 @@ class EnclaveManagerTest : public testing::Test, EnclaveManager::Observer {
}
}

void DoAssertion(std::unique_ptr<sync_pb::WebauthnCredentialSpecifics> entity,
std::unique_ptr<enclave::ClaimedPIN> claimed_pin) {
auto ui_request = std::make_unique<enclave::CredentialRequest>();
ui_request->signing_callback = manager_.HardwareKeySigningCallback();
ui_request->wrapped_secret =
*manager_.GetWrappedSecret(/*version=*/kSecretVersion);
ui_request->entity = std::move(entity);
ui_request->claimed_pin = std::move(claimed_pin);
ui_request->save_passkey_callback =
base::BindOnce([](sync_pb::WebauthnCredentialSpecifics) {
NOTREACHED_IN_MIGRATION();
});
struct GetAssertionResponseExpectation {
device::CtapDeviceResponseCode result =
device::CtapDeviceResponseCode::kSuccess;
uint32_t size = 1;
};

void DoAssertion(
std::unique_ptr<sync_pb::WebauthnCredentialSpecifics> entity,
std::unique_ptr<enclave::ClaimedPIN> claimed_pin,
GetAssertionResponseExpectation expected_response,
std::unique_ptr<enclave::CredentialRequest> custom_ui_request = nullptr) {
std::unique_ptr<enclave::CredentialRequest> ui_request;
if (custom_ui_request) {
ui_request = std::move(custom_ui_request);
} else {
ui_request = std::make_unique<enclave::CredentialRequest>();
ui_request->signing_callback = manager_.HardwareKeySigningCallback();
ui_request->wrapped_secret =
*manager_.GetWrappedSecret(/*version=*/kSecretVersion);
ui_request->entity = std::move(entity);
ui_request->claimed_pin = std::move(claimed_pin);
ui_request->save_passkey_callback = base::BindOnce(
[](sync_pb::WebauthnCredentialSpecifics) { NOTREACHED_NORETURN(); });
}

enclave::EnclaveAuthenticator authenticator(
std::move(ui_request), /*network_context_factory=*/
Expand Down Expand Up @@ -389,8 +402,8 @@ class EnclaveManagerTest : public testing::Test, EnclaveManager::Observer {

ASSERT_TRUE(status.has_value());
ASSERT_TRUE(true);
ASSERT_EQ(status, device::CtapDeviceResponseCode::kSuccess);
ASSERT_EQ(responses.size(), 1u);
ASSERT_EQ(status, expected_response.result);
ASSERT_EQ(responses.size(), expected_response.size);
}

bool Register() {
Expand Down Expand Up @@ -477,7 +490,8 @@ TEST_F(EnclaveManagerTest, Basic) {
EXPECT_EQ(security_domain_service_->num_pin_members(), 0u);

DoCreate(/*claimed_pin=*/nullptr, /*out_specifics=*/nullptr);
DoAssertion(GetTestEntity(), /*claimed_pin=*/nullptr);
DoAssertion(GetTestEntity(), /*claimed_pin=*/nullptr,
GetAssertionResponseExpectation());
}

TEST_F(EnclaveManagerTest, SecretsArriveBeforeRegistrationRequested) {
Expand Down Expand Up @@ -710,7 +724,8 @@ TEST_F(EnclaveManagerTest, SetupWithPIN) {
EnclaveManager::MakeClaimedPINSlowly(pin, manager_.GetWrappedPIN());
std::unique_ptr<sync_pb::WebauthnCredentialSpecifics> entity;
DoCreate(/*claimed_pin=*/nullptr, &entity);
DoAssertion(std::move(entity), std::move(claimed_pin));
DoAssertion(std::move(entity), std::move(claimed_pin),
GetAssertionResponseExpectation());
}

TEST_F(EnclaveManagerTest, SetupWithPIN_CertXMLFailure) {
Expand Down Expand Up @@ -764,7 +779,8 @@ TEST_F(EnclaveManagerTest, AddDeviceAndPINToAccount) {
EnclaveManager::MakeClaimedPINSlowly(pin, manager_.GetWrappedPIN());
std::unique_ptr<sync_pb::WebauthnCredentialSpecifics> entity;
DoCreate(/*claimed_pin=*/nullptr, &entity);
DoAssertion(std::move(entity), std::move(claimed_pin));
DoAssertion(std::move(entity), std::move(claimed_pin),
GetAssertionResponseExpectation());
}

TEST_F(EnclaveManagerTest, ChangePIN) {
Expand Down Expand Up @@ -805,7 +821,8 @@ TEST_F(EnclaveManagerTest, ChangePIN) {
EnclaveManager::MakeClaimedPINSlowly(new_pin, manager_.GetWrappedPIN());
std::unique_ptr<sync_pb::WebauthnCredentialSpecifics> entity;
DoCreate(/*claimed_pin=*/nullptr, &entity);
DoAssertion(std::move(entity), std::move(claimed_pin));
DoAssertion(std::move(entity), std::move(claimed_pin),
GetAssertionResponseExpectation());
}

TEST_F(EnclaveManagerTest, EnclaveForgetsClient_SetupWithPIN) {
Expand Down Expand Up @@ -1205,8 +1222,13 @@ class EnclaveUVTest : public EnclaveManagerTest {
fake_provider_.emplace<crypto::ScopedNullUserVerifyingKeyProvider>();
}

void UseFailingUVKeySupport() {
fake_provider_.emplace<crypto::ScopedFailingUserVerifyingKeyProvider>();
}

absl::variant<crypto::ScopedFakeUserVerifyingKeyProvider,
crypto::ScopedNullUserVerifyingKeyProvider>
crypto::ScopedNullUserVerifyingKeyProvider,
crypto::ScopedFailingUserVerifyingKeyProvider>
fake_provider_;

#if BUILDFLAG(IS_MAC)
Expand Down Expand Up @@ -1470,6 +1492,68 @@ TEST_F(EnclaveUVTest, DeferredUVKeyCreation) {
EXPECT_FALSE(user_state.deferred_uv_key_creation());
EXPECT_FALSE(user_state.wrapped_uv_private_key().empty());
}

TEST_F(EnclaveUVTest, UnregisterOnFailedDeferredUVKeyCreation) {
security_domain_service_->pretend_there_are_members();
NoArgCallback loaded_callback;
manager_.Load(loaded_callback.callback());
loaded_callback.WaitForCallback();

BoolCallback register_callback;
manager_.RegisterIfNeeded(register_callback.callback());
ASSERT_FALSE(manager_.is_idle());
register_callback.WaitForCallback();

std::vector<uint8_t> key(kTestKey.begin(), kTestKey.end());
ASSERT_FALSE(manager_.has_pending_keys());
manager_.StoreKeys(gaia_id_, {std::move(key)},
/*last_key_version=*/kSecretVersion);
ASSERT_TRUE(manager_.is_idle());
ASSERT_TRUE(manager_.has_pending_keys());

BoolCallback add_callback;
ASSERT_TRUE(manager_.AddDeviceToAccount(
/*pin_metadata=*/std::nullopt, add_callback.callback()));
ASSERT_FALSE(manager_.is_idle());
add_callback.WaitForCallback();

EXPECT_EQ(manager_.uv_key_state(),
EnclaveManager::UvKeyState::kUsesSystemUIDeferredCreation);
const auto& user_state =
manager_.local_state_for_testing().users().find(gaia_id_)->second;
EXPECT_TRUE(user_state.deferred_uv_key_creation());
EXPECT_TRUE(user_state.wrapped_uv_private_key().empty());

UseFailingUVKeySupport();
EnclaveManager::EnableInvariantChecksForTesting(false);

base::RunLoop run_loop;
auto ui_request = std::make_unique<enclave::CredentialRequest>();
ui_request->signing_callback = manager_.HardwareKeySigningCallback();
ui_request->wrapped_secret =
*manager_.GetWrappedSecret(/*version=*/kSecretVersion);
ui_request->entity = GetTestEntity();
ui_request->claimed_pin = nullptr;
ui_request->save_passkey_callback = base::BindOnce(
[](sync_pb::WebauthnCredentialSpecifics) { NOTREACHED_NORETURN(); });
ui_request->user_verified = true;
ui_request->uv_key_creation_callback =
manager_.UserVerifyingKeyCreationCallback();
ui_request->unregister_callback =
base::BindOnce(&EnclaveManager::Unenroll, manager_.GetWeakPtr(),
base::BindLambdaForTesting(
[&run_loop](bool) { run_loop.QuitWhenIdle(); }));

GetAssertionResponseExpectation expected_response;
expected_response.result = device::CtapDeviceResponseCode::kCtap2ErrOther;
expected_response.size = 0;
DoAssertion(GetTestEntity(), /*claimed_pin=*/nullptr, expected_response,
std::move(ui_request));
run_loop.Run();

EXPECT_FALSE(manager_.is_registered());
}

#endif // BUILDFLAG(IS_WIN)

#endif // BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC)
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/webauthn/gpm_enclave_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,9 @@ void GPMEnclaveController::StartEnclaveTransaction(
request->user_verified = true;
request->uv_key_creation_callback =
enclave_manager_->UserVerifyingKeyCreationCallback();
request->unregister_callback =
base::BindOnce(&EnclaveManager::Unenroll,
enclave_manager_->GetWeakPtr(), base::DoNothing());
break;
case EnclaveUserVerificationMethod::kUnsatisfiable:
NOTREACHED_NORETURN();
Expand Down
2 changes: 1 addition & 1 deletion crypto/scoped_fake_user_verifying_key_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class FakeUserVerifyingKeyProvider : public UserVerifyingKeyProvider {

class FailingUserVerifyingSigningKey : public UserVerifyingSigningKey {
public:
FailingUserVerifyingSigningKey() : label_("") {}
FailingUserVerifyingSigningKey() : label_("test") {}
~FailingUserVerifyingSigningKey() override = default;

void Sign(base::span<const uint8_t> data,
Expand Down
98 changes: 54 additions & 44 deletions device/fido/enclave/enclave_authenticator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ void EnclaveAuthenticator::MakeCredential(CtapMakeCredentialRequest request,
std::move(request), std::move(options), std::move(callback));

if (ui_request_->uv_key_creation_callback) {
includes_new_uv_key_ = true;
std::move(ui_request_->uv_key_creation_callback)
.Run(base::BindOnce(
&EnclaveAuthenticator::DispatchMakeCredentialWithNewUVKey,
Expand Down Expand Up @@ -195,6 +196,7 @@ void EnclaveAuthenticator::GetAssertion(CtapGetAssertionRequest request,
request, options, std::move(callback));

if (ui_request_->uv_key_creation_callback) {
includes_new_uv_key_ = true;
std::move(ui_request_->uv_key_creation_callback)
.Run(base::BindOnce(
&EnclaveAuthenticator::DispatchGetAssertionWithNewUVKey,
Expand Down Expand Up @@ -257,28 +259,9 @@ void EnclaveAuthenticator::ProcessMakeCredentialResponse(
auto parse_result = ParseMakeCredentialResponse(
std::move(*response), pending_make_credential_request_->request,
*ui_request_->key_version, ui_request_->user_verified);
// TODO(enclave): This should identify and handle failures of AddUVKey
// requests, and signal these back to the EnclaveManager to unregister the
// device.
if (absl::holds_alternative<std::string>(parse_result)) {
FIDO_LOG(ERROR) << base::StrCat(
{"Error in registration response from server: ",
absl::get<std::string>(parse_result)});
CompleteRequestWithError(CtapDeviceResponseCode::kCtap2ErrOther);
return;
}
if (absl::holds_alternative<int>(parse_result)) {
int code = absl::get<int>(parse_result);
if (ui_request_->pin_result_callback &&
(code == kIncorrectPIN || code == kPINLocked)) {
std::move(ui_request_->pin_result_callback)
.Run(code == kIncorrectPIN ? PINValidationResult::kIncorrect
: PINValidationResult::kLocked);
}
FIDO_LOG(DEBUG) << base::StrCat(
{"Received an error response from the enclave: ",
base::NumberToString(code)});
CompleteRequestWithError(EnclaveErrorToCtapResponseCode(code));
if (absl::holds_alternative<ErrorResponse>(parse_result)) {
auto& error_details = absl::get<ErrorResponse>(parse_result);
ProcessErrorResponse(error_details);
return;
}

Expand All @@ -305,28 +288,9 @@ void EnclaveAuthenticator::ProcessGetAssertionResponse(
const std::string& cred_id_str = ui_request_->entity->credential_id();
auto parse_result = ParseGetAssertionResponse(
std::move(*response), base::as_bytes(base::make_span(cred_id_str)));
if (absl::holds_alternative<std::string>(parse_result)) {
FIDO_LOG(ERROR) << base::StrCat(
{"Error in assertion response from server: ",
absl::get<std::string>(parse_result)});
CompleteRequestWithError(CtapDeviceResponseCode::kCtap2ErrOther);
return;
}
// TODO(enclave): This should identify and handle failures of AddUVKey
// requests, and signal these back to the EnclaveManager to unregister the
// device.
if (absl::holds_alternative<int>(parse_result)) {
int code = absl::get<int>(parse_result);
if (ui_request_->pin_result_callback &&
(code == kIncorrectPIN || code == kPINLocked)) {
std::move(ui_request_->pin_result_callback)
.Run(code == kIncorrectPIN ? PINValidationResult::kIncorrect
: PINValidationResult::kLocked);
}
FIDO_LOG(DEBUG) << base::StrCat(
{"Received an error response from the enclave: ",
base::NumberToString(code)});
CompleteRequestWithError(EnclaveErrorToCtapResponseCode(code));
if (absl::holds_alternative<ErrorResponse>(parse_result)) {
auto& error_details = absl::get<ErrorResponse>(parse_result);
ProcessErrorResponse(error_details);
return;
}
if (ui_request_->pin_result_callback) {
Expand Down Expand Up @@ -387,6 +351,52 @@ void EnclaveAuthenticator::CompleteGetAssertionRequest(
pending_get_assertion_request_.reset();
}

void EnclaveAuthenticator::ProcessErrorResponse(const ErrorResponse& error) {
if (includes_new_uv_key_ && error.index < 1) {
// An error was received while trying to register a new UV key. If
// the error index is 1 or more then the error is specific to a request
// following the UV key submission, which is fine. Otherwise the UV key
// was not successfully submitted which is fatal to the device's
// enclave service registration.
std::move(ui_request_->unregister_callback).Run();
if (error.error_string.has_value()) {
FIDO_LOG(ERROR)
<< "Failed UV key submission. Error in registration response from "
"server: "
<< *error.error_string;
CompleteRequestWithError(CtapDeviceResponseCode::kCtap2ErrOther);
} else {
CHECK(error.error_code.has_value());
FIDO_LOG(DEBUG)
<< "Failed UV key submission. Received an error response from the "
"enclave: "
<< *error.error_code;
CompleteRequestWithError(
EnclaveErrorToCtapResponseCode(*error.error_code));
}
return;
}
if (error.error_string.has_value()) {
FIDO_LOG(ERROR) << base::StrCat(
{"Error in registration response from server: ", *error.error_string});
CompleteRequestWithError(CtapDeviceResponseCode::kCtap2ErrOther);
return;
}

CHECK(error.error_code.has_value());
int code = *error.error_code;
if (ui_request_->pin_result_callback &&
(code == kIncorrectPIN || code == kPINLocked)) {
std::move(ui_request_->pin_result_callback)
.Run(code == kIncorrectPIN ? PINValidationResult::kIncorrect
: PINValidationResult::kLocked);
}
FIDO_LOG(DEBUG) << base::StrCat(
{"Received an error response from the enclave: ",
base::NumberToString(code)});
CompleteRequestWithError(EnclaveErrorToCtapResponseCode(code));
}

void EnclaveAuthenticator::Cancel() {}

AuthenticatorType EnclaveAuthenticator::GetType() const {
Expand Down
Loading

0 comments on commit 0e7f401

Please sign in to comment.