1280348 - Server side asserts in TLS 1.2 mode with client auth if PKCS#11 bypass mode is used

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Julien Pierre]

When using an NSS server with PKCS#11 bypass enabled, TLS 1.2, and client auth required, and IE11 as the client with a SHA1/RSA client cert, the server asserts as follows with NSS 3.25 :

#0  0x0000003cbd4325e5 in raise () from /lib64/libc.so.6
#1  0x0000003cbd433dc5 in abort () from /lib64/libc.so.6
#2  0x00007f6fdcef3b93 in PR_Assert (s=0x7f6fdcc6bb6a "0", file=0x7f6fdcc6b878 "ssl3con.c", ln=4028) at ../../../../pr/src/io/prlog.c:553
#3  0x00007f6fdcc292b3 in ssl3_GetHashMechanismByHashType (hashType=ssl_hash_sha1) at ssl3con.c:4028
#4  0x00007f6fdcc2b651 in ssl3_ComputeBypassHandshakeHash (buf=0x7f6fb8035f80 "\001", len=1500, hashAlg=ssl_hash_sha1, hashes=0x7f6fd04728b0)
    at ssl3con.c:5157
#5  0x00007f6fdcc377a2 in ssl3_HandleCertificateVerify (ss=0x7f6fb8021ff0, b=0x7f6fb802abe7 "", length=130, hashes=0x7f6fd04729f0) at ssl3con.c:10571
#6  0x00007f6fdcc3c457 in ssl3_HandlePostHelloHandshakeMessage (ss=0x7f6fb8021ff0, b=0x7f6fb802abe5 "\002\001", length=132, hashesPtr=0x7f6fd04729f0)
    at ssl3con.c:12750
#7  0x00007f6fdcc3c089 in ssl3_HandleHandshakeMessage (ss=0x7f6fb8021ff0, b=0x7f6fb802abe5 "\002\001", length=132, endOfRecord=1) at ssl3con.c:12657
#8  0x00007f6fdcc3c774 in ssl3_HandleHandshake (ss=0x7f6fb8021ff0, origBuf=0x7f6fb8022228) at ssl3con.c:12838
#9  0x00007f6fdcc3ddde in ssl3_HandleRecord (ss=0x7f6fb8021ff0, cText=0x7f6fd0472b90, databuf=0x7f6fb8022228) at ssl3con.c:13528
#10 0x00007f6fdcc3ff45 in ssl3_GatherCompleteHandshake (ss=0x7f6fb8021ff0, flags=0) at ssl3gthr.c:480
#11 0x00007f6fdcc40b9e in ssl_GatherRecord1stHandshake (ss=0x7f6fb8021ff0) at sslcon.c:81
#12 0x00007f6fdcc4997d in ssl_Do1stHandshake (ss=0x7f6fb8021ff0) at sslsecur.c:70
#13 0x00007f6fdcc4b8c5 in ssl_SecureRecv (ss=0x7f6fb8021ff0, buf=0x7f6fb80009b8 "", len=8191, flags=0) at sslsecur.c:856
#14 0x00007f6fdcc55acc in ssl_Recv (fd=0x7f6fa4008d70, buf=0x7f6fb80009b8, len=8191, flags=0, timeout=20000) at sslsock.c:2698
#15 0x00007f6fdceef994 in PR_Recv (fd=0x7f6fa4008d70, buf=0x7f6fb80009b8, amount=8191, flags=0, timeout=20000) at ../../../../pr/src/io/priometh.c:188

Comment 1

[Julien Pierre]

Attachment: Allow SHA-1 and SHA-512 client certs for PKCS#11 bypass mode

I have tested this patch and it appears to fix the problem.  The server no longer asserts and behaves as intended. It accepts the SHA-1 client cert, and is able to validate it.

ssl3_GetHashMechanismByHashType is only called from 2 places :
1) ssl3_GetPrfHashMechanism
2) ssl3_ComputeBypassHandshakeHash

I believe the change I made is proper for the 2nd usage. Not sure if it is proper for the 1st usage. If we only want to change the 2nd case, I could duplicate ssl3_GetHashMechanismByHashType and move it to the 
#ifndef NO_PKCS11_BYPASS
section.

Comment 2

[Alicja Kario]

I have made few test cases to verify the behaviour of servers handling Certificate Verify messages:

https://github.com/tomato42/tlsfuzzer/blob/bad-certificate-verify/scripts/test-certificate-verify.py
https://github.com/tomato42/tlsfuzzer/blob/bad-certificate-verify/scripts/test-rsa-sigs-on-certificate-verify.py
https://github.com/tomato42/tlsfuzzer/blob/bad-certificate-verify/scripts/test-certificate-verify-malformed-sig.py

unfortunately I'm quite busy this week, so won't be able to test it myself, additionally RHEL does not support bypass mode so any bugs in it won't be caught in our testing.

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8763000 [details] [diff] [review]
Allow SHA-1 and SHA-512 client certs for PKCS#11 bypass mode

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

If this works to address the problem, I think that this is good.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 8763000 [details] [diff] [review]
Allow SHA-1 and SHA-512 client certs for PKCS#11 bypass mode

r=kaie


I wonder if this should get included in NSS 3.25, past RC0.

Comment 5

[Julien Pierre]

Thanks for the reviews. I would very much appreciate it if this went into NSS 3.25 . When is the projected RTM date for 3.25 ?

I do have another case to fix for PKCS#11 bypass with TLS 1.2 client auth which involves client side, but I'm not sure that I can get that other case fixed soon. I will file a separate bug for that.

Comment 6

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/1116d2526646

Comment 7

[Kai Engert [:KaiE:]]

fixed on NSS_3_25_BRANCH
https://hg.mozilla.org/projects/nss/rev/171b72cde25f

Comment 8

[Wan-Teh Chang]

Comment on attachment 8763000 [details] [diff] [review]
Allow SHA-1 and SHA-512 client certs for PKCS#11 bypass mode

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

::: lib/ssl/ssl3con.c
@@ +4026,5 @@
>          case ssl_hash_none:
>              /* ssl_hash_none is for pre-1.2 suites, which use SHA-256. */
>              return CKM_SHA256;
> +        case ssl_hash_sha1:
> +            return CKM_SHA_1;

Are you sure you want to add a case for ssl_hash_sha1?!

Which cipher suites use SHA-1 in its TLS 1.2 PRF?

Comment 9

[Wan-Teh Chang]

Comment on attachment 8763000 [details] [diff] [review]
Allow SHA-1 and SHA-512 client certs for PKCS#11 bypass mode

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

UPDATE: I see that this function has two callers.
The new ssl_hash_sha1 case is appropriate for
ssl3_ComputeBypassHandshakeHash.

::: lib/ssl/ssl3con.c
@@ +4024,5 @@
>              return CKM_SHA384;
>          case ssl_hash_sha256:
>          case ssl_hash_none:
>              /* ssl_hash_none is for pre-1.2 suites, which use SHA-256. */
>              return CKM_SHA256;

The ssl_hash_none case is only correct for one caller of
this function (ssl3_GetPrfHashMechanism). I think more
cleanup should be done to make this code less confusing.

Comment 10

[Julien Pierre]

Wan-Teh,

Your comment is why I asked whether the change was appropriate for the 1st caller.

Reviewing this, none of the cipher suites in RFC 5246 use SHA-1 for the PRF, or SHA-512 for that matter - they all use SHA-256. Cipher suites from RFC 5289 use SHA-384 for the PRF.

So, I think most likely, the new SHA-1 and SHA-512 cases I added will never be triggered from the PRF function call. We could have 2 functions for this, but the extra case statements are likely harmless for the PRF case.