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

net/third_party/nss/ssl/ssl3con.c

Index: net/third_party/nss/ssl/ssl3con.c
===================================================================
--- net/third_party/nss/ssl/ssl3con.c	(revision 219612)
+++ net/third_party/nss/ssl/ssl3con.c	(working copy)
@@ -3933,6 +3933,24 @@
 		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. */
@@ -4043,6 +4061,13 @@
 	    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) {
@@ -4791,6 +4816,30 @@
     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.
@@ -6044,7 +6093,12 @@
 		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 */
@@ -6994,6 +7048,32 @@
 		}
 		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 */
@@ -7227,6 +7307,13 @@
 		     (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