net: fix crash when failing to import a client-side cert into NSS.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 879689ad6dd149d58a656858cf558d424074efd1..56597b135ed248bcd3e6e10e36b5c600f422dc32 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -2137,24 +2137,19 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
     if (that->ssl_config_.client_cert) {
       PCCERT_CONTEXT cert_context =
           that->ssl_config_.client_cert->os_cert_handle();
-      PCERT_KEY_CONTEXT key_context = reinterpret_cast<PCERT_KEY_CONTEXT>(
-          PORT_ZAlloc(sizeof(CERT_KEY_CONTEXT)));
-      if (!key_context)
-        return SECFailure;
-      key_context->cbSize = sizeof(*key_context);
 
+      HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProv = INVALID_HANDLE;
+      DWORD dwKeySpec = 0;

[wtc, 2011/09/23 19:53:34]

I haven't seen the INVALID_HANDLE macro before.  Since
HCRYPTPROV_OR_NCRYPT_KEY_HANDLE is ULONG_PTR, which is
an integer type, you can simply initialize it to 0.
At least verify INVALID_HANDLE has the value 0.

Nit: we should follow the Google C++ naming convention and
name these variables crypt_prov and key_spec.

       BOOL must_free = FALSE;
       BOOL acquired_key = CryptAcquireCertificatePrivateKey(
           cert_context, CRYPT_ACQUIRE_CACHE_FLAG, NULL,
-          &key_context->hCryptProv, &key_context->dwKeySpec, &must_free);
-      if (acquired_key && key_context->hCryptProv) {
-        DCHECK_NE(key_context->dwKeySpec, CERT_NCRYPT_KEY_SPEC);
+          &hCryptProv, &dwKeySpec, &must_free);
+      // Since we passed CRYPT_ACQUIRE_CACHE_FLAG, |must_free| must be false
+      // according to the MSDN documentation.
+      DCHECK(must_free == FALSE);

[wtc, 2011/09/23 19:53:34]

You can use DCHECK_EQ here.  cpplint probably will suggest
this.

Can you change this to CHECK?  (I confirmed MSDN says
this is true.)  Also, it seems that we should only assert
this when the function succeeds.

Similarly, I believe we can also assert hCryptProv is
non-NULL when the function succeeds, and remove the
hCryptProv != INVALID_HANDLE test below.  rsleevi likes
very defensive programming (i.e., he doesn't trust the
OS library's documentation), so I suspect the
hCryptProv != INVALID_HANDLE test is an instance of
very defensive programming.

[Ryan Sleevi, 2011/09/23 22:33:50]

Double checked my notes from disassembling this function when looking into
http://crbug.com/71928 and I agree, it should be fine to CHECK here. The code
responsible for updating |must_free| is solely in Windows - not in CSP control -
so I trust that. It's only when Windows passes off to CSPs to do work that the
distrust of MSDN docs comes in.

 
-        // The certificate cache may have been updated/used, in which case,
-        // duplicate the existing handle, since NSS will free it when no
-        // longer in use.
-        if (!must_free)
-          CryptContextAddRef(key_context->hCryptProv, NULL, 0);
+      if (acquired_key && hCryptProv != INVALID_HANDLE) {
+        DCHECK_NE(dwKeySpec, CERT_NCRYPT_KEY_SPEC);
 
         SECItem der_cert;
         der_cert.type = siDERCertBuffer;
@@ -2162,11 +2157,16 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
         der_cert.len  = cert_context->cbCertEncoded;
 
         // TODO(rsleevi): Error checking for NSS allocation errors.
-        *result_certs = CERT_NewCertList();
         CERTCertDBHandle* db_handle = CERT_GetDefaultCertDB();
         CERTCertificate* user_cert = CERT_NewTempCertificate(
             db_handle, &der_cert, NULL, PR_FALSE, PR_TRUE);
-        CERT_AddCertToListTail(*result_certs, user_cert);
+        if (!user_cert) {
+          // Importing the certificate can fail for reasons including a serial
+          // number collision. See crbug.com/97355.
+          return SECFailure;
+        }
+        CERTCertList* cert_chain = CERT_NewCertList();
+        CERT_AddCertToListTail(cert_chain, user_cert);
 
         // Add the intermediates.
         X509Certificate::OSCertHandles intermediates =
@@ -2178,12 +2178,26 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
 
           CERTCertificate* intermediate = CERT_NewTempCertificate(
               db_handle, &der_cert, NULL, PR_FALSE, PR_TRUE);
-          CERT_AddCertToListTail(*result_certs, intermediate);
+          if (!intermediate) {
+            CERT_DestroyCertList(cert_chain);
+            return SECFailure;
+          }
+          CERT_AddCertToListTail(cert_chain, intermediate);
         }
+        PCERT_KEY_CONTEXT key_context = reinterpret_cast<PCERT_KEY_CONTEXT>(
+            PORT_ZAlloc(sizeof(CERT_KEY_CONTEXT));
+        key_context->cbSize = sizeof(*key_context);
+        // NSS will free this context when no longer in use, but the
+        // |must_free| result from CryptAcquireCertificatePrivateKey was false
+        // so we increment the refcount to negate NSS's future decrement.
+        CryptContextAddRef(hCryptProv, NULL, 0);
+        key_context->hCryptProv = hCryptProv;
+        key_context->dwKeySpec = dwKeySpec;
         *result_private_key = key_context;
+        *result_certs = cert_chain;
+
         return SECSuccess;
       }
-      PORT_Free(key_context);
       LOG(WARNING) << "Client cert found without private key";
     }
     // Send no client certificate.
@@ -2320,6 +2334,12 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
           der_cert.len = cert_data.Length;
           CERTCertificate* nss_cert = CERT_NewTempCertificate(
               CERT_GetDefaultCertDB(), &der_cert, NULL, PR_FALSE, PR_TRUE);
+          if (!nss_cert) {
+            // In the event of an NSS error we make up an OS error and reuse
+            // the error handling, below.
+            os_error = errKCCreateChainFailed;
+            break;
+          }
           CERT_AddCertToListTail(*result_certs, nss_cert);
         }
       }