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

net/socket/ssl_client_socket_nss.cc

 // Copyright (c) 2011 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.
 
 // This file includes code SSLClientSocketNSS::DoVerifyCertComplete() derived
 // from AuthCertificateCallback() in
 // mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp.
 
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
@@ skipping 2119 matching lines @@
     CERTCertList** result_certs,
     void** result_private_key) {
   SSLClientSocketNSS* that = reinterpret_cast<SSLClientSocketNSS*>(arg);
 
   that->client_auth_cert_needed_ = !that->ssl_config_.send_client_cert;
 #if defined(OS_WIN)
   if (that->ssl_config_.send_client_cert) {
     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;
         der_cert.data = cert_context->pbCertEncoded;
         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 =
             that->ssl_config_.client_cert->GetIntermediateCertificates();
         for (X509Certificate::OSCertHandles::const_iterator it =
             intermediates.begin(); it != intermediates.end(); ++it) {
           der_cert.data = (*it)->pbCertEncoded;
           der_cert.len = (*it)->cbCertEncoded;
 
           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.
     return SECFailure;
   }
 
   that->client_certs_.clear();
 
   std::vector<CERT_NAME_BLOB> issuer_list(ca_names->nnames);
   for (int i = 0; i < ca_names->nnames; ++i) {
@@ skipping 94 matching lines @@
           that->ssl_config_.client_cert->CreateClientCertificateChain();
       if (chain) {
         identity = reinterpret_cast<SecIdentityRef>(
             const_cast<void*>(CFArrayGetValueAtIndex(chain, 0)));
       }
       if (identity)
         os_error = SecIdentityCopyPrivateKey(identity, &private_key);
 
       if (chain && identity && os_error == noErr) {
         // TODO(rsleevi): Error checking for NSS allocation errors.
         *result_certs = CERT_NewCertList();

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

Should the same restructuring added in the Windows implementation be added here?
Not setting |*result_certs| or |*result_private_key| until the very end (lines
296-297)

[wtc, 2011/09/26 18:26:07]

rsleevi: ("lines 296-297" is a typo).  The Mac code uses
a different strategy: set |*result_certs| and |*result_private_key|
first, and clean them up on failure (lines 2352-2359).

So the Mac code should not need more changes.

         *result_private_key = private_key;
 
         for (CFIndex i = 0; i < CFArrayGetCount(chain); ++i) {
           CSSM_DATA cert_data;
           SecCertificateRef cert_ref;
           if (i == 0) {
             cert_ref = that->ssl_config_.client_cert->os_cert_handle();
           } else {
             cert_ref = reinterpret_cast<SecCertificateRef>(
                 const_cast<void*>(CFArrayGetValueAtIndex(chain, i)));
           }
           os_error = SecCertificateGetData(cert_ref, &cert_data);
           if (os_error != noErr)
             break;
 
           SECItem der_cert;
           der_cert.type = siDERCertBuffer;
           der_cert.data = cert_data.Data;
           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);
         }
       }
       if (os_error == noErr) {
         CFRelease(chain);
         return SECSuccess;
       }
       LOG(WARNING) << "Client cert found, but could not be used: "
                    << os_error;
       if (*result_certs) {
@@ skipping 165 matching lines @@
   valid_thread_id_ = base::PlatformThread::CurrentId();
 }
 
 bool SSLClientSocketNSS::CalledOnValidThread() const {
   EnsureThreadIdAssigned();
   base::AutoLock auto_lock(lock_);
   return valid_thread_id_ == base::PlatformThread::CurrentId();
 }
 
 }  // namespace net