Skip to content

Commit

Permalink
[webauthn] Virtual attestation cert valid
Browse files Browse the repository at this point in the history 
The virtual authenticator issues self signed certificates for
attestation. Fix the valid from and expiry dates so they're valid for
about 20 years.

This also makes some attestation object functions be const so we can
call them from const references.

Fixed: 1077081
Change-Id: I75dbd4042eef4d2e120fa22a8b912906e14f6f82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2183114
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Auto-Submit: Nina Satragno <nsatragno@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#766444}
  • Loading branch information
@nsatragno
nsatragno authored and Commit Bot committed May 7, 2020
1 parent 5fa6b10 commit 0a63a1e
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 29 deletions.
4 changes: 2 additions & 2 deletions device/fido/attestation_statement.cc
View file
Expand Up @@ -22,11 +22,11 @@ NoneAttestationStatement::NoneAttestationStatement()
NoneAttestationStatement::~NoneAttestationStatement() = default;

bool NoneAttestationStatement::
IsAttestationCertificateInappropriatelyIdentifying() {
IsAttestationCertificateInappropriatelyIdentifying() const {
return false;
}

bool NoneAttestationStatement::IsSelfAttestation() {
bool NoneAttestationStatement::IsSelfAttestation() const {
return false;
}

Expand Down
8 changes: 4 additions & 4 deletions device/fido/attestation_statement.h
View file
Expand Up @@ -35,13 +35,13 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AttestationStatement {

// Returns true if the attestation is a "self" attestation, i.e. is just the
// private key signing itself to show that it is fresh.
virtual bool IsSelfAttestation() = 0;
virtual bool IsSelfAttestation() const = 0;

// Returns true if the attestation is known to be inappropriately identifying.
// Some tokens return unique attestation certificates even when the bit to
// request that is not set. (Normal attestation certificates are not
// indended to be trackable.)
virtual bool IsAttestationCertificateInappropriatelyIdentifying() = 0;
virtual bool IsAttestationCertificateInappropriatelyIdentifying() const = 0;

// Return the DER bytes of the leaf X.509 certificate, if any.
virtual base::Optional<base::span<const uint8_t>> GetLeafCertificate()
Expand All @@ -67,8 +67,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) NoneAttestationStatement
~NoneAttestationStatement() override;

cbor::Value AsCBOR() const override;
bool IsSelfAttestation() override;
bool IsAttestationCertificateInappropriatelyIdentifying() override;
bool IsSelfAttestation() const override;
bool IsAttestationCertificateInappropriatelyIdentifying() const override;
base::Optional<base::span<const uint8_t>> GetLeafCertificate() const override;

private:
Expand Down
8 changes: 4 additions & 4 deletions device/fido/attestation_statement_formats.cc
View file
Expand Up @@ -135,12 +135,12 @@ cbor::Value FidoAttestationStatement::AsCBOR() const {
return cbor::Value(std::move(attestation_statement_map));
}

bool FidoAttestationStatement::IsSelfAttestation() {
bool FidoAttestationStatement::IsSelfAttestation() const {
return false;
}

bool FidoAttestationStatement::
IsAttestationCertificateInappropriatelyIdentifying() {
IsAttestationCertificateInappropriatelyIdentifying() const {
// An attestation certificate is considered inappropriately identifying if it
// contains a common name of "FT FIDO 0100". See "Inadequately batched
// attestation certificates" on https://www.chromium.org/security-keys
Expand Down Expand Up @@ -194,12 +194,12 @@ cbor::Value PackedAttestationStatement::AsCBOR() const {
return cbor::Value(std::move(attestation_statement_map));
}

bool PackedAttestationStatement::IsSelfAttestation() {
bool PackedAttestationStatement::IsSelfAttestation() const {
return x509_certificates_.empty();
}

bool PackedAttestationStatement::
IsAttestationCertificateInappropriatelyIdentifying() {
IsAttestationCertificateInappropriatelyIdentifying() const {
for (const auto& der_bytes : x509_certificates_) {
if (IsCertificateInappropriatelyIdentifying(der_bytes)) {
return true;
Expand Down
8 changes: 4 additions & 4 deletions device/fido/attestation_statement_formats.h
View file
Expand Up @@ -30,8 +30,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoAttestationStatement
~FidoAttestationStatement() override;

cbor::Value AsCBOR() const override;
bool IsSelfAttestation() override;
bool IsAttestationCertificateInappropriatelyIdentifying() override;
bool IsSelfAttestation() const override;
bool IsAttestationCertificateInappropriatelyIdentifying() const override;
base::Optional<base::span<const uint8_t>> GetLeafCertificate() const override;

private:
Expand All @@ -56,8 +56,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) PackedAttestationStatement
~PackedAttestationStatement() override;

cbor::Value AsCBOR() const override;
bool IsSelfAttestation() override;
bool IsAttestationCertificateInappropriatelyIdentifying() override;
bool IsSelfAttestation() const override;
bool IsAttestationCertificateInappropriatelyIdentifying() const override;
base::Optional<base::span<const uint8_t>> GetLeafCertificate() const override;

private:
Expand Down
64 changes: 64 additions & 0 deletions device/fido/fido_test_data.h
View file
Expand Up @@ -864,6 +864,70 @@ constexpr uint8_t kTestCorruptedU2fSignResponse[] = {0x01, 0x00, 0x00,
0x00, 0x90, 0x00};

// CTAP requests ---------------------------------------------------------------
// A MakeCredential request with no RK and no UV.
constexpr uint8_t kCtapSimpleMakeCredentialRequest[] = {
// clang-format off
// authenticatorMakeCredential command
0x01,
// map(5)
0xa5,
// key(1) - clientDataHash
0x01,
// bytes(32) -- see kClientDataHash
0x58, 0x20, 0x8d, 0xd8, 0x74, 0x4d, 0x79, 0x3, 0xb0, 0xa3, 0x53, 0x8a, 0x49,
0xea, 0xfa, 0xae, 0xc8, 0x33, 0xac, 0xbf, 0xd2, 0x85, 0xa5, 0xdf, 0x44, 0x3,
0xa2, 0xe, 0x4e, 0x13, 0xe3, 0xd5, 0x3e, 0x50,
// key(2) - rp
0x02,
// map(2)
0xa2,
// key - "id"
0x62, 0x69, 0x64,
// value - "acme.com"
0x68, 0x61, 0x63, 0x6d, 0x65, 0x2e, 0x63, 0x6f, 0x6d,
// key - "name"
0x64, 0x6e, 0x61, 0x6d, 0x65,
// value - "Acme"
0x64, 0x41, 0x63, 0x6d, 0x65,
// key(3) - user
0x03,
// map(3)
0xa3,
// key - "id"
0x62, 0x69, 0x64,
// value - user id
0x48, 0x10, 0x98, 0x23, 0x72, 0x35, 0x40, 0x98, 0x72,
// key - "name"
0x64, 0x6e, 0x61, 0x6d, 0x65,
// value - "johnpsmith@example.com"
0x76, 0x6a, 0x6f, 0x68, 0x6e, 0x70, 0x73, 0x6d, 0x69, 0x74, 0x68, 0x40,
0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d,
// key - "displayName"
0x6b, 0x64, 0x69, 0x73, 0x70, 0x6c, 0x61, 0x79, 0x4e, 0x61, 0x6d, 0x65,
// value - "John P. Smith"
0x6d, 0x4a, 0x6f, 0x68, 0x6e, 0x20, 0x50, 0x2e, 0x20, 0x53, 0x6d, 0x69,
0x74, 0x68,
// key(4) - pubKeyCredParams
0x04,
// array(1)
0x81,
// map(2)
0xa2,
// key - "alg"
0x63, 0x61, 0x6c, 0x67,
// value - -7
0x26,
// key - "type"
0x64, 0x74, 0x79, 0x70, 0x65,
// value - "public-key"
0x6a, 0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79,
// key(7) - options
0x07,
// map(0)
0xa0,
// clang-format on
};

constexpr uint8_t kCtapMakeCredentialRequest[] = {
// clang-format off
// authenticatorMakeCredential command
Expand Down
4 changes: 2 additions & 2 deletions device/fido/opaque_attestation_statement.cc
View file
Expand Up @@ -32,7 +32,7 @@ Value OpaqueAttestationStatement::AsCBOR() const {
return cbor::Value(std::move(new_map));
}

bool OpaqueAttestationStatement::IsSelfAttestation() {
bool OpaqueAttestationStatement::IsSelfAttestation() const {
DCHECK(attestation_statement_map_.is_map());
const Value::MapValue& m(attestation_statement_map_.GetMap());
const Value alg("alg");
Expand All @@ -43,7 +43,7 @@ bool OpaqueAttestationStatement::IsSelfAttestation() {
}

bool OpaqueAttestationStatement::
IsAttestationCertificateInappropriatelyIdentifying() {
IsAttestationCertificateInappropriatelyIdentifying() const {
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions device/fido/opaque_attestation_statement.h
View file
Expand Up @@ -24,8 +24,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) OpaqueAttestationStatement

// AttestationStatement:
cbor::Value AsCBOR() const override;
bool IsSelfAttestation() override;
bool IsAttestationCertificateInappropriatelyIdentifying() override;
bool IsSelfAttestation() const override;
bool IsAttestationCertificateInappropriatelyIdentifying() const override;
base::Optional<base::span<const uint8_t>> GetLeafCertificate() const override;

private:
Expand Down
71 changes: 61 additions & 10 deletions device/fido/virtual_ctap2_device_unittest.cc
View file
Expand Up @@ -11,9 +11,13 @@
#include "base/test/bind_test_util.h"
#include "base/test/task_environment.h"
#include "components/cbor/reader.h"
#include "device/fido/attestation_statement.h"
#include "device/fido/authenticator_get_assertion_response.h"
#include "device/fido/device_response_converter.h"
#include "device/fido/fido_parsing_utils.h"
#include "device/fido/fido_test_data.h"
#include "device/fido/test_callback_receiver.h"
#include "net/cert/x509_certificate.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -25,23 +29,28 @@ using TestCallbackReceiver =
test::ValueCallbackReceiver<base::Optional<std::vector<uint8_t>>>;

void SendCommand(VirtualCtap2Device* device,
base::span<const uint8_t> command) {
base::span<const uint8_t> command,
FidoDevice::DeviceCallback callback = base::DoNothing()) {
device->DeviceTransact(fido_parsing_utils::Materialize(command),
base::DoNothing());
std::move(callback));
}

// DecodeCBOR parses a CBOR structure, ignoring the first byte of |in|, which is
// assumed to be a CTAP2 status byte.
base::Optional<cbor::Value> DecodeCBOR(base::span<const uint8_t> in) {
CHECK(!in.empty());
return cbor::Reader::Read(in.subspan(1));
}

} // namespace

class VirtualCtap2DeviceTest : public ::testing::Test {
protected:
void MakeSelfDestructingDevice() {
auto state = base::MakeRefCounted<VirtualFidoDevice::State>();
state->fingerprints_enrolled = true;
VirtualCtap2Device::Config config;
config.internal_uv_support = true;
device_ = std::make_unique<VirtualCtap2Device>(state, std::move(config));
void MakeDevice() { device_ = std::make_unique<VirtualCtap2Device>(); }

state->simulate_press_callback =
void MakeSelfDestructingDevice() {
MakeDevice();
device_->mutable_state()->simulate_press_callback =
base::BindLambdaForTesting([&](VirtualFidoDevice* _) {
device_.reset();
return true;
Expand Down Expand Up @@ -129,12 +138,54 @@ TEST_F(VirtualCtap2DeviceTest, ParseGetAssertionRequestForVirtualCtapKey) {
// does not crash.
TEST_F(VirtualCtap2DeviceTest, DestroyInsideSimulatePressCallback) {
MakeSelfDestructingDevice();
SendCommand(device_.get(), test_data::kCtapMakeCredentialRequest);
SendCommand(device_.get(), test_data::kCtapSimpleMakeCredentialRequest);
ASSERT_FALSE(device_);

MakeSelfDestructingDevice();
SendCommand(device_.get(), test_data::kCtapGetAssertionRequest);
ASSERT_FALSE(device_);
}

// Tests that the attestation certificate returned on MakeCredential is valid.
// See
// https://w3c.github.io/webauthn/#sctn-packed-attestation-cert-requirements
TEST_F(VirtualCtap2DeviceTest, AttestationCertificateIsValid) {
MakeDevice();
TestCallbackReceiver callback_receiver;
SendCommand(device_.get(), test_data::kCtapSimpleMakeCredentialRequest,
callback_receiver.callback());
callback_receiver.WaitForCallback();

base::Optional<cbor::Value> cbor = DecodeCBOR(*callback_receiver.value());
ASSERT_TRUE(cbor);
base::Optional<AuthenticatorMakeCredentialResponse> response =
ReadCTAPMakeCredentialResponse(
FidoTransportProtocol::kUsbHumanInterfaceDevice, std::move(cbor));
ASSERT_TRUE(response);

const AttestationStatement& attestation =
response->attestation_object().attestation_statement();

EXPECT_FALSE(attestation.IsSelfAttestation());
EXPECT_FALSE(
attestation.IsAttestationCertificateInappropriatelyIdentifying());

base::span<const uint8_t> cert_bytes = *attestation.GetLeafCertificate();
scoped_refptr<net::X509Certificate> cert =
net::X509Certificate::CreateFromBytes(
reinterpret_cast<const char*>(cert_bytes.data()), cert_bytes.size());
ASSERT_TRUE(cert);

const auto& subject = cert->subject();
EXPECT_EQ("Batch Certificate", subject.common_name);
EXPECT_EQ("US", subject.country_name);
EXPECT_THAT(subject.organization_names, testing::ElementsAre("Chromium"));
EXPECT_THAT(subject.organization_unit_names,
testing::ElementsAre("Authenticator Attestation"));

base::Time now = base::Time::Now();
EXPECT_LT(cert->valid_start(), now);
EXPECT_GT(cert->valid_expiry(), now);
}

} // namespace device
5 changes: 4 additions & 1 deletion device/fido/virtual_fido_device.cc
View file
Expand Up @@ -194,6 +194,9 @@ VirtualFidoDevice::GenerateAttestationCertificate(
};

// https://w3c.github.io/webauthn/#sctn-packed-attestation-cert-requirements
// Make the certificate expire about 20 years from now.
base::Time expiry_date =
base::Time::Now() + base::TimeDelta::FromDays(365 * 20);
std::string attestation_cert;
if (!net::x509_util::CreateSelfSignedCert(
attestation_private_key->key(), net::x509_util::DIGEST_SHA256,
Expand All @@ -202,7 +205,7 @@ VirtualFidoDevice::GenerateAttestationCertificate(
? state_->individual_attestation_cert_common_name
: state_->attestation_cert_common_name),
kAttestationCertSerialNumber, base::Time::FromTimeT(1500000000),
base::Time::FromTimeT(1500000000), extensions, &attestation_cert)) {
expiry_date, extensions, &attestation_cert)) {
DVLOG(2) << "Failed to create attestation certificate";
return base::nullopt;
}
Expand Down

0 comments on commit 0a63a1e

Please sign in to comment.