915408 - [Client side] TLS 1.2 client authentication needs to support SHA-1 signatures

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Maintain a backup handshake hash (SHA-1) for potential TLS 1.2 client authentication

Our current TLS 1.2 implementation has a limitation that the signatures used
in client authentication must use the same hash function used in the TLS 1.2 PRF,
which is SHA-256 by default.

We have run into TLS 1.2 clients that cannot sign SHA-256 hashes. Here are
some examples:

1. An older version of the Estonian ID card has 1024-bit RSA keys and cannot
sign SHA-256 hashes. (The problem seems to be that a DigestInfo containing a
SHA-256 hash cannot fit in the APDU used to talk to the card.)

2. An old DSA implementation is likely to require the hash value be 20 bytes
because in FIPS 186-2 and older, DSA can only be used with SHA-1.

3. On Windows, if a private key is in a CAPI service provider, the service
provider may not be the newer provider type that supports AES and SHA-2.

So, on the server side, we need to accept SHA-1 signatures for client auth.
On the client side, we need to determine when we should generate SHA-1
signatures for client auth.

Implementation options:

1. To support multiple hash functions in client auth, we can either buffer all
the handshake messages, or hash the handshake messages using the most commonly
used hash functions.

The attached patch hashes the handshake messages using both the PRF hash
function and SHA-1, because in TLS 1.0 and 1.1 we are already hashing the
handshake messages using two hash functions. This is fine for now, but could
be inconvenient when we support the PRF based on SHA-384.

2. On the client side, which hash function to use can be determined by either
NSS itself or by the ss->getClientAuthData callback function. If the latter,
we will need to add a new version of getClientAuthData that returns the
selected signature and hash algorithms.

It seems better to let NSS determine this, otherwise every application is
likely to need to duplicate the same logic in its ss->getClientAuthData
callback.

The attached patch is incomplete -- it is client side only, and it is missing
the PKCS #11 bypass mode. But it may be a good idea to check it in first as a
partial solution to this problem.

Comment 1

[Wan-Teh Chang]

Attachment: Remove unnecessary null checks in ssl3_HandleCertificateRequest

ss->ssl3.clientCertificate and ss->ssl3.clientPrivateKey cannot be NULL at
that point because we just checked them for null earlier.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It seems like part of the problem is that we hash the data and then separately sign the hash. If we were to do the hashing and signing using a single PKCS#11 mechanism like CKM_SHA1_RSA_PKCS or CKM_SHA256_RSA_PKCS, then in theory we should be able to query the PKCS#11 implementation to ask it which of those mechanisms it supports using the PKCS#11 metadata functions like C_GetMechanismList and C_GetMechanismInfo. If the implementation claims to support CKM_SHA256_RSA_PKCS (for example), then we should definitely feel comfortable using a SHA256-RSA signature. However, I've heard that PKCS#11 implementations tend to not implement uncommon features correctly and so maybe such queries would not be reliable. IMO, ideally we should try as hard as possible to use stronger hash algorithms so that we can help servers deprecate SHA1.

I think the approach of storing all the handshake records without hashing them, and then hashing them once we've decided on what algorithm to support, makes more sense than the preemptive potentially-extra hashing for the reasons you suggested, and also because the extra hashing is almost always unnecessary. Also, the memory needed to store ClientHello...ServerHelloDone is relatively small in most cases and never should be huge.

However, I think that the implementation you attached is a reasonable course of action in the short term, especially since client authentication is relatively uncommon. I will review the patch on Thursday.

Comment 3

[Wan-Teh Chang]

Attachment: Maintain a backup handshake hash (SHA-1) for potential TLS 1.2 client authentication, v2

Change ssl3_DestroyBackupHandshakeHashIfNotNeeded to do nothing if PKCS #11
bypass mode is enabled.

Comment 4

[Robert Relyea]

> It seems like part of the problem is that we hash the data and then separately sign the hash. If we were to do 
> the hashing and signing using a single PKCS#11 mechanism like CKM_SHA1_RSA_PKCS or CKM_SHA256_RSA_PKCS.

This would reduce compatibility. Almost no smart cards support the combined hash functions, which is why NSS never uses them.

> 1. An older version of the Estonian ID card has 1024-bit RSA keys and cannot
> sign SHA-256 hashes. (The problem seems to be that a DigestInfo containing a
> SHA-256 hash cannot fit in the APDU used to talk to the card.)

That's bizarre, almost all smart card take raw RSA operations. The PKCS #1 formatting is usually done in the PKCS #11 modules. I wonder what's in the command that make 256 bit hashes not fit in the APDU. A raw 1024-bit RSA operation fits in the APDU. Do we know if the card support CKM_RSA_X_509. We could fall back to that and format the block ourselves.

> 2. An old DSA implementation is likely to require the hash value be 20 bytes
> because in FIPS 186-2 and older, DSA can only be used with SHA-1.

We can code around this problem. If we have to. DSA signs using the higher hash sizes by truncation, so if we truncate the hash before we send it, we wind up with mathematically the same signature. I did something similiar to this at Red Hat for testing with certain ECC implementations (I did the reverse and padded a SHA-1 signature out to
a SHA-256 signature because the token was refusing to use a SHA-1 signature).

> 3. On Windows, if a private key is in a CAPI service provider, the service
> provider may not be the newer provider type that supports AES and SHA-2.

I'm not sure where AES comes in (since this is a private key op). IIRC, the CAPI PCKS #11 module only gets keys from the windows store (so we don't get confused by tokens installed in both PKCS #11 and CAPI). That means only unpatched Windows XP or earlier has the problem. Do we have any statistics on how many of those are still out there (particularly since CAs are now signing certificates with SHA-256).


I think point 1 is the only difficult issue. I'm not against making our code support a different hash for signing versus prf, but we should make sure we aren't adding a lot of code for some old tokens which are going to be replaced in 12 months.

bob

Comment 5

[Robert Relyea]

BTW, do we know other servers support different hashes for client auth and tls prf?

Comment 6

[Wan-Teh Chang]

Bob: OpenSSL servers support all the hash functions for client auth, including {md5, rsa}.
OpenSSL achieves that by buffering handshake messages.

OpenSSL also supports different hashes for TLS PRF, at least SHA-384 and the default SHA-256.
The SHA-384-based PRF is required by AES_256_GCM cipher suites, which several users asked about.

Comment 7

[Robert Relyea]

So that means clients could be sending either hash type, which we then need to deal with.

Also, are you saying SHA-384 is required even for AES_256_GCM. We're hosed then, because we only have 256.

bob

Comment 8

[Wan-Teh Chang]

There is a US government PIV card and an older version of the Estonian ID card
that can only sign SHA-1 hashes on Windows 7. When IE uses one of these cards
for TLS 1.2 client auth, it will generate SHA-1 signatures. So our servers need
to support SHA-1 signatures for client auth.

Yes, all AES_256_GCM cipher suites are specified to use the SHA-384 PRF. This
is why NSS doesn't support AES_256_GCM cipher suites right now.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 804025 [details] [diff] [review]
Maintain a backup handshake hash (SHA-1) for potential TLS 1.2 client authentication, v2

Review of attachment 804025 [details] [diff] [review]:
-----------------------------------------------------------------

We should modify the ssl coverage tests for TLS 1.2 so that we are exercising both code paths--the case where we sign using the backup hash, and the case where we sign with the normal hash.

::: lib/ssl/ssl3con.c
@@ +3842,5 @@
>  		ssl_MapLowLevelError(SSL_ERROR_DIGEST_FAILURE);
>  		return SECFailure;
>  	    }
> +
> +	    /* A backup SHA-1 hash for a potential client auth signature. */

I think the following comment is better here than the original place you put it:

/* In TLS 1.2, ssl3_ComputeHandshakeHashes always uses the handshake hash
 * function (SHA-256). If the server or the client does not support SHA-256
 * as a signature hash, we can either maintain a backup SHA-1 handshake
 * hash or buffer all handshake messages.
 */

This way, it is at the beginning in both document order and execution order.

@@ +3844,5 @@
>  	    }
> +
> +	    /* A backup SHA-1 hash for a potential client auth signature. */
> +	    if (!ss->sec.isServer) {
> +		ss->ssl3.hs.md5 = PK11_CreateDigestContext(SEC_OID_SHA1);

It might be clearer to do something like:

    #define backupSha1Hash md5

So that all this code uses "ss->ssl3.hs.backupHash" instead. It is hard for my brain to accept storing this SHA-1 hash in a variable names "md5" as I read every line of this patch.

@@ +3845,5 @@
> +
> +	    /* 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) {

s/ == NULL// to be consistent with other checks in the patch.

@@ +4727,5 @@
> +{
> +    SECStatus rv = SECSuccess;
> +
> +    PORT_Assert( ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss) );
> +    PORT_Assert( ss->ssl3.hs.hashType == handshake_hash_single );

PORT_Assert( !ss->isServer );

@@ +5997,5 @@
> +    /* In TLS 1.2, ssl3_ComputeHandshakeHashes always uses the handshake hash
> +     * function (SHA-256). If the server or the client does not support SHA-256
> +     * as a signature hash, we can either maintain a backup SHA-1 handshake
> +     * hash or buffer all handshake messages.
> +     */

As I noted above, this comment would be better in ssl3_InitHandshakeHashes.

@@ +6000,5 @@
> +     * hash or buffer all handshake messages.
> +     */
> +    if (ss->ssl3.hs.hashType == handshake_hash_single && ss->ssl3.hs.md5) {
> +	rv = ssl3_ComputeBackupHandshakeHashes(ss, &hashes);
> +	PORT_Assert(ss->ssl3.hs.md5 == NULL);

s/ == NULL//

@@ +6716,5 @@
> +static PRBool
> +ssl3_ClientKeyPrefersSHA1(sslSocket *ss)
> +{
> +    SECKEYPublicKey *pubk;
> +    PRBool prefer_sha1 = PR_FALSE;

nit: normally we would name this preferSHA1.

@@ +6740,5 @@
> +static void
> +ssl3_DestroyBackupHandshakeHashIfNotNeeded(sslSocket *ss,
> +					   const SECItem *algorithms)
> +{
> +    PRBool need_backup_hash = PR_FALSE;

nit: needBackupHash.

@@ +6751,5 @@
> +	return;
> +    }
> +#endif
> +    PORT_Assert(ss->ssl3.hs.md5);
> +    if (ssl3_ClientKeyPrefersSHA1(ss)) {

No need to change this since we're planning to undo this patch in favor of the approach of buffering the handshake messages soon. But, normally it would be better to figure out if the server accepts SHA-1 first and then call ssl3_ClientKeyPrefersSHA1, since ssl3_ClientKeyPrefersSHA1 is the more expensive operation.

@@ +6935,5 @@
>  		ss->ssl3.clientPrivateKey = NULL;
>  	    }
>  	    goto send_no_certificate;
>  	}
> +	if (isTLS12) {

Seems like (if ss->ssl3.hs.hashType == handshake_hash_single) would be a better check since that is the check you use elsewhere.

Comment 10

[Wan-Teh Chang]

Comment on attachment 803306 [details] [diff] [review]
Remove unnecessary null checks in ssl3_HandleCertificateRequest

Patch checked in: https://hg.mozilla.org/projects/nss/rev/cabde5f68135

Comment 11

[Wan-Teh Chang]

Comment on attachment 804025 [details] [diff] [review]
Maintain a backup handshake hash (SHA-1) for potential TLS 1.2 client authentication, v2

Review of attachment 804025 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the code review. I made all the changes you suggested.

::: lib/ssl/ssl3con.c
@@ +3845,5 @@
> +
> +	    /* 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) {

I made this change elsewhere but not in this function because this
function uses the == NULL form.

Comment 12

[Wan-Teh Chang]

Attachment: Maintain a backup handshake hash (SHA-1) for potential TLS 1.2 client authentication, v3

Patch checked in: https://hg.mozilla.org/projects/nss/rev/e3a50f120ac7

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Wan-Teh, should we mark this FIXED? If not, what additional work is needed? Can we do that additional work in another bug?

Comment 14

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 15

[David Benjamin]

Attachment: backuphash-nss.patch

Here's a followup patch to also take server preferences into account when picking SHA-1 vs. SHA-256, otherwise we'll incorrectly send a SHA-256 handshake to servers which request only SHA-1 in CertificateVerify. The one server I know which does this is https://insidemit-apps.mit.edu, but that's hard for other people to test against without MIT credentials.

Tested by enabling TLS 1.2 in Firefox, swapping out the copy of libssl3.so with the one I built, and looking at a packet dump when talking to that server.

Comment 16

[Wan-Teh Chang]

Comment on attachment 8343800 [details] [diff] [review]
backuphash-nss.patch

Review of attachment 8343800 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/727e4a8bfe94

Comment 17

[Wan-Teh Chang]

I filed bug 948064 for the additional work needed to fix this bug
completely.

Updated the bug's summary to reflect what was fixed in NSS 3.15.4.