Skip to content

Commit

Permalink
webauthn: set credProtect=3 for uv=preferred, rk=required.
Browse files Browse the repository at this point in the history 
Change-Id: Ibdc36d22239432e5dc0297754bc6aa4e8b10b37f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4193550
Auto-Submit: Adam Langley <agl@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/main@{#1098107}
  • Loading branch information
Adam Langley authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent e8c66e0 commit afd522f
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 62 deletions.
88 changes: 66 additions & 22 deletions content/browser/webauth/authenticator_common_impl.cc
View file
Expand Up @@ -33,6 +33,7 @@
#include "device/fido/authenticator_data.h"
#include "device/fido/authenticator_get_assertion_response.h"
#include "device/fido/ctap_make_credential_request.h"
#include "device/fido/features.h"
#include "device/fido/fido_authenticator.h"
#include "device/fido/fido_constants.h"
#include "device/fido/fido_parsing_utils.h"
Expand All @@ -57,7 +58,6 @@

#if BUILDFLAG(IS_CHROMEOS)
#include "device/fido/cros/authenticator.h"
#include "device/fido/features.h"
#endif

#if BUILDFLAG(IS_WIN)
Expand Down Expand Up @@ -332,6 +332,68 @@ std::unique_ptr<device::FidoDiscoveryFactory> MakeDiscoveryFactory(
return discovery_factory;
}

absl::optional<device::CredProtectRequest> ProtectionPolicyToCredProtect(
blink::mojom::ProtectionPolicy protection_policy,
const device::MakeCredentialOptions& make_credential_options) {
switch (protection_policy) {
case blink::mojom::ProtectionPolicy::UNSPECIFIED:
// Some platform authenticators have the behaviour that uv=required
// demands a local reauthentication but uv=preferred can be satisfied by
// just clicking a button. Since the device has to be unlocked by the
// user, this seems to balance the demands of uv=required against the
// fact that quite a number of (non-mobile) devices lack biometrics and
// thus full UV requires entering the local password. Since password
// autofill doesn't demand entering the local password all the time, it
// would be sad if WebAuthn was much worse in that respect.
//
// Also, some sites have (or will) implement a sign-in flow where the
// user enters their username and then the site makes a WebAuthn
// request, with an allowlist, where completing that request is
// sufficient to sign-in. I.e. there's no additional password challenge.
// Since these sites are trying to replace passwords, we expect them to
// set uv=preferred in order to work well with the platform behaviour
// detailed in the first paragraph.
//
// If such sites remembered the UV flag from the registration and enforced
// it at assertion time, that would break situations where closing a
// laptop lid covers the biometric sensor and makes entering a password
// preferable. But without any enforcement of the UV flag, someone could
// pick a security key off the ground and do a uv=false request to get a
// sufficient assertion.
//
// Thus if rk=required and uv=preferred, credProtect level three is set
// to tell security keys to only create an assertion after UV for this
// credential. (Sites can still override this by setting a specific
// credProtect level.)
//
// If a site sets rk=preferred then we assume that they're doing something
// unusual and will only set credProtect level two.
//
// See also
// https://chromium.googlesource.com/chromium/src/+/main/content/browser/webauth/cred_protect.md
if (make_credential_options.resident_key ==
device::ResidentKeyRequirement::kRequired &&
make_credential_options.user_verification ==
device::UserVerificationRequirement::kPreferred &&
base::FeatureList::IsEnabled(device::kWebAuthnCredProtectThree)) {
return device::CredProtectRequest::kUVRequired;
}
if (make_credential_options.resident_key !=
device::ResidentKeyRequirement::kDiscouraged) {
// Otherwise, kUVOrCredIDRequired is made the default unless
// the authenticator defaults to something better.
return device::CredProtectRequest::kUVOrCredIDRequiredOrBetter;
}
return absl::nullopt;
case blink::mojom::ProtectionPolicy::NONE:
return device::CredProtectRequest::kUVOptional;
case blink::mojom::ProtectionPolicy::UV_OR_CRED_ID_REQUIRED:
return device::CredProtectRequest::kUVOrCredIDRequired;
case blink::mojom::ProtectionPolicy::UV_REQUIRED:
return device::CredProtectRequest::kUVRequired;
}
}

} // namespace

// static
Expand Down Expand Up @@ -697,27 +759,9 @@ void AuthenticatorCommonImpl::MakeCredential(
return;
}

absl::optional<device::CredProtectRequest> cred_protect_request;
switch (options->protection_policy) {
case blink::mojom::ProtectionPolicy::UNSPECIFIED:
if (might_create_resident_key) {
// If not specified, kUVOrCredIDRequired is made the default unless
// the authenticator defaults to something better.
cred_protect_request =
device::CredProtectRequest::kUVOrCredIDRequiredOrBetter;
}
break;
case blink::mojom::ProtectionPolicy::NONE:
cred_protect_request = device::CredProtectRequest::kUVOptional;
break;
case blink::mojom::ProtectionPolicy::UV_OR_CRED_ID_REQUIRED:
cred_protect_request = device::CredProtectRequest::kUVOrCredIDRequired;
break;
case blink::mojom::ProtectionPolicy::UV_REQUIRED:
cred_protect_request = device::CredProtectRequest::kUVRequired;
break;
}

absl::optional<device::CredProtectRequest> cred_protect_request =
ProtectionPolicyToCredProtect(options->protection_policy,
*make_credential_options_);
if (cred_protect_request) {
make_credential_options_->cred_protect_request = {
{*cred_protect_request, options->enforce_protection_policy}};
Expand Down
88 changes: 48 additions & 40 deletions content/browser/webauth/authenticator_impl_unittest.cc
View file
Expand Up @@ -7676,58 +7676,68 @@ TEST_F(ResidentKeyAuthenticatorImplTest, CredProtectRegistration) {
const int kOk = 0;
const int kNonsense = 1;
const int kNotAllow = 2;
const device::UserVerificationRequirement kUV =
device::UserVerificationRequirement::kRequired;
const device::UserVerificationRequirement kUP =
device::UserVerificationRequirement::kDiscouraged;
const device::UserVerificationRequirement kUVPref =
device::UserVerificationRequirement::kPreferred;

const struct {
bool supported_by_authenticator;
bool is_resident;
blink::mojom::ProtectionPolicy protection;
bool enforce;
bool uv;
device::UserVerificationRequirement uv;
int expected_outcome;
blink::mojom::ProtectionPolicy resulting_policy;
} kExpectations[] = {
// clang-format off
// Support | Resdnt | Level | Enf | UV || Result | Prot level
{ false, false, UNSPECIFIED, false, false, kOk, NONE},
{ false, false, UNSPECIFIED, true, false, kNonsense, UNSPECIFIED},
{ false, false, NONE, false, false, kNonsense, UNSPECIFIED},
{ false, false, NONE, true, false, kNonsense, UNSPECIFIED},
{ false, false, UV_OR_CRED, false, false, kOk, NONE},
{ false, false, UV_OR_CRED, true, false, kNotAllow, UNSPECIFIED},
{ false, false, UV_OR_CRED, false, true, kOk, NONE},
{ false, false, UV_OR_CRED, true, true, kNotAllow, UNSPECIFIED},
{ false, false, UV_REQ, false, false, kNonsense, UNSPECIFIED},
{ false, false, UV_REQ, false, true, kOk, NONE},
{ false, false, UV_REQ, true, false, kNonsense, UNSPECIFIED},
{ false, false, UV_REQ, true, true, kNotAllow, UNSPECIFIED},
{ false, true, UNSPECIFIED, false, false, kOk, NONE},
{ false, true, UNSPECIFIED, true, false, kNonsense, UNSPECIFIED},
{ false, true, NONE, false, false, kOk, NONE},
{ false, true, NONE, true, false, kNonsense, UNSPECIFIED},
{ false, true, UV_OR_CRED, false, false, kOk, NONE},
{ false, true, UV_OR_CRED, true, false, kNotAllow, UNSPECIFIED},
{ false, true, UV_REQ, false, false, kNonsense, UNSPECIFIED},
{ false, true, UV_REQ, false, true, kOk, NONE},
{ false, true, UV_REQ, true, false, kNonsense, UNSPECIFIED},
{ false, true, UV_REQ, true, true, kNotAllow, UNSPECIFIED},
{ false, false, UNSPECIFIED, false, kUP, kOk, NONE},
{ false, false, UNSPECIFIED, true, kUP, kNonsense, UNSPECIFIED},
{ false, false, UNSPECIFIED, false, kUVPref, kOk, NONE},
{ false, false, NONE, false, kUP, kNonsense, UNSPECIFIED},
{ false, false, NONE, true, kUP, kNonsense, UNSPECIFIED},
{ false, false, UV_OR_CRED, false, kUP, kOk, NONE},
{ false, false, UV_OR_CRED, true, kUP, kNotAllow, UNSPECIFIED},
{ false, false, UV_OR_CRED, false, kUV, kOk, NONE},
{ false, false, UV_OR_CRED, true, kUV, kNotAllow, UNSPECIFIED},
{ false, false, UV_REQ, false, kUP, kNonsense, UNSPECIFIED},
{ false, false, UV_REQ, false, kUV, kOk, NONE},
{ false, false, UV_REQ, true, kUP, kNonsense, UNSPECIFIED},
{ false, false, UV_REQ, true, kUV, kNotAllow, UNSPECIFIED},
{ false, true, UNSPECIFIED, false, kUP, kOk, NONE},
{ false, true, UNSPECIFIED, true, kUP, kNonsense, UNSPECIFIED},
{ false, true, NONE, false, kUP, kOk, NONE},
{ false, true, NONE, true, kUP, kNonsense, UNSPECIFIED},
{ false, true, UV_OR_CRED, false, kUP, kOk, NONE},
{ false, true, UV_OR_CRED, true, kUP, kNotAllow, UNSPECIFIED},
{ false, true, UV_REQ, false, kUP, kNonsense, UNSPECIFIED},
{ false, true, UV_REQ, false, kUV, kOk, NONE},
{ false, true, UV_REQ, true, kUP, kNonsense, UNSPECIFIED},
{ false, true, UV_REQ, true, kUV, kNotAllow, UNSPECIFIED},

// For the case where the authenticator supports credProtect we do not
// repeat the cases above that are |kNonsense| on the assumption that
// authenticator support is irrelevant. Therefore these are just the non-
// kNonsense cases from the prior block.
{ true, false, UNSPECIFIED, false, false, kOk, NONE},
{ true, false, UV_OR_CRED, false, false, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, true, false, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, false, true, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, true, true, kOk, UV_OR_CRED},
{ true, false, UV_REQ, false, true, kOk, UV_REQ},
{ true, false, UV_REQ, true, true, kOk, UV_REQ},
{ true, true, UNSPECIFIED, false, false, kOk, UV_OR_CRED},
{ true, true, NONE, false, false, kOk, NONE},
{ true, true, UV_OR_CRED, false, false, kOk, UV_OR_CRED},
{ true, true, UV_OR_CRED, true, false, kOk, UV_OR_CRED},
{ true, true, UV_REQ, false, true, kOk, UV_REQ},
{ true, true, UV_REQ, true, true, kOk, UV_REQ},
{ true, false, UNSPECIFIED, false, kUP, kOk, NONE},
{ true, false, UV_OR_CRED, false, kUP, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, true, kUP, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, false, kUV, kOk, UV_OR_CRED},
{ true, false, UV_OR_CRED, true, kUV, kOk, UV_OR_CRED},
{ true, false, UV_REQ, false, kUV, kOk, UV_REQ},
{ true, false, UV_REQ, true, kUV, kOk, UV_REQ},
{ true, true, UNSPECIFIED, false, kUP, kOk, UV_OR_CRED},
{ true, true, UNSPECIFIED, false, kUVPref, kOk, UV_REQ},
{ true, true, NONE, false, kUP, kOk, NONE},
{ true, true, NONE, false, kUVPref, kOk, NONE},
{ true, true, UV_OR_CRED, false, kUP, kOk, UV_OR_CRED},
{ true, true, UV_OR_CRED, true, kUP, kOk, UV_OR_CRED},
{ true, true, UV_OR_CRED, false, kUVPref, kOk, UV_OR_CRED},
{ true, true, UV_REQ, false, kUV, kOk, UV_REQ},
{ true, true, UV_REQ, true, kUV, kOk, UV_REQ},
// clang-format on
};

Expand All @@ -7739,7 +7749,7 @@ TEST_F(ResidentKeyAuthenticatorImplTest, CredProtectRegistration) {
virtual_device_factory_->SetCtap2Config(config);
virtual_device_factory_->mutable_state()->registrations.clear();

SCOPED_TRACE(::testing::Message() << "uv=" << test.uv);
SCOPED_TRACE(::testing::Message() << "uv=" << UVToString(test.uv));
SCOPED_TRACE(::testing::Message() << "enforce=" << test.enforce);
SCOPED_TRACE(::testing::Message()
<< "level=" << ProtectionPolicyDescription(test.protection));
Expand All @@ -7753,9 +7763,7 @@ TEST_F(ResidentKeyAuthenticatorImplTest, CredProtectRegistration) {
: device::ResidentKeyRequirement::kDiscouraged;
options->protection_policy = test.protection;
options->enforce_protection_policy = test.enforce;
options->authenticator_selection->user_verification_requirement =
test.uv ? device::UserVerificationRequirement::kRequired
: device::UserVerificationRequirement::kDiscouraged;
options->authenticator_selection->user_verification_requirement = test.uv;

AuthenticatorStatus status =
AuthenticatorMakeCredential(std::move(options)).status;
Expand Down
9 changes: 9 additions & 0 deletions content/browser/webauth/cred_protect.md
View file
@@ -0,0 +1,9 @@
# Chromium's credProtect behaviour in WebAuthn

CTAP defines an extension called [`credProtect`](https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#sctn-credProtect-extension) which restricts when credentials on security keys may be used. Support for it is required [in some cases](https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#mandatory-features) and thus it is widely supported by security keys.

Chromium will request a protection level of [userVerificationOptionalWithCredentialIDList](https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#userverificationoptionalwithcredentialidlist) when creating a credential if [`residentKey`](https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-residentkey) is set to `preferred` or `required`. (Setting [`requireResidentKey`](https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-requireresidentkey) is treated the same as `required`.) This ensures that simple physical possession of a security key does not allow the presence of a discoverable credential for a given RP ID to be queried.

Additionally, if [`residentKey`](https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-residentkey) is `required` _and_ [`userVerification`](https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-userverification) is `preferred`, the protection level will be increased to [userVerificationRequired](https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#userverificationrequired). This ensures that physical possession of a security key does not allow sign-in to a site that doesn't demand user verification. (This is not a complete protection; sites should still carefully consider the security of their users.)

If an explicit `credProtect` level is requested by the site, that will override these defaults. These defaults never cause the protection level to be lower than the security key's default, if that is higher.
5 changes: 5 additions & 0 deletions device/fido/features.cc
View file
Expand Up @@ -52,4 +52,9 @@ BASE_FEATURE(kWebAuthnNoPasskeysError,
"WebAuthenticationNoPasskeysError",
base::FEATURE_ENABLED_BY_DEFAULT);

// Added in M112. Remove in or after M115.
BASE_FEATURE(kWebAuthnCredProtectThree,
"WebAuthenticationCredProtectThree",
base::FEATURE_ENABLED_BY_DEFAULT);

} // namespace device
4 changes: 4 additions & 0 deletions device/fido/features.h
View file
Expand Up @@ -52,6 +52,10 @@ BASE_DECLARE_FEATURE(kDisableWebAuthnWithBrokenCerts);
COMPONENT_EXPORT(DEVICE_FIDO)
BASE_DECLARE_FEATURE(kWebAuthnNoPasskeysError);

// Set credProtect=3 when rk=required and uv=preferred.
COMPONENT_EXPORT(DEVICE_FIDO)
BASE_DECLARE_FEATURE(kWebAuthnCredProtectThree);

} // namespace device

#endif // DEVICE_FIDO_FEATURES_H_

0 comments on commit afd522f

Please sign in to comment.