On Windows, prepare to generate SHA-1 signatures for TLS 1.2 client authentication.

net/third_party/nss/ssl/ssl3con.c

 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */
 
@@ skipping 3915 matching lines @@
             if (ss->ssl3.hs.sha == NULL) {
                 ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
                 return SECFailure;
             }
             ss->ssl3.hs.hashType = handshake_hash_single;
 
             if (PK11_DigestBegin(ss->ssl3.hs.sha) != SECSuccess) {
                 ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
                 return SECFailure;
             }
+
+#ifdef _WIN32
+            /* A backup SHA-1 hash for a potential client auth signature. */
+            if (!ss->sec.isServer) {
+                ss->ssl3.hs.md5 = PK11_CreateDigestContext(SEC_OID_SHA1);
+                if (ss->ssl3.hs.md5 == NULL) {
+                    PK11_DestroyContext(ss->ssl3.hs.sha, PR_TRUE);

[agl, 2013/08/28 15:15:08]

Is destroying hs.sha needed here? It's not done in the case on line 3950 or
3934.

[wtc, 2013/08/28 22:05:45]

Done.

+                    ss->ssl3.hs.sha = NULL;
+                    ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
+                    return SECFailure;
+                }
+
+                if (PK11_DigestBegin(ss->ssl3.hs.md5) != SECSuccess) {
+                    ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
+                    return SECFailure;
+                }
+            }
+#endif
         } else {
             /* Both ss->ssl3.hs.md5 and ss->ssl3.hs.sha should be NULL or
              * created successfully. */
             ss->ssl3.hs.md5 = PK11_CreateDigestContext(SEC_OID_MD5);
             if (ss->ssl3.hs.md5 == NULL) {
                 ssl_MapLowLevelError(SSL_ERROR_MD5_DIGEST_FAILURE);
                 return SECFailure;
             }
             ss->ssl3.hs.sha = PK11_CreateDigestContext(SEC_OID_SHA1);
             if (ss->ssl3.hs.sha == NULL) {
@@ skipping 90 matching lines @@
         }
         return rv;
     }
 #endif
     if (ss->ssl3.hs.hashType == handshake_hash_single) {
         rv = PK11_DigestOp(ss->ssl3.hs.sha, b, l);
         if (rv != SECSuccess) {
             ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
             return rv;
         }
+        if (ss->ssl3.hs.md5) {
+            rv = PK11_DigestOp(ss->ssl3.hs.md5, b, l);
+            if (rv != SECSuccess) {
+                ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
+                return rv;
+            }
+        }
     } else {

[Ryan Sleevi, 2013/08/28 20:40:09]

Does it make sense to collapse the if on line 4058/4071 into the if on 4064

That is, at this point, both branches are updating sha1 unconditionally. md5 is
updated conditionally. The only differences are the error codes.

[wtc, 2013/08/28 22:05:45]

Yes. The code can be further simplified if we only report the new
error code SSL_ERROR_DIGEST_FAILURE. I assume this is what you
suggested?

    rv = PK11_DigestOp(ss->ssl3.hs.sha, b, l);
    if (rv != SECSuccess) {
        ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
        return rv;
    }
    if (ss->ssl3.hs.md5) {
        rv = PK11_DigestOp(ss->ssl3.hs.md5, b, l);
        if (rv != SECSuccess) {
            ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
            return rv;
        }
    } else {
        PORT_Assert(ss->ssl3.hs.hashType == handshake_hash_single);
    }

I didn't do it because I wanted to maintain this patch as a
Chromium-only patch. We can consider making this change to the NSS
upstream directly.

         rv = PK11_DigestOp(ss->ssl3.hs.md5, b, l);
         if (rv != SECSuccess) {
             ssl_MapLowLevelError(SSL_ERROR_MD5_DIGEST_FAILURE);
             return rv;
         }
         rv = PK11_DigestOp(ss->ssl3.hs.sha, b, l);
         if (rv != SECSuccess) {
             ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
             return rv;
         }
@@ skipping 727 matching lines @@
                 rv = SECFailure;
             }
             if (shaStateBuf != shaStackBuf) {
                 PORT_ZFree(shaStateBuf, shaStateLen);
             }
         }
     }
     return rv;
 }
 
+static SECStatus
+ssl3_ComputeBackupHandshakeHashes(sslSocket * ss,
+                                  SSL3Hashes * hashes) /* output goes here. */
+{
+    SECStatus rv = SECSuccess;
+
+    PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
+    PORT_Assert( ss->ssl3.hs.hashType == handshake_hash_single );
+
+    rv = PK11_DigestFinal(ss->ssl3.hs.md5, hashes->u.raw, &hashes->len,
+                          sizeof(hashes->u.raw));
+    if (rv != SECSuccess) {
+        ssl_MapLowLevelError(SSL_ERROR_SHA_DIGEST_FAILURE);
+        rv = SECFailure;
+        goto loser;
+    }
+    hashes->hashAlg = SEC_OID_SHA1;
+
+loser:
+    PK11_DestroyContext(ss->ssl3.hs.md5, PR_TRUE);
+    ss->ssl3.hs.md5 = NULL;
+    return rv;
+}
+
 /*
  * SSL 2 based implementations pass in the initial outbound buffer
  * so that the handshake hash can contain the included information.
  *
  * Called from ssl2_BeginClientHandshake() in sslcon.c
  */
 SECStatus
 ssl3_StartHandshakeHash(sslSocket *ss, unsigned char * buf, int length)
 {
     SECStatus rv;
@@ skipping 1233 matching lines @@
     unsigned int  len;
     SSL3SignatureAndHashAlgorithm sigAndHash;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveXmitBufLock(ss));
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
     SSL_TRC(3, ("%d: SSL3[%d]: send certificate_verify handshake",
                 SSL_GETPID(), ss->fd));
 
     ssl_GetSpecReadLock(ss);
-    rv = ssl3_ComputeHandshakeHashes(ss, ss->ssl3.pwSpec, &hashes, 0);
+    if (ss->ssl3.hs.hashType == handshake_hash_single && ss->ssl3.hs.md5) {
+        rv = ssl3_ComputeBackupHandshakeHashes(ss, &hashes);

[Ryan Sleevi, 2013/08/28 20:40:09]

Given that ssl3_ComputeBackupHandshakeHashes has access to ss here, wouldn't it
make more sense to move this logic into that, and eliminate the added
ssl3_ComputeBackupHandshakeHashes?

[wtc, 2013/08/28 22:05:45]

That was my original plan. I didn't do that for two reasons.

1. The backup handshake hash is only used at this call site. In
the other call sites (when we send or handle a Finished message),
we only use the handshake hash (of the PRF).

2. ssl3_ComputeHandshakeHashes is a very complicated function. I
wanted to make it easy to review my change, so I created a new
function for it.

+        PORT_Assert(ss->ssl3.hs.md5 == NULL);
+    } else {
+        rv = ssl3_ComputeHandshakeHashes(ss, ss->ssl3.pwSpec, &hashes, 0);
+    }
     ssl_ReleaseSpecReadLock(ss);
     if (rv != SECSuccess) {
         goto done;      /* err code was set by ssl3_ComputeHandshakeHashes */
     }
 
     isTLS = (PRBool)(ss->ssl3.pwSpec->version > SSL_LIBRARY_VERSION_3_0);
     isTLS12 = (PRBool)(ss->ssl3.pwSpec->version >= SSL_LIBRARY_VERSION_TLS_1_2);
     if (ss->ssl3.platformClientKey) {
 #ifdef NSS_PLATFORM_CLIENT_AUTH
         keyType = CERT_GetCertKeyType(
@@ skipping 37 matching lines @@
     if (isTLS12) {
         rv = ssl3_TLSSignatureAlgorithmForKeyType(keyType,
                                                   &sigAndHash.sigAlg);
         if (rv != SECSuccess) {
             goto done;
         }
         /* We always sign using the handshake hash function. It's possible that
          * a server could support SHA-256 as the handshake hash but not as a
          * signature hash. In that case we wouldn't be able to do client
          * certificates with it. The alternative is to buffer all handshake
          * messages. */

[Ryan Sleevi, 2013/08/28 20:40:09]

This comment seems inaccurate now.

[wtc, 2013/08/28 22:05:45]

Thanks. This is also not the best place for this comment. I updated
the comment and moved it before the ssl3_ComputeHandshakeHashes call
at the beginning of this function.

         sigAndHash.hashAlg = hashes.hashAlg;
 
         rv = ssl3_AppendSignatureAndHashAlgorithm(ss, &sigAndHash);
         if (rv != SECSuccess) {
             goto done;  /* err set by AppendHandshake. */
         }
     }
     rv = ssl3_AppendHandshakeVariable(ss, buf.data, buf.len, 2);
     if (rv != SECSuccess) {
         goto done;      /* error code set by AppendHandshake */
@@ skipping 871 matching lines @@
                 if (ss->ssl3.clientCertificate != NULL) {
                     CERT_DestroyCertificate(ss->ssl3.clientCertificate);
                     ss->ssl3.clientCertificate = NULL;
                 }
                 if (ss->ssl3.platformClientKey) {
                     ssl_FreePlatformKey(ss->ssl3.platformClientKey);
                     ss->ssl3.platformClientKey = (PlatformKey)NULL;
                 }
                 goto send_no_certificate;
             }
+
+            if (isTLS12 && ss->ssl3.hs.md5) {
+                PRBool need_backup_hash = PR_FALSE;
+#ifdef _WIN32
+                /* If the key is in CAPI, assume conservatively that the CAPI
+                 * service provider may be unable to sign SHA-256 hashes.
+                 * Use SHA-1 if the server supports it. */
+                if (ss->ssl3.platformClientKey->dwKeySpec !=
+                    CERT_NCRYPT_KEY_SPEC) {
+                    /* CAPI only supports RSA and DSA signatures. Ideally we
+                     * should get the key type. Since DSA client keys are rare,
+                     * assume we have an RSA client key. */

[Ryan Sleevi, 2013/08/28 20:40:09]

Why not also support DSA?

if (algorithms.data[i] == tls_hash_sha1 &&
    (algorithms.data[i+1] == tls_sig_rsa ||
     algorithms.data[i+1] == tls_sig_dsa)) {
  need_backup_hash = PR_TRUE;
  break;
}

[wtc, 2013/08/28 22:05:45]

Originally I thought I'd need to call
        keyType = CERT_GetCertKeyType(
            &ss->ssl3.clientCertificate->subjectPublicKeyInfo);
to get the key type, and then test either
    algorithms.data[i+1] == tls_sig_rsa
or
    algorithms.data[i+1] == tls_sig_dsa
based on the key type. I wanted to avoid the cost of the
CERT_GetCertKeyType call.

But your suggested change should be fine. It is as good as checking
for tls_sig_rsa only.

+                    for (i = 0; i < algorithms.len; i += 2) {
+                        if (algorithms.data[i] == tls_hash_sha1 &&
+                            algorithms.data[i + 1] == tls_sig_rsa) {
+                            need_backup_hash = PR_TRUE;
+                            break;
+                        }
+                    }
+                }
+#endif  /* _WIN32 */
+                if (!need_backup_hash) {
+                    PK11_DestroyContext(ss->ssl3.hs.md5, PR_TRUE);
+                    ss->ssl3.hs.md5 = NULL;
+                }
+            }
             break;  /* not an error */
         }
 #endif   /* NSS_PLATFORM_CLIENT_AUTH */
         /* check what the callback function returned */
         if ((!ss->ssl3.clientCertificate) || (!ss->ssl3.clientPrivateKey)) {
             /* we are missing either the key or cert */
             if (ss->ssl3.clientCertificate) {
                 /* got a cert, but no key - free it */
                 CERT_DestroyCertificate(ss->ssl3.clientCertificate);
                 ss->ssl3.clientCertificate = NULL;
@@ skipping 213 matching lines @@
     PRBool sendClientCert;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
     PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
 
     sendClientCert = !ss->ssl3.sendEmptyCert &&
                      ss->ssl3.clientCertChain  != NULL &&
                      (ss->ssl3.platformClientKey ||
                      ss->ssl3.clientPrivateKey != NULL);
 
+    if (!sendClientCert &&
+        ss->ssl3.hs.hashType == handshake_hash_single && ss->ssl3.hs.md5) {
+        /* Don't need the backup handshake hash. */
+        PK11_DestroyContext(ss->ssl3.hs.md5, PR_TRUE);
+        ss->ssl3.hs.md5 = NULL;
+    }

[Ryan Sleevi, 2013/08/28 20:40:09]

I think it's very weird to do the cleanup in this function. Could you explain
why here, versus the 'normal' path?

[wtc, 2013/08/28 22:05:45]

This is to stop the backup SHA-1 hashing as soon as possible.

ssl3_SendClientSecondRound is only called by ssl3_HandleServerHelloDone.
CertificateRequest is sent immediately before ServerHelloDone.
So, when we receive ServerHelloDone, we know whether we need to send
a client certificate. If we don't need to send a client certificate,
then we can stop the backup SHA-1 hashing.

Note: at this point the ServerHelloDone message is already hashed, so
the saving in hashing only applies to ClientKeyExchange and Finished.

+
     /* We must wait for the server's certificate to be authenticated before
      * sending the client certificate in order to disclosing the client
      * certificate to an attacker that does not have a valid cert for the
      * domain we are connecting to.
      *
      * XXX: We should do the same for the NPN extension, but for that we
      * need an option to give the application the ability to leak the NPN
      * information to get better performance.
      *
      * During the initial handshake on a connection, we never send/receive
@@ skipping 5024 matching lines @@
             PORT_Free(ss->ssl3.hs.recvdFragments.buf);
         }
     }
 
     ss->ssl3.initialized = PR_FALSE;
 
     SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
 }
 
 /* End of ssl3con.c */