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..0caa47f9eba45ee2b2ae101a082e0ada733ffeda 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -2166,6 +2166,13 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
         CERTCertDBHandle* db_handle = CERT_GetDefaultCertDB();
         CERTCertificate* user_cert = CERT_NewTempCertificate(
             db_handle, &der_cert, NULL, PR_FALSE, PR_TRUE);
+        if (!user_cert) {
+          // Importing the certificate can fail for reasons including a serial
+          // number collision. See crbug.com/97355.
+          CERT_DestroyCertList(*result_certs);
+          *result_certs = NULL;
+          return SECFailure;

[mattm, 2011/09/22 23:19:21]

Oh, these two cases also need to (maybe) decref the hCryptProv and free the
key_context?

[wtc, 2011/09/23 00:55:55]

mattm is right.  Perhaps we should do the error cleanup in
a more systematic way, or we can build the cert list first,
and then build key_context.

[agl, 2011/09/23 17:52:22]

Indeed, the state to be cleaned up is much more complex than I first noticed.

I've reduced the scope of the key_context so that we don't have to clean it up.
I think I've also dealt with the hCryptProv but that CryptoAPI call is odd. It
appears to return a flag indicating whether the caller needs to free a
refcounted object and I believe, in our case, that's always false. But why
wouldn't it just consistently return a reference in any case? 

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

I don't know.  Perhaps they trade off easy of use for
performance (avoid a CryptReleaseContext call when it's
not necessary).

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

This API was originally introduced and ran on Windows 95. In it's current
implementation, it's just an InterlockedIncrement/InterlockedDecrement. However,
these primitives behave differently on 95 (
http://blogs.msdn.com/b/oldnewthing/archive/2004/05/06/127141.aspx ), so my
guess is that wtc is correct - it's a performance thing.

Now a days it hardly matters now with XADD.

+        }
         CERT_AddCertToListTail(*result_certs, user_cert);
 
         // Add the intermediates.
@@ -2178,6 +2185,11 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
 
           CERTCertificate* intermediate = CERT_NewTempCertificate(
               db_handle, &der_cert, NULL, PR_FALSE, PR_TRUE);
+          if (!intermediate) {
+            CERT_DestroyCertList(*result_certs);
+            *result_certs = NULL;
+            return SECFailure;
+          }
           CERT_AddCertToListTail(*result_certs, intermediate);
         }
         *result_private_key = key_context;
@@ -2320,6 +2332,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);
         }
       }