Merge 122053 - Properly parse UTF8Strings in certificates on Windows.

BUG=114168
TEST=https://www.verisign.co.jp appears correctly regardless of system
locale. Additionally, net_unittests:X509TypesTest* should cover this.

Review URL: https://chromiumcodereview.appspot.com/9358080

TBR=rsleevi@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9414018

git-svn-id: svn://svn.chromium.org/chrome/branches/1025/src@122341 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 182 insertions, 145 deletions

diff --git a/crypto/capi_util.cc b/crypto/capi_util.cc
index 7593f39..2cf1062 100644
--- a/crypto/capi_util.cc
+++ b/crypto/capi_util.cc
@@ -46,4 +46,12 @@ BOOL CryptAcquireContextLocked(HCRYPTPROV* prov,
   return CryptAcquireContext(prov, container, provider, prov_type, flags);
 }
 
+void* WINAPI CryptAlloc(size_t size) {
+  return malloc(size);
+}
+
+void WINAPI CryptFree(void* p) {
+  free(p);
+}
+
 }  // namespace crypto
diff --git a/crypto/capi_util.h b/crypto/capi_util.h
index 3aa681e..1786a24 100644
--- a/crypto/capi_util.h
+++ b/crypto/capi_util.h
@@ -29,6 +29,12 @@ CRYPTO_EXPORT BOOL CryptAcquireContextLocked(HCRYPTPROV* prov,
                                              DWORD prov_type,
                                              DWORD flags);
 
+// Wrappers of malloc and free for CryptoAPI routines that need memory
+// allocators, such as in CRYPT_DECODE_PARA. Such routines require WINAPI
+// calling conventions.
+CRYPTO_EXPORT void* WINAPI CryptAlloc(size_t size);
+CRYPTO_EXPORT void WINAPI CryptFree(void* p);
+
 }  // namespace crypto
 
 #endif  // CRYPTO_CAPI_UTIL_H_
diff --git a/crypto/signature_verifier_win.cc b/crypto/signature_verifier_win.cc
index d294c4d..dfb17a4 100644
--- a/crypto/signature_verifier_win.cc
+++ b/crypto/signature_verifier_win.cc
@@ -5,23 +5,10 @@
 #include "crypto/signature_verifier.h"
 
 #include "base/logging.h"
+#include "crypto/capi_util.h"
 
 #pragma comment(lib, "crypt32.lib")
 
-namespace {
-
-// Wrappers of malloc and free for CRYPT_DECODE_PARA, which requires the
-// WINAPI calling convention.
-void* WINAPI MyCryptAlloc(size_t size) {
-  return malloc(size);
-}
-
-void WINAPI MyCryptFree(void* p) {
-  free(p);
-}
-
-}  // namespace
-
 namespace crypto {
 
 SignatureVerifier::SignatureVerifier() : hash_object_(0), public_key_(0) {
@@ -47,8 +34,8 @@ bool SignatureVerifier::VerifyInit(const uint8* signature_algorithm,
 
   CRYPT_DECODE_PARA decode_para;
   decode_para.cbSize = sizeof(decode_para);
-  decode_para.pfnAlloc = MyCryptAlloc;
-  decode_para.pfnFree = MyCryptFree;
+  decode_para.pfnAlloc = crypto::CryptAlloc;
+  decode_para.pfnFree = crypto::CryptFree;
   CERT_PUBLIC_KEY_INFO* cert_public_key_info = NULL;
   DWORD struct_len = 0;
   BOOL ok;
@@ -66,7 +53,7 @@ bool SignatureVerifier::VerifyInit(const uint8* signature_algorithm,
   ok = CryptImportPublicKeyInfo(provider_,
                                 X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
                                 cert_public_key_info, public_key_.receive());
-  free(cert_public_key_info);
+  crypto::CryptFree(cert_public_key_info);
   if (!ok)
     return false;
 
@@ -89,7 +76,7 @@ bool SignatureVerifier::VerifyInit(const uint8* signature_algorithm,
       hash_alg_id = CALG_SHA1;
     else if (!strcmp(signature_algorithm_id->pszObjId, szOID_RSA_MD5RSA))
       hash_alg_id = CALG_MD5;
-    free(signature_algorithm_id);
+    crypto::CryptFree(signature_algorithm_id);
     DCHECK_NE(static_cast<ALG_ID>(CALG_MD4), hash_alg_id);
     if (hash_alg_id == CALG_MD4)
       return false;  // Unsupported hash algorithm.
diff --git a/net/base/x509_cert_types.h b/net/base/x509_cert_types.h
index 57d785f..d11092d2 100644
--- a/net/base/x509_cert_types.h
+++ b/net/base/x509_cert_types.h
@@ -51,10 +51,12 @@ struct NET_EXPORT CertPrincipal {
   explicit CertPrincipal(const std::string& name);
   ~CertPrincipal();
 
-#if defined(OS_MACOSX)
+#if defined(OS_MACOSX) || defined(OS_WIN)
   // Parses a BER-format DistinguishedName.
   bool ParseDistinguishedName(const void* ber_name_data, size_t length);
+#endif
 
+#if defined(OS_MACOSX)
   // Compare this CertPrincipal with |against|, returning true if they're
   // equal enough to be a possible match. This should NOT be used for any
   // security relevant decisions.
@@ -66,7 +68,7 @@ struct NET_EXPORT CertPrincipal {
   // order: CN, O and OU and returns the first non-empty one found.
   std::string GetDisplayName() const;
 
-  // The different attributes for a principal.  They may be "".
+  // The different attributes for a principal, stored in UTF-8.  They may be "".
   // Note that some of them can have several values.
 
   std::string common_name;
diff --git a/net/base/x509_cert_types_mac_unittest.cc b/net/base/x509_cert_types_unittest.cc
index e4809b0..af1b65b 100644
--- a/net/base/x509_cert_types_mac_unittest.cc
+++ b/net/base/x509_cert_types_unittest.cc
@@ -213,6 +213,7 @@ static const uint8 EntrustDN[] = {
 
 namespace net {
 
+#if defined(OS_MACOSX)
 TEST(X509TypesTest, Matching) {
   CertPrincipal spamco;
   spamco.common_name = "SpamCo Dept. Of Certificization";
@@ -249,6 +250,7 @@ TEST(X509TypesTest, Matching) {
   EXPECT_FALSE(bogus.Matches(spamco));
   EXPECT_FALSE(spamco.Matches(bogus));
 }
+#endif
 
 TEST(X509TypesTest, ParseDNVerisign) {
   CertPrincipal verisign;
diff --git a/net/base/x509_cert_types_win.cc b/net/base/x509_cert_types_win.cc
new file mode 100644
index 0000000..0db63b1
--- /dev/null
+++ b/net/base/x509_cert_types_win.cc
@@ -0,0 +1,139 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/x509_cert_types.h"
+
+#include <windows.h>
+#include <wincrypt.h>
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/string_util.h"
+#include "base/utf_string_conversions.h"
+#include "crypto/capi_util.h"
+
+#pragma comment(lib, "crypt32.lib")
+
+namespace net {
+
+namespace {
+
+// A list of OIDs to decode. Any OID not on this list will be ignored for
+// purposes of parsing.
+const char* kOIDs[] = {
+  szOID_COMMON_NAME,
+  szOID_LOCALITY_NAME,
+  szOID_STATE_OR_PROVINCE_NAME,
+  szOID_COUNTRY_NAME,
+  szOID_STREET_ADDRESS,
+  szOID_ORGANIZATION_NAME,
+  szOID_ORGANIZATIONAL_UNIT_NAME,
+  szOID_DOMAIN_COMPONENT
+};
+
+// Converts the value for |attribute| to an UTF-8 string, storing the result
+// in |value|. Returns false if the string cannot be converted.
+bool GetAttributeValue(PCERT_RDN_ATTR attribute,
+                       std::string* value) {
+  DWORD chars_needed = CertRDNValueToStrW(attribute->dwValueType,
+                                          &attribute->Value, NULL, 0);
+  if (chars_needed == 0)
+    return false;
+  if (chars_needed == 1) {
+    // The value is actually an empty string (chars_needed includes a single
+    // char for a NULL value). Don't bother converting - just clear the
+    // string.
+    value->clear();
+    return true;
+  }
+  std::wstring wide_name;
+  DWORD chars_written = CertRDNValueToStrW(
+      attribute->dwValueType, &attribute->Value,
+      WriteInto(&wide_name, chars_needed), chars_needed);
+  if (chars_written <= 1)
+    return false;
+  wide_name.resize(chars_written - 1);
+  *value = WideToUTF8(wide_name);
+  return true;
+}
+
+// Adds a type+value pair to the appropriate vector from a C array.
+// The array is keyed by the matching OIDs from kOIDS[].
+bool AddTypeValuePair(PCERT_RDN_ATTR attribute,
+                      std::vector<std::string>* values[]) {
+  for (size_t oid = 0; oid < arraysize(kOIDs); ++oid) {
+    if (strcmp(attribute->pszObjId, kOIDs[oid]) == 0) {
+      std::string value;
+      if (!GetAttributeValue(attribute, &value))
+        return false;
+      values[oid]->push_back(value);
+      break;
+    }
+  }
+  return true;
+}
+
+// Stores the first string of the vector, if any, to *single_value.
+void SetSingle(const std::vector<std::string>& values,
+               std::string* single_value) {
+  // We don't expect to have more than one CN, L, S, and C.
+  LOG_IF(WARNING, values.size() > 1) << "Didn't expect multiple values";
+  if (!values.empty())
+    *single_value = values[0];
+}
+
+}  // namespace
+
+bool CertPrincipal::ParseDistinguishedName(const void* ber_name_data,
+                                           size_t length) {
+  DCHECK(ber_name_data);
+
+  CRYPT_DECODE_PARA decode_para;
+  decode_para.cbSize = sizeof(decode_para);
+  decode_para.pfnAlloc = crypto::CryptAlloc;
+  decode_para.pfnFree = crypto::CryptFree;
+  CERT_NAME_INFO* name_info = NULL;
+  DWORD name_info_size = 0;
+  BOOL rv;
+  rv = CryptDecodeObjectEx(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
+                           X509_NAME,
+                           reinterpret_cast<const BYTE*>(ber_name_data),
+                           length,
+                           CRYPT_DECODE_ALLOC_FLAG | CRYPT_DECODE_NOCOPY_FLAG,
+                           &decode_para,
+                           &name_info, &name_info_size);
+  if (!rv)
+    return false;
+  scoped_ptr_malloc<CERT_NAME_INFO> scoped_name_info(name_info);
+
+  std::vector<std::string> common_names, locality_names, state_names,
+      country_names;
+
+  std::vector<std::string>* values[] = {
+      &common_names, &locality_names,
+      &state_names, &country_names,
+      &this->street_addresses,
+      &this->organization_names,
+      &this->organization_unit_names,
+      &this->domain_components
+  };
+  DCHECK(arraysize(kOIDs) == arraysize(values));
+
+  for (DWORD cur_rdn = 0; cur_rdn < name_info->cRDN; ++cur_rdn) {
+    PCERT_RDN rdn = &name_info->rgRDN[cur_rdn];
+    for (DWORD cur_ava = 0; cur_ava < rdn->cRDNAttr; ++cur_ava) {
+      PCERT_RDN_ATTR ava = &rdn->rgRDNAttr[cur_ava];
+      if (!AddTypeValuePair(ava, values))
+        return false;
+    }
+  }
+
+  SetSingle(common_names, &this->common_name);
+  SetSingle(locality_names, &this->locality_name);
+  SetSingle(state_names, &this->state_or_province_name);
+  SetSingle(country_names, &this->country_name);
+  return true;
+}
+
+}  // namespace net
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 527316d..4654211 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -15,6 +15,7 @@
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "crypto/capi_util.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_capi_types.h"
 #include "crypto/sha2.h"
@@ -188,16 +189,6 @@ void ExplodedTimeToSystemTime(const base::Time::Exploded& exploded,
 
 //-----------------------------------------------------------------------------
 
-// Wrappers of malloc and free for CRYPT_DECODE_PARA, which requires the
-// WINAPI calling convention.
-void* WINAPI MyCryptAlloc(size_t size) {
-  return malloc(size);
-}
-
-void WINAPI MyCryptFree(void* p) {
-  free(p);
-}
-
 // Decodes the cert's subjectAltName extension into a CERT_ALT_NAME_INFO
 // structure and stores it in *output.
 void GetCertSubjectAltName(PCCERT_CONTEXT cert,
@@ -210,8 +201,8 @@ void GetCertSubjectAltName(PCCERT_CONTEXT cert,
 
   CRYPT_DECODE_PARA decode_para;
   decode_para.cbSize = sizeof(decode_para);
-  decode_para.pfnAlloc = MyCryptAlloc;
-  decode_para.pfnFree = MyCryptFree;
+  decode_para.pfnAlloc = crypto::CryptAlloc;
+  decode_para.pfnFree = crypto::CryptFree;
   CERT_ALT_NAME_INFO* alt_name_info = NULL;
   DWORD alt_name_info_size = 0;
   BOOL rv;
@@ -232,8 +223,8 @@ void GetCertSubjectAltName(PCCERT_CONTEXT cert,
 bool CertSubjectCommonNameHasNull(PCCERT_CONTEXT cert) {
   CRYPT_DECODE_PARA decode_para;
   decode_para.cbSize = sizeof(decode_para);
-  decode_para.pfnAlloc = MyCryptAlloc;
-  decode_para.pfnFree = MyCryptFree;
+  decode_para.pfnAlloc = crypto::CryptAlloc;
+  decode_para.pfnFree = crypto::CryptFree;
   CERT_NAME_INFO* name_info = NULL;
   DWORD name_info_size = 0;
   BOOL rv;
@@ -393,8 +384,8 @@ void GetCertPoliciesInfo(PCCERT_CONTEXT cert,
 
   CRYPT_DECODE_PARA decode_para;
   decode_para.cbSize = sizeof(decode_para);
-  decode_para.pfnAlloc = MyCryptAlloc;
-  decode_para.pfnFree = MyCryptFree;
+  decode_para.pfnAlloc = crypto::CryptAlloc;
+  decode_para.pfnFree = crypto::CryptFree;
   CERT_POLICIES_INFO* policies_info = NULL;
   DWORD policies_info_size = 0;
   BOOL rv;
@@ -544,119 +535,15 @@ void AppendPublicKeyHashes(PCCERT_CHAIN_CONTEXT chain,
   }
 }
 
-// A list of OIDs to decode. Any OID not on this list will be ignored for
-// purposes of parsing.
-const char* kOIDs[] = {
-  szOID_COMMON_NAME,
-  szOID_LOCALITY_NAME,
-  szOID_STATE_OR_PROVINCE_NAME,
-  szOID_COUNTRY_NAME,
-  szOID_STREET_ADDRESS,
-  szOID_ORGANIZATION_NAME,
-  szOID_ORGANIZATIONAL_UNIT_NAME,
-  szOID_DOMAIN_COMPONENT
-};
-
-// Converts the value for |attribute| to an ASCII string, storing the result
-// in |value|. Returns false if the string cannot be converted.
-bool GetAttributeValue(PCERT_RDN_ATTR attribute,
-                       std::string* value) {
-  DWORD bytes_needed = CertRDNValueToStrA(attribute->dwValueType,
-                                          &attribute->Value, NULL, 0);
-  if (bytes_needed == 0)
-    return false;
-  if (bytes_needed == 1) {
-    // The value is actually an empty string (bytes_needed includes a single
-    // byte for a NULL value). Don't bother converting - just clear the
-    // string.
-    value->clear();
-    return true;
-  }
-  DWORD bytes_written = CertRDNValueToStrA(
-      attribute->dwValueType, &attribute->Value,
-      WriteInto(value, bytes_needed), bytes_needed);
-  if (bytes_written <= 1)
-    return false;
-  return true;
-}
-
-// Adds a type+value pair to the appropriate vector from a C array.
-// The array is keyed by the matching OIDs from kOIDS[].
-bool AddTypeValuePair(PCERT_RDN_ATTR attribute,
-                      std::vector<std::string>* values[]) {
-  for (size_t oid = 0; oid < arraysize(kOIDs); ++oid) {
-    if (strcmp(attribute->pszObjId, kOIDs[oid]) == 0) {
-      std::string value;
-      if (!GetAttributeValue(attribute, &value))
-        return false;
-      values[oid]->push_back(value);
-      break;
-    }
-  }
-  return true;
-}
-
-// Stores the first string of the vector, if any, to *single_value.
-void SetSingle(const std::vector<std::string>& values,
-               std::string* single_value) {
-  // We don't expect to have more than one CN, L, S, and C.
-  LOG_IF(WARNING, values.size() > 1) << "Didn't expect multiple values";
-  if (!values.empty())
-    *single_value = values[0];
-}
-
-bool ParsePrincipal(CERT_NAME_BLOB* name, CertPrincipal* principal) {
-  CRYPT_DECODE_PARA decode_para;
-  decode_para.cbSize = sizeof(decode_para);
-  decode_para.pfnAlloc = MyCryptAlloc;
-  decode_para.pfnFree = MyCryptFree;
-  CERT_NAME_INFO* name_info = NULL;
-  DWORD name_info_size = 0;
-  BOOL rv;
-  rv = CryptDecodeObjectEx(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-                           X509_NAME, name->pbData, name->cbData,
-                           CRYPT_DECODE_ALLOC_FLAG | CRYPT_DECODE_NOCOPY_FLAG,
-                           &decode_para,
-                           &name_info, &name_info_size);
-  if (!rv)
-    return false;
-  scoped_ptr_malloc<CERT_NAME_INFO> scoped_name_info(name_info);
-
-  std::vector<std::string> common_names, locality_names, state_names,
-      country_names;
-
-  std::vector<std::string>* values[] = {
-      &common_names, &locality_names,
-      &state_names, &country_names,
-      &(principal->street_addresses),
-      &(principal->organization_names),
-      &(principal->organization_unit_names),
-      &(principal->domain_components)
-  };
-  DCHECK(arraysize(kOIDs) == arraysize(values));
-
-  for (DWORD cur_rdn = 0; cur_rdn < name_info->cRDN; ++cur_rdn) {
-    PCERT_RDN rdn = &name_info->rgRDN[cur_rdn];
-    for (DWORD cur_ava = 0; cur_ava < rdn->cRDNAttr; ++cur_ava) {
-      PCERT_RDN_ATTR ava = &rdn->rgRDNAttr[cur_ava];
-      if (!AddTypeValuePair(ava, values))
-        return false;
-    }
-  }
-
-  SetSingle(common_names, &principal->common_name);
-  SetSingle(locality_names, &principal->locality_name);
-  SetSingle(state_names, &principal->state_or_province_name);
-  SetSingle(country_names, &principal->country_name);
-  return true;
-}
 
 }  // namespace
 
 void X509Certificate::Initialize() {
   DCHECK(cert_handle_);
-  ParsePrincipal(&cert_handle_->pCertInfo->Subject, &subject_);
-  ParsePrincipal(&cert_handle_->pCertInfo->Issuer, &issuer_);
+  subject_.ParseDistinguishedName(cert_handle_->pCertInfo->Subject.pbData,
+                                  cert_handle_->pCertInfo->Subject.cbData);
+  issuer_.ParseDistinguishedName(cert_handle_->pCertInfo->Issuer.pbData,
+                                 cert_handle_->pCertInfo->Issuer.cbData);
 
   valid_start_ = Time::FromFileTime(cert_handle_->pCertInfo->NotBefore);
   valid_expiry_ = Time::FromFileTime(cert_handle_->pCertInfo->NotAfter);
diff --git a/net/net.gyp b/net/net.gyp
index 6c301a3..4060754 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -251,6 +251,7 @@
         'base/x509_cert_types.cc',
         'base/x509_cert_types.h',
         'base/x509_cert_types_mac.cc',
+        'base/x509_cert_types_win.cc',
         'base/x509_certificate.cc',
         'base/x509_certificate.h',
         'base/x509_certificate_mac.cc',
@@ -1053,7 +1054,7 @@
         'base/upload_data_unittest.cc',
         'base/upload_data_stream_unittest.cc',
         'base/x509_certificate_unittest.cc',
-        'base/x509_cert_types_mac_unittest.cc',
+        'base/x509_cert_types_unittest.cc',
         'base/x509_util_nss_unittest.cc',
         'base/x509_util_openssl_unittest.cc',
         'disk_cache/addr_unittest.cc',
@@ -1297,6 +1298,11 @@
             ],
           },
         ],
+        [ 'OS != "win" and OS != "mac"', {
+          'sources!': [
+            'base/x509_cert_types_unittest.cc',
+          ],
+        }]
       ],
     },
     {