When performing SSL client authentication on Windows via NSS, change the returned key type to use a PCERT_KEY_CONTEXT allocated via NSS's PORT_Alloc(), rather than an HCRYPTPROV, for native client certificate authentication.

There are two reasons for doing this; first, a PCERT_KEY_CONTEXT lets us transmit a dwKeySpec, indicating whether to use the AT_KEYEXCHANGE or AT_SIGNATURE key for CryptoAPI keys. Second, a small piece of syntactic fluff, a PCERT_KEY_CONTEXT easily supports CNG keys for Vista+, which though not presently supported, is a TODO.

R=wtc
BUG=37560, 71748
TEST=Perform SSL client auth on Windows.

Review URL: http://codereview.chromium.org/4670004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@73913 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 42 insertions, 77 deletions

diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index b84651f..603583c 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -2147,69 +2147,26 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
     if (that->ssl_config_.client_cert) {
       PCCERT_CONTEXT cert_context =
           that->ssl_config_.client_cert->os_cert_handle();
-      if (VLOG_IS_ON(1)) {
-        do {
-          DWORD size_needed = 0;
-          BOOL got_info = CertGetCertificateContextProperty(
-              cert_context, CERT_KEY_PROV_INFO_PROP_ID, NULL, &size_needed);
-          if (!got_info) {
-            VLOG(1) << "Failed to get key prov info size " << GetLastError();
-            break;
-          }
-          std::vector<BYTE> raw_info(size_needed);
-          got_info = CertGetCertificateContextProperty(
-              cert_context, CERT_KEY_PROV_INFO_PROP_ID, &raw_info[0],
-              &size_needed);
-          if (!got_info) {
-            VLOG(1) << "Failed to get key prov info " << GetLastError();
-            break;
-          }
-          PCRYPT_KEY_PROV_INFO info =
-              reinterpret_cast<PCRYPT_KEY_PROV_INFO>(&raw_info[0]);
-          VLOG(1) << "Container Name: " << info->pwszContainerName
-                  << "\nProvider Name: " << info->pwszProvName
-                  << "\nProvider Type: " << info->dwProvType
-                  << "\nFlags: " << info->dwFlags
-                  << "\nProvider Param Count: " << info->cProvParam
-                  << "\nKey Specifier: " << info->dwKeySpec;
-        } while (false);
-
-        do {
-          DWORD size_needed = 0;
-          BOOL got_identifier = CertGetCertificateContextProperty(
-              cert_context, CERT_KEY_IDENTIFIER_PROP_ID, NULL, &size_needed);
-          if (!got_identifier) {
-            VLOG(1) << "Failed to get key identifier size "
-                    << GetLastError();
-            break;
-          }
-          std::vector<BYTE> raw_id(size_needed);
-          got_identifier = CertGetCertificateContextProperty(
-              cert_context, CERT_KEY_IDENTIFIER_PROP_ID, &raw_id[0],
-              &size_needed);
-          if (!got_identifier) {
-            VLOG(1) << "Failed to get key identifier " << GetLastError();
-            break;
-          }
-          VLOG(1) << "Key Identifier: " << base::HexEncode(&raw_id[0],
-                                                           size_needed);
-        } while (false);
-      }
-      HCRYPTPROV provider = NULL;
-      DWORD key_spec = AT_KEYEXCHANGE;
+      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);
+
       BOOL must_free = FALSE;
       BOOL acquired_key = CryptAcquireCertificatePrivateKey(
           cert_context,
           CRYPT_ACQUIRE_CACHE_FLAG | CRYPT_ACQUIRE_COMPARE_KEY_FLAG,
-          NULL, &provider, &key_spec, &must_free);
-      if (acquired_key && provider) {
-        DCHECK_NE(key_spec, CERT_NCRYPT_KEY_SPEC);
+          NULL, &key_context->hCryptProv, &key_context->dwKeySpec,
+          &must_free);
+      if (acquired_key && key_context->hCryptProv) {
+        DCHECK_NE(key_context->dwKeySpec, CERT_NCRYPT_KEY_SPEC);
 
         // 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(provider, NULL, 0);
+          CryptContextAddRef(key_context->hCryptProv, NULL, 0);
 
         SECItem der_cert;
         der_cert.type = siDERCertBuffer;
@@ -2235,10 +2192,10 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
               db_handle, &der_cert, NULL, PR_FALSE, PR_TRUE);
           CERT_AddCertToListTail(*result_certs, intermediate);
         }
-        // TODO(wtc): |key_spec| should be passed along with |provider|.
-        *result_private_key = reinterpret_cast<void*>(provider);
+        *result_private_key = key_context;
         return SECSuccess;
       }
+      PORT_Free(key_context);
       LOG(WARNING) << "Client cert found without private key";
     }
     // Send no client certificate.
@@ -2355,7 +2312,7 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
       if (chain && identity && os_error == noErr) {
         // TODO(rsleevi): Error checking for NSS allocation errors.
         *result_certs = CERT_NewCertList();
-        *result_private_key = reinterpret_cast<void*>(private_key);
+        *result_private_key = private_key;
 
         for (CFIndex i = 0; i < CFArrayGetCount(chain); ++i) {
           CSSM_DATA cert_data;
diff --git a/net/third_party/nss/ssl/ssl.h b/net/third_party/nss/ssl/ssl.h
index 3515007..21d7c8d 100644
--- a/net/third_party/nss/ssl/ssl.h
+++ b/net/third_party/nss/ssl/ssl.h
@@ -351,8 +351,12 @@ SSL_IMPORT SECStatus SSL_GetClientAuthDataHook(PRFileDesc *fd,
  *		    the client cert, and any following being used for chain
  *		    building
  *	pRetKey - pointer to native key pointer, for return of key
- *          - Windows: pointer to HCRYPTPROV
- *          - Mac OS X: pointer to SecKeyRef
+ *          - Windows: A pointer to a PCERT_KEY_CONTEXT that was allocated
+ *                     via PORT_Alloc(). Ownership of the PCERT_KEY_CONTEXT
+ *                     is transferred to NSS, which will free via
+ *                     PORT_Free().
+ *          - Mac OS X: A pointer to a SecKeyRef. Ownership is
+ *                      transferred to NSS, which will free via CFRelease().
  */
 typedef SECStatus (PR_CALLBACK *SSLGetPlatformClientAuthData)(void *arg,
                                 PRFileDesc *fd,
diff --git a/net/third_party/nss/ssl/sslimpl.h b/net/third_party/nss/ssl/sslimpl.h
index c656f65..1ea82da 100644
--- a/net/third_party/nss/ssl/sslimpl.h
+++ b/net/third_party/nss/ssl/sslimpl.h
@@ -854,7 +854,7 @@ const ssl3CipherSuiteDef *suite_def;
 
 #ifdef NSS_PLATFORM_CLIENT_AUTH
 #if defined(XP_WIN32)
-typedef HCRYPTPROV PlatformKey;
+typedef PCERT_KEY_CONTEXT PlatformKey;
 #elif defined(XP_MACOSX)
 typedef SecKeyRef PlatformKey;
 #else
diff --git a/net/third_party/nss/ssl/sslplatf.c b/net/third_party/nss/ssl/sslplatf.c
index 3b2ffb48..213feee 100644
--- a/net/third_party/nss/ssl/sslplatf.c
+++ b/net/third_party/nss/ssl/sslplatf.c
@@ -101,7 +101,12 @@ loser:
 void
 ssl_FreePlatformKey(PlatformKey key)
 {
-    CryptReleaseContext(key, 0);
+    if (key) {
+        if (key->dwKeySpec != CERT_NCRYPT_KEY_SPEC)
+            CryptReleaseContext(key->hCryptProv, 0);
+        /* FIXME(rsleevi): Close CNG keys. */
+        PORT_Free(key);
+    }
 }
 
 void
@@ -148,28 +153,32 @@ ssl_GetPlatformAuthInfoForKey(PlatformKey key,
 {
     DWORD bytesNeeded = 0;
     ssl_InitPlatformAuthInfo(info);
+    if (!key || key->dwKeySpec == CERT_NCRYPT_KEY_SPEC)
+        goto error;
+
     bytesNeeded = sizeof(info->provType);
-    if (!CryptGetProvParam(key, PP_PROVTYPE, (BYTE*)&info->provType,
-                           &bytesNeeded, 0))
+    if (!CryptGetProvParam(key->hCryptProv, PP_PROVTYPE,
+                           (BYTE*)&info->provType, &bytesNeeded, 0))
         goto error;
 
     bytesNeeded = 0;
-    if (!CryptGetProvParam(key, PP_CONTAINER, NULL, &bytesNeeded, 0))
+    if (!CryptGetProvParam(key->hCryptProv, PP_CONTAINER, NULL, &bytesNeeded,
+                           0))
         goto error;
     info->container = (char*)PORT_Alloc(bytesNeeded);
     if (info->container == NULL)
         goto error;
-    if (!CryptGetProvParam(key, PP_CONTAINER, (BYTE*)info->container,
-                           &bytesNeeded, 0))
+    if (!CryptGetProvParam(key->hCryptProv, PP_CONTAINER,
+                           (BYTE*)info->container, &bytesNeeded, 0))
         goto error;
 
     bytesNeeded = 0;
-    if (!CryptGetProvParam(key, PP_NAME, NULL, &bytesNeeded, 0))
+    if (!CryptGetProvParam(key->hCryptProv, PP_NAME, NULL, &bytesNeeded, 0))
         goto error;
     info->provider = (char*)PORT_Alloc(bytesNeeded);
     if (info->provider == NULL)
         goto error;
-    if (!CryptGetProvParam(key, PP_NAME, (BYTE*)info->provider,
+    if (!CryptGetProvParam(key->hCryptProv, PP_NAME, (BYTE*)info->provider,
                            &bytesNeeded, 0))
         goto error;
 
@@ -188,10 +197,6 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
     SECStatus    rv             = SECFailure;
     PRBool       doDerEncode    = PR_FALSE;
     SECItem      hashItem;
-    /* TODO(rsleevi): Should AT_SIGNATURE also be checked if doing client
-     * auth? 
-     */
-    DWORD        keySpec        = AT_KEYEXCHANGE;
     HCRYPTKEY    hKey           = 0;
     DWORD        argLen         = 0;
     ALG_ID       keyAlg         = 0;
@@ -202,7 +207,7 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
     unsigned int i              = 0;
 
     buf->data = NULL;
-    if (!CryptGetUserKey(key, keySpec, &hKey)) {
+    if (!CryptGetUserKey(key->hCryptProv, key->dwKeySpec, &hKey)) {
         if (GetLastError() == NTE_NO_KEY) {
             PORT_SetError(SEC_ERROR_NO_KEY);
         } else {
@@ -225,7 +230,6 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
             hashItem.len  = sizeof(SSL3Hashes);
             break;
         case CALG_DSS_SIGN:
-        /* TODO: Support CALG_ECDSA once tested */
         case CALG_ECDSA:
             if (keyAlg == CALG_ECDSA) {
                 doDerEncode = PR_TRUE;
@@ -242,7 +246,7 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
     }
     PRINT_BUF(60, (NULL, "hash(es) to be signed", hashItem.data, hashItem.len));
 
-    if (!CryptCreateHash(key, hashAlg, 0, 0, &hHash)) {
+    if (!CryptCreateHash(key->hCryptProv, hashAlg, 0, 0, &hHash)) {
         PORT_SetError(SSL_ERROR_SIGN_HASHES_FAILURE);
         goto done;    
     }
@@ -259,7 +263,7 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
         PORT_SetError(SSL_ERROR_SIGN_HASHES_FAILURE);
         goto done;
     }
-    if (!CryptSignHash(hHash, keySpec, NULL, CRYPT_NOHASHOID,
+    if (!CryptSignHash(hHash, key->dwKeySpec, NULL, CRYPT_NOHASHOID,
                        NULL, &signatureLen) || signatureLen == 0) {
         PORT_SetError(SSL_ERROR_SIGN_HASHES_FAILURE);
         goto done;
@@ -268,7 +272,7 @@ ssl3_PlatformSignHashes(SSL3Hashes *hash, PlatformKey key, SECItem *buf,
     if (!buf->data)
         goto done;    /* error code was set. */
 
-    if (!CryptSignHash(hHash, keySpec, NULL, CRYPT_NOHASHOID,
+    if (!CryptSignHash(hHash, key->dwKeySpec, NULL, CRYPT_NOHASHOID,
                        (BYTE*)buf->data, &signatureLen)) {
         PORT_SetError(SSL_ERROR_SIGN_HASHES_FAILURE);
         goto done;