[webauthn] Add C, O and OU to virtual attestation cert

As per the spec [1], the attestation certificate must have the C, O, OU,
and CN attributes set.

[1] https://w3c.github.io/webauthn/#sctn-packed-attestation-cert-requirements

Fixed: 1035092
Change-Id: Iff61fe93136e45a0e20d42a0a08ddf5c34f0bc4e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1974703Reviewed-by: Adam Langley <agl@chromium.org>
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726403}

device/fido/virtual_fido_device.cc

@@ -193,12 +193,14 @@ VirtualFidoDevice::GenerateAttestationCertificate(
       {kTransportTypesOID, false /* not critical */, kTransportTypesContents},
   };
 
+  // https://w3c.github.io/webauthn/#sctn-packed-attestation-cert-requirements
   std::string attestation_cert;
   if (!net::x509_util::CreateSelfSignedCert(
           attestation_private_key->key(), net::x509_util::DIGEST_SHA256,
-          "CN=" + (individual_attestation_requested
-                       ? state_->individual_attestation_cert_common_name
-                       : state_->attestation_cert_common_name),
+          "C=US, O=Chromium, OU=Authenticator Attestation, CN=" +
+              (individual_attestation_requested
+                   ? state_->individual_attestation_cert_common_name
+                   : state_->attestation_cert_common_name),
           kAttestationCertSerialNumber, base::Time::FromTimeT(1500000000),
           base::Time::FromTimeT(1500000000), extensions, &attestation_cert)) {
     DVLOG(2) << "Failed to create attestation certificate";

net/cert/x509_util.cc

@@ -9,6 +9,7 @@
 #include <memory>
 
 #include "base/lazy_instance.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
@@ -75,23 +76,69 @@ const EVP_MD* ToEVP(DigestAlgorithm alg) {
   return nullptr;
 }
 
-// Adds an X.509 Name with the specified common name to |cbb|.
-bool AddNameWithCommonName(CBB* cbb, base::StringPiece common_name) {
+// Adds an X.509 Name with the specified distinguished name to |cbb|.
+bool AddName(CBB* cbb, base::StringPiece name) {
   // See RFC 4519.
   static const uint8_t kCommonName[] = {0x55, 0x04, 0x03};
+  static const uint8_t kCountryName[] = {0x55, 0x04, 0x06};
+  static const uint8_t kOrganizationName[] = {0x55, 0x04, 0x0a};
+  static const uint8_t kOrganizationalUnitName[] = {0x55, 0x04, 0x0b};
+
+  std::vector<std::string> attributes = SplitString(
+      name, /*separators=*/",", base::WhitespaceHandling::TRIM_WHITESPACE,
+      base::SplitResult::SPLIT_WANT_NONEMPTY);
+
+  if (attributes.size() == 0) {
+    LOG(ERROR) << "Missing DN or wrong format";
+    return false;
+  }
 
   // See RFC 5280, section 4.1.2.4.
-  CBB rdns, rdn, attr, type, value;
-  if (!CBB_add_asn1(cbb, &rdns, CBS_ASN1_SEQUENCE) ||
-      !CBB_add_asn1(&rdns, &rdn, CBS_ASN1_SET) ||
-      !CBB_add_asn1(&rdn, &attr, CBS_ASN1_SEQUENCE) ||
-      !CBB_add_asn1(&attr, &type, CBS_ASN1_OBJECT) ||
-      !CBB_add_bytes(&type, kCommonName, sizeof(kCommonName)) ||
-      !CBB_add_asn1(&attr, &value, CBS_ASN1_UTF8STRING) ||
-      !CBB_add_bytes(&value,
-                     reinterpret_cast<const uint8_t*>(common_name.data()),
-                     common_name.size()) ||
-      !CBB_flush(cbb)) {
+  CBB rdns;
+  if (!CBB_add_asn1(cbb, &rdns, CBS_ASN1_SEQUENCE)) {
+    return false;
+  }
+
+  for (const std::string& attribute : attributes) {
+    std::vector<std::string> parts =
+        SplitString(attribute, /*separators=*/"=",
+                    base::WhitespaceHandling::KEEP_WHITESPACE,
+                    base::SplitResult::SPLIT_WANT_ALL);
+    if (parts.size() != 2) {
+      LOG(ERROR) << "Wrong DN format at " + attribute;
+      return false;
+    }
+
+    const std::string& type_string = parts[0];
+    const std::string& value_string = parts[1];
+    base::span<const uint8_t> type_bytes;
+    if (type_string == "CN") {
+      type_bytes = kCommonName;
+    } else if (type_string == "C") {
+      type_bytes = kCountryName;
+    } else if (type_string == "O") {
+      type_bytes = kOrganizationName;
+    } else if (type_string == "OU") {
+      type_bytes = kOrganizationalUnitName;
+    } else {
+      LOG(ERROR) << "Unrecognized type " + type_string;
+      return false;
+    }
+
+    CBB rdn, attr, type, value;
+    if (!CBB_add_asn1(&rdns, &rdn, CBS_ASN1_SET) ||
+        !CBB_add_asn1(&rdn, &attr, CBS_ASN1_SEQUENCE) ||
+        !CBB_add_asn1(&attr, &type, CBS_ASN1_OBJECT) ||
+        !CBB_add_bytes(&type, type_bytes.data(), type_bytes.size()) ||
+        !CBB_add_asn1(&attr, &value, CBS_ASN1_UTF8STRING) ||
+        !CBB_add_bytes(&value,
+                       reinterpret_cast<const uint8_t*>(value_string.data()),
+                       value_string.size()) ||
+        !CBB_flush(&rdns)) {
+      return false;
+    }
+  }
+  if (!CBB_flush(cbb)) {
     return false;
   }
   return true;
@@ -235,17 +282,6 @@ bool CreateSelfSignedCert(EVP_PKEY* key,
   crypto::EnsureOpenSSLInit();
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
-  // Because |subject| only contains a common name and starts with 'CN=', there
-  // is no need for a full RFC 2253 parser here. Do some sanity checks though.
-  static const char kCommonNamePrefix[] = "CN=";
-  if (!base::StartsWith(subject, kCommonNamePrefix,
-                        base::CompareCase::SENSITIVE)) {
-    LOG(ERROR) << "Subject must begin with " << kCommonNamePrefix;
-    return false;
-  }
-  base::StringPiece common_name = subject;
-  common_name.remove_prefix(sizeof(kCommonNamePrefix) - 1);
-
   // See RFC 5280, section 4.1. First, construct the TBSCertificate.
   bssl::ScopedCBB cbb;
   CBB tbs_cert, version, validity;
@@ -257,13 +293,13 @@ bool CreateSelfSignedCert(EVP_PKEY* key,
                     CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0) ||
       !CBB_add_asn1_uint64(&version, 2) ||
       !CBB_add_asn1_uint64(&tbs_cert, serial_number) ||
-      !AddRSASignatureAlgorithm(&tbs_cert, alg) ||       // signature
-      !AddNameWithCommonName(&tbs_cert, common_name) ||  // issuer
+      !AddRSASignatureAlgorithm(&tbs_cert, alg) ||  // signature
+      !AddName(&tbs_cert, subject) ||               // issuer
       !CBB_add_asn1(&tbs_cert, &validity, CBS_ASN1_SEQUENCE) ||
       !AddTime(&validity, not_valid_before) ||
       !AddTime(&validity, not_valid_after) ||
-      !AddNameWithCommonName(&tbs_cert, common_name) ||  // subject
-      !EVP_marshal_public_key(&tbs_cert, key)) {         // subjectPublicKeyInfo
+      !AddName(&tbs_cert, subject) ||             // subject
+      !EVP_marshal_public_key(&tbs_cert, key)) {  // subjectPublicKeyInfo
     return false;
   }
 

net/cert/x509_util.h

@@ -47,9 +47,7 @@ NET_EXPORT_PRIVATE bool GetTLSServerEndPointChannelBinding(
 // The certificate is signed by the private key in |key|. The key length and
 // signature algorithm may be updated periodically to match best practices.
 //
-// |subject| is a distinguished name defined in RFC4514 with _only_ a CN
-// component, as in:
-//   CN=Michael Wong
+// |subject| is a distinguished name defined in RFC4514.
 //
 // SECURITY WARNING
 //

net/cert/x509_util_unittest.cc

@@ -30,11 +30,8 @@ TEST(X509UtilTest, CreateKeyAndSelfSigned) {
 
   std::string der_cert;
   ASSERT_TRUE(x509_util::CreateKeyAndSelfSignedCert(
-      "CN=subject",
-      1,
-      base::Time::Now(),
-      base::Time::Now() + base::TimeDelta::FromDays(1),
-      &private_key,
+      "CN=subject, OU=org unit, O=org, C=CA", 1, base::Time::Now(),
+      base::Time::Now() + base::TimeDelta::FromDays(1), &private_key,
       &der_cert));
 
   ASSERT_TRUE(private_key.get());
@@ -43,7 +40,10 @@ TEST(X509UtilTest, CreateKeyAndSelfSigned) {
       der_cert.data(), der_cert.size()));
   ASSERT_TRUE(cert.get());
 
-  EXPECT_EQ("subject", cert->subject().GetDisplayName());
+  EXPECT_EQ("subject", cert->subject().common_name);
+  EXPECT_EQ("org unit", cert->subject().organization_unit_names[0]);
+  EXPECT_EQ("org", cert->subject().organization_names[0]);
+  EXPECT_EQ("CA", cert->subject().country_name);
   EXPECT_FALSE(cert->HasExpired());
 }