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

On Windows, prefer to generate SHA-1 signatures for TLS 1.2 client
authentication if the client private key is in a CAPI service provider.

R=rsleevi@chromium.org,agl@chromium.org
BUG=278370
TEST=will require manual testing by the bug reporter.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220595

[wtc, 2013-08-28 00:04:45]

Notes on the CL:

1. There are two possible approaches to support a non-SHA256
hash function in TLS 1.2 client auth signatures:
* Buffer handshake messages
* Compute all supported hashes

I chose the latter approach in this CL to minimize the diffs.
Intuitively the hashing cost should be only slightly more
expensive than the hashing cost in TLS 1.0 and 1.1, which
computes both MD5 and SHA1 hashes.

2. Although a more general version of this CL would be
suitable for NSS upstream, I think a TLS 1.2 server is
unlikely to not support SHA-256 client auth signatures.
So I wrote this CL for Chromium only. This allows me to
ignore the PKCS #11 bypass code paths.

3. To minimize ifdefs, most of the code in this CL is
outside #ifdef _WIN32. This should add minimal overhead
on other platforms because the ss->ssl3.hs.md5 null check
is cheap.

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

LGTM, although it's missing a patch file.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:3942: PK11_DestroyContext(ss->ssl3.hs.sha,
PR_TRUE);
Is destroying hs.sha needed here? It's not done in the case on line 3950 or
3934.

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

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:4071: } else {
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.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6097: rv =
ssl3_ComputeBackupHandshakeHashes(ss, &hashes);
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?

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6159: * messages. */
This comment seems inaccurate now.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:7062: * assume we have an RSA client key. */
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;
}

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:7315: }
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:44]

Thank you for the reviews. Please review patch set 2.

1. I added a patch file.

2. I now test whether the CAPI service provider supports
SHA-256. This allows us to use SHA-256 signatures more often.

Please let me know if you prefer the original conservative
but simpler approach.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:3942: PK11_DestroyContext(ss->ssl3.hs.sha,
PR_TRUE);
On 2013/08/28 15:15:08, agl wrote:
> Is destroying hs.sha needed here? It's not done in the case on line 3950 or
> 3934.

Done.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:4071: } else {

On 2013/08/28 20:40:09, Ryan Sleevi wrote:
> Does it make sense to collapse the if on line 4058/4071 into the if on 4064

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.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6097: rv =
ssl3_ComputeBackupHandshakeHashes(ss, &hashes);

On 2013/08/28 20:40:09, Ryan Sleevi wrote:
> 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?

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.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:6159: * messages. */

On 2013/08/28 20:40:09, Ryan Sleevi wrote:
> This comment seems inaccurate now.

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.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:7062: * assume we have an RSA client key. */

On 2013/08/28 20:40:09, Ryan Sleevi wrote:
> 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;
> }

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.

https://codereview.chromium.org/23545010/diff/1/net/third_party/nss/ssl/ssl3c...
net/third_party/nss/ssl/ssl3con.c:7315: }

On 2013/08/28 20:40:09, Ryan Sleevi wrote:
> I think it's very weird to do the cleanup in this function. Could you explain
> why here, versus the 'normal' path?

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.

https://codereview.chromium.org/23545010/diff/28001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23545010/diff/28001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7062: while (CryptGetProvParam(prov,
PP_ENUMALGS,

It may be cheaper to check SHA-256 support by trying to create
a SHA-256 hash object in the CAPI service provider...

[agl, 2013-08-28 22:16:02]

LGTM

https://codereview.chromium.org/23545010/diff/28001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/23545010/diff/28001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:6100: rv =
ssl3_ComputeBackupHandshakeHashes(ss, &hashes);
(doesn't matter): here you are using tabs rather than 8 spaces at the beginning
of the line (which I believe is NSS style), but codereview isn't showing that in
other parts of the file.

[wtc, 2013-08-29 21:32:02]

I just uploaded patch set 3. The users experiencing this
problem sent me three different error codes for the client
auth signature failure. So I think the conservative approach,
which doesn't try to detect if the client private key can
sign SHA-256 hashes, is more likely to fix all variants of this problem.

[Ryan Sleevi, 2013-08-29 21:33:32]

lgtm

[commit-bot: I haz the power, 2013-08-29 23:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/23545010/27001

[commit-bot: I haz the power, 2013-08-30 03:31:54]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-30 14:51:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/23545010/27001

[commit-bot: I haz the power, 2013-08-30 15:52:20]

Change committed as 220595