Use X509Certificate printable_string_is_utf8 hack in more ChromeOS client cert code

Enable the X509Certificate printable_string_is_utf8 hack for:
- ClientCertResolver: Parses client certificates to match Issuer/Subject
  patterns
- enterprise.platformKeys.getCertificates API: Parses client
  certificates to get DER representation.

BUG=788655
TEST=chromeos_unittests --gtest_filter=ClientCertResolverTest.*

Change-Id: Iea02c525ed91017ccd1957da2ea0e28597c1bd9f
Reviewed-on: https://chromium-review.googlesource.com/823974
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524262}

chrome/browser/chromeos/platform_keys/platform_keys_nss.cc

@@ -599,8 +599,13 @@ void FilterCertificatesOnWorkerThread(
     if (cert_slot != state->slot_)
       continue;
 
+    // Allow UTF-8 inside PrintableStrings in client certificates. See
+    // crbug.com/770323 and crbug.com/788655.
+    net::X509Certificate::UnsafeCreateOptions options;
+    options.printable_string_is_utf8 = true;
     scoped_refptr<net::X509Certificate> cert =
-        net::x509_util::CreateX509CertificateFromCERTCertificate(cert_handle);
+        net::x509_util::CreateX509CertificateFromCERTCertificate(cert_handle,
+                                                                 {}, options);
     if (!cert)
       continue;
 

chromeos/network/client_cert_resolver.cc

@@ -123,9 +123,13 @@ struct MatchCertWithPattern {
 
   bool operator()(const CertAndIssuer& cert_and_issuer) {
     if (!pattern.issuer().Empty() || !pattern.subject().Empty()) {
+      // Allow UTF-8 inside PrintableStrings in client certificates. See
+      // crbug.com/770323 and crbug.com/788655.
+      net::X509Certificate::UnsafeCreateOptions options;
+      options.printable_string_is_utf8 = true;
       scoped_refptr<net::X509Certificate> x509_cert =
           net::x509_util::CreateX509CertificateFromCERTCertificate(
-              cert_and_issuer.cert.get());
+              cert_and_issuer.cert.get(), {}, options);
       if (!x509_cert)
         return false;
       if (!pattern.issuer().Empty() &&

chromeos/network/client_cert_resolver_unittest.cc

@@ -32,6 +32,7 @@
 #include "crypto/scoped_test_nss_db.h"
 #include "net/base/net_errors.h"
 #include "net/cert/nss_cert_database_chromeos.h"
+#include "net/cert/pem_tokenizer.h"
 #include "net/cert/x509_certificate.h"
 #include "net/cert/x509_util_nss.h"
 #include "net/test/cert_test_util.h"
@@ -43,10 +44,10 @@ namespace chromeos {
 
 namespace {
 
-const char* kWifiStub = "wifi_stub";
-const char* kWifiSSID = "wifi_ssid";
-const char* kUserProfilePath = "user_profile";
-const char* kUserHash = "user_hash";
+constexpr char kWifiStub[] = "wifi_stub";
+constexpr char kWifiSSID[] = "wifi_ssid";
+constexpr char kUserProfilePath[] = "user_profile";
+constexpr char kUserHash[] = "user_hash";
 
 }  // namespace
 
@@ -106,9 +107,10 @@ class ClientCertResolverTest : public testing::Test,
     }
   }
 
-  // Imports a client certificate. Its PKCS#11 ID is stored in |test_cert_id_|.
-  // If |import_issuer| is true, also imports the CA cert (stored as PEM in
-  // test_ca_cert_pem_) that issued the client certificate.
+  // Imports a client certificate. After a subsequent StartCertLoader()
+  // invocation, the PKCS#11 ID of the imported certificate will be stored in
+  // |test_cert_id_|.  If |import_issuer| is true, also imports the CA cert
+  // (stored as PEM in test_ca_cert_pem_) that issued the client certificate.
   void SetupTestCerts(const std::string& prefix, bool import_issuer) {
     // Load a CA cert.
     net::ScopedCERTCertificateList ca_cert_list =
@@ -134,6 +136,35 @@ class ClientCertResolverTest : public testing::Test,
     ASSERT_TRUE(test_client_cert_.get());
   }
 
+  // Imports a client certificate with a subject CommonName encoded as
+  // PrintableString, but containing invalid characters. It is imported into the
+  // user slot. After a subsequent StartCertLoader() invocation, the PKCS#11 ID
+  // of the imported certificate will be stored in |test_cert_id_|.
+  void SetupTestCertWithBadPrintableString() {
+    base::FilePath certs_dir = net::GetTestNetDataDirectory().AppendASCII(
+        "parse_certificate_unittest");
+    ASSERT_TRUE(net::ImportSensitiveKeyFromFile(
+        certs_dir, "v3_certificate_template.pk8", test_nssdb_.slot()));
+
+    std::string file_data;
+    ASSERT_TRUE(base::ReadFileToString(
+        certs_dir.AppendASCII(
+            "subject_printable_string_containing_utf8_client_cert.pem"),
+        &file_data));
+
+    net::PEMTokenizer pem_tokenizer(file_data, {"CERTIFICATE"});
+    ASSERT_TRUE(pem_tokenizer.GetNext());
+    std::string cert_der(pem_tokenizer.data());
+    ASSERT_FALSE(pem_tokenizer.GetNext());
+
+    test_client_cert_ = net::x509_util::CreateCERTCertificateFromBytes(
+        reinterpret_cast<const uint8_t*>(cert_der.data()), cert_der.size());
+    ASSERT_TRUE(test_client_cert_);
+
+    ASSERT_TRUE(net::ImportClientCertToSlot(test_client_cert_.get(),
+                                            test_nssdb_.slot()));
+  }
+
   void SetupTestCertInSystemToken(const std::string& prefix) {
     test_nsscertdb_->SetSystemSlot(
         crypto::ScopedPK11Slot(PK11_ReferenceSlot(test_system_nssdb_.slot())));
@@ -190,9 +221,10 @@ class ClientCertResolverTest : public testing::Test,
   }
 
   // Sets up a policy with a certificate pattern that matches any client cert
-  // with a certain Issuer CN. It will match the test client cert.
+  // with a certain Issuer CN. It will match the test client cert imported by
+  // SetupTestCerts.
   void SetupPolicyMatchingIssuerCN(onc::ONCSource onc_source) {
-    const char* kTestPolicy =
+    const char* test_policy =
         "[ { \"GUID\": \"wifi_stub\","
         "    \"Name\": \"wifi_stub\","
         "    \"Type\": \"WiFi\","
@@ -214,7 +246,47 @@ class ClientCertResolverTest : public testing::Test,
     std::string error;
     std::unique_ptr<base::Value> policy_value =
         base::JSONReader::ReadAndReturnError(
-            kTestPolicy, base::JSON_ALLOW_TRAILING_COMMAS, nullptr, &error);
+            test_policy, base::JSON_ALLOW_TRAILING_COMMAS, nullptr, &error);
+    ASSERT_TRUE(policy_value) << error;
+
+    base::ListValue* policy = nullptr;
+    ASSERT_TRUE(policy_value->GetAsList(&policy));
+
+    std::string user_hash =
+        onc_source == onc::ONC_SOURCE_USER_POLICY ? kUserHash : "";
+    managed_config_handler_->SetPolicy(
+        onc_source, user_hash, *policy,
+        base::DictionaryValue() /* no global network config */);
+  }
+
+  // Sets up a policy with a certificate pattern that matches any client cert
+  // with a Subject Organization set to "Blar". It will match the test client
+  // cert imported by SetupTestCertWithBadPrintableString.
+  void SetupPolicyMatchingSubjectOrgForBadPrintableStringCert(
+      onc::ONCSource onc_source) {
+    const char* test_policy =
+        "[ { \"GUID\": \"wifi_stub\","
+        "    \"Name\": \"wifi_stub\","
+        "    \"Type\": \"WiFi\","
+        "    \"WiFi\": {"
+        "      \"Security\": \"WPA-EAP\","
+        "      \"SSID\": \"wifi_ssid\","
+        "      \"EAP\": {"
+        "        \"Outer\": \"EAP-TLS\","
+        "        \"ClientCertType\": \"Pattern\","
+        "        \"ClientCertPattern\": {"
+        "          \"Subject\": {"
+        "            \"Organization\": \"Blar\""
+        "          }"
+        "        }"
+        "      }"
+        "    }"
+        "} ]";
+
+    std::string error;
+    std::unique_ptr<base::Value> policy_value =
+        base::JSONReader::ReadAndReturnError(
+            test_policy, base::JSON_ALLOW_TRAILING_COMMAS, nullptr, &error);
     ASSERT_TRUE(policy_value) << error;
 
     base::ListValue* policy = nullptr;
@@ -230,7 +302,7 @@ class ClientCertResolverTest : public testing::Test,
   void SetupCertificateConfigMatchingIssuerCN(
       onc::ONCSource onc_source,
       client_cert::ClientCertConfig* client_cert_config) {
-    const char* kTestOncPattern =
+    const char* test_onc_pattern =
         "{"
         "  \"Issuer\": {"
         "    \"CommonName\": \"B CA\""
@@ -238,8 +310,9 @@ class ClientCertResolverTest : public testing::Test,
         "}";
     std::string error;
     std::unique_ptr<base::Value> onc_pattern_value =
-        base::JSONReader::ReadAndReturnError(
-            kTestOncPattern, base::JSON_ALLOW_TRAILING_COMMAS, nullptr, &error);
+        base::JSONReader::ReadAndReturnError(test_onc_pattern,
+                                             base::JSON_ALLOW_TRAILING_COMMAS,
+                                             nullptr, &error);
     ASSERT_TRUE(onc_pattern_value) << error;
 
     base::DictionaryValue* onc_pattern_dict;
@@ -254,7 +327,7 @@ class ClientCertResolverTest : public testing::Test,
   // particular it will match the test client cert.
   void SetupPolicyMatchingIssuerPEM(onc::ONCSource onc_source,
                                     const std::string& identity) {
-    const char* kTestPolicyTemplate =
+    const char* test_policy_template =
         "[ { \"GUID\": \"wifi_stub\","
         "    \"Name\": \"wifi_stub\","
         "    \"Type\": \"WiFi\","
@@ -272,7 +345,7 @@ class ClientCertResolverTest : public testing::Test,
         "    }"
         "} ]";
     std::string policy_json = base::StringPrintf(
-        kTestPolicyTemplate, identity.c_str(), test_ca_cert_pem_.c_str());
+        test_policy_template, identity.c_str(), test_ca_cert_pem_.c_str());
 
     std::string error;
     std::unique_ptr<base::Value> policy_value =
@@ -374,6 +447,32 @@ TEST_F(ClientCertResolverTest, MatchIssuerCNWithoutIssuerInstalled) {
   EXPECT_EQ(1, network_properties_changed_count_);
 }
 
+// Test that matching works on a certificate with invalid characters in a
+// PrintableString field. See crbug.com/788655.
+TEST_F(ClientCertResolverTest, MatchSubjectOrgOnBadPrintableStringCert) {
+  ASSERT_NO_FATAL_FAILURE(SetupTestCertWithBadPrintableString());
+
+  SetupWifi();
+  scoped_task_environment_.RunUntilIdle();
+
+  SetupNetworkHandlers();
+  ASSERT_NO_FATAL_FAILURE(
+      SetupPolicyMatchingSubjectOrgForBadPrintableStringCert(
+          onc::ONC_SOURCE_USER_POLICY));
+  scoped_task_environment_.RunUntilIdle();
+
+  network_properties_changed_count_ = 0;
+  StartCertLoader();
+  scoped_task_environment_.RunUntilIdle();
+
+  // Verify that the resolver positively matched the pattern in the policy with
+  // the test client cert and configured the network.
+  std::string pkcs11_id;
+  GetServiceProperty(shill::kEapCertIdProperty, &pkcs11_id);
+  EXPECT_EQ(test_cert_id_, pkcs11_id);
+  EXPECT_EQ(1, network_properties_changed_count_);
+}
+
 TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) {
   SetupTestCerts("client_1", true /* import issuer */);
   SetupWifi();