1274718 - TLS 1.3: Implement async AuthCertificate

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Set "security.tls.version.max" to 4.
2. Type <https://tls13.cloudflare.com/> into the location bar.

Actual result:
Nothing happens. Firefox does not even display an error page. If you open the URL in a new tab, the tab will be completely blank. If you reset "security.tls.version.max". You will see the error page.

Expected result:
Firefox should load the webpage.

I don't know if this is our fault, but I'm filing this anyway.

Comment 1

[Masatoshi Kimura [:emk]]

NSS does not yet support async AuthCertificate for TLS 1.3[1] that Firefox depends on.[2]
[1] https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/security/nss/lib/ssl/ssl3con.c#11372
[2] https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/security/manager/ssl/SSLServerCertVerification.cpp#1601

Comment 2

[Martin Thomson [:mt:]]

Ugh, I forgot to land this one.

Patches (that need cleanup) here:
https://github.com/ekr/gecko-dev/pull/17/files
https://github.com/ekr/gecko-dev/pull/18/files

Comment 3

[Martin Thomson [:mt:]]

Thanks for checking on this, btw.

Comment 4

[Martin Thomson [:mt:]]

Attachment: bug1274718-1.patch

Rietveld: http://codereview.appspot.com/293470043

Do you think that we want to uplift this to 3.24 (and Aurora)?  We can't use TLS 1.3 until this lands in m-c and it's possible to turn it on.

Comment 5

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/971214957947

Comment 6

[Martin Thomson [:mt:]]

Oops: https://hg.mozilla.org/projects/nss/rev/5dda5c8922f3

Comment 7

[Wan-Teh Chang]

Comment on attachment 8755137 [details] [diff] [review]
bug1274718-1.patch

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

I only reviewed the changes to lib/ssl. I have some questions
and suggested changes.

::: lib/ssl/tls13con.c
@@ +2376,2 @@
>       * waiting for server authentication. See the long block
>       * comment in ssl3_SendClientSecondRound for more detail.

That long block comment in ssl3_SendClientSecondRound contains
a discussion of renegotiation. Since renegotiation has been
removed in TLS 1.3, it is confusing to just refer to that block
comment here. We should copy the relevant part of that block
comment.

@@ +2380,5 @@
>          PR_NOT_REACHED("unexpected ss->ssl3.hs.restartTarget");
>          PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
>          return SECFailure;
>      }
> +    if (ss->ssl3.hs.authCertificatePending) {

Can you explain why TLS 1.3 doesn't need to check
sendClientCert || ss->ssl3.sendEmptyCert here?  (I
understand ss->firstHsDone is not applicable to TLS
1.3.)

@@ +2385,1 @@
>          SSL_TRC(3, ("%d: TLS13[%p]: deferring ssl3_SendClientSecondRound because"

Nit: this SSL_TRC message should say "tls13_SendClientSecondRound"
instead of "ssl3_SendClientSecondRound".

Comment 8

[Martin Thomson [:mt:]]

Attachment: bug1274718-1.patch

Thanks for checking Wan-Teh.  You caught a bug.

(In reply to Wan-Teh Chang from comment #7)
> >       * waiting for server authentication. See the long block
> >       * comment in ssl3_SendClientSecondRound for more detail.
> 
> That long block comment in ssl3_SendClientSecondRound contains
> a discussion of renegotiation. Since renegotiation has been
> removed in TLS 1.3, it is confusing to just refer to that block
> comment here. We should copy the relevant part of that block
> comment.

Good idea.  It's the same length anyway.

> > +    if (ss->ssl3.hs.authCertificatePending) {
> 
> Can you explain why TLS 1.3 doesn't need to check
> sendClientCert || ss->ssl3.sendEmptyCert here?  (I
> understand ss->firstHsDone is not applicable to TLS
> 1.3.)

That's a mistake.  It's a harmless one, in the sense that it doesn't affect functionality, but it does slow the handshake down unnecessarily in the common case.  I can't remember why I made that change, so it's best to revert it now, I think.

Comment 10

[Masatoshi Kimura [:emk]]

Reopening so that the follow-up patch doesn't get lost.

Comment 11

[Martin Thomson [:mt:]]

ekr: http://codereview.appspot.com/295540043

Comment 12

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/bace9e5a2cb1

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Sorry, backed out due to ssl_gtests bustage:

ssl_loopback_unittest.cc:1345: Failure
Value of: SSL_AuthCertificateComplete(client->ssl_fd(), 0)
  Actual: -1
Expected: SECSuccess
Which is: 0
[  FAILED  ] SSLv2ClientHelloTestF.Connect13 (68 ms)

https://public-artifacts.taskcluster.net/iZn33_x-QEu3jztmuBicgg/2/public/logs/live_backing.log

https://hg.mozilla.org/projects/nss/rev/98019fcb2a7a

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

The SSLv2 test fails when _all_ ssl_gtests are run, compiling only ssl_v2_client_hello_unittest.cc and running tests seems fine.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

The problem seems to be TlsConnectDatagram13.AuthCompleteAfterFinished:

[       OK ] TlsConnectDatagram13.AuthCompleteAfterFinished (83 ms)
[----------] 2 tests from TlsConnectDatagram13 (220 ms total)

[----------] 3 tests from SSLv2ClientHelloTestF
[ RUN      ] SSLv2ClientHelloTestF.Connect13
Version: TLS (no version)
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
client: Handshake
client: Original packet: [517] 1603010200010001fc0304c8836efb6441e0e34114bcfa43b08b5dbdc5ee62d9d86c56af480d68ebe79a2500000cc02bc02fcca9cca8009eccaa010001c70000000b0009000006736572766572ff01000100000a000a00080017001800190100ff020002000b0028014e014c00170042410401f13bcb3805116b8a6edfeaf455bea52de8e5bceda888b37f33550a33e429a9e3282129266cd437d7e008f2d72a830cf6b8a1a23536a11d6f2e5eb9fc97be6d01000102010035558b63859e61c439317df737d686aae5c12c4ea7fe213a8c92c07c6211fd31d26e8fc83c25a3054e09af4f33dfdde54c98e549ee8061c1d85052a761ee9e2683d12ab8ba24e65d62f056ea280b41855849d8bc12f2317add3534edbe4fbbc733403453cdd15c400e7587a987574a853757764194efe7b18ecfcbb23a353865076c181933ed6d6b2c3c777a62de3b60d89cbbb8fe6ad2d8440d3147566c9df57bf285bc664c9dcc83ea6a249a66a8aff773cb0b32e486ccea587c42dfd462f2713febcdd3e1687e0f6617c100410bf907833d3353fece78b8c294e8c2f98d9c71a6dde089faedbd9c27cb558bfb0d4132f41963b33b17a345cadecfc87e4aef000d00180016040105010601020104030503060302030502040202020015002d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
client: Filtered packet: [30] 801c01030400030000001000c02f0fdb39636b23caf82b79f637ef8ba540
client: Read --> wouldblock 5
client: Would have blocked
server: Handshake
server: Handshake failed with SSL error 28: SSL received a malformed Client Hello handshake message.
server: Changing state from CONNECTING to ERROR
Poll() waiters = 1 timers = 27
client: Readable
client: Handshake
client: Handshake failed with SSL error 62: SSL peer rejected a handshake message for unacceptable content.
client: Changing state from CONNECTING to ERROR
client: call SSL_AuthCertificateComplete
ssl_loopback_unittest.cc:1346: Failure
Value of: SSL_AuthCertificateComplete(client->ssl_fd(), 0)
  Actual: -1
Expected: SECSuccess
Which is: 0
[  FAILED  ] SSLv2ClientHelloTestF.Connect13 (79 ms)

Here the "TlsConnectDatagram13.AuthCompleteAfterFinished" test finishes while "SSLv2ClientHelloTestF.Connect13" is already running.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Without the patch posted here you can see that "TlsConnectDatagram13.AuthCompleteAfterFinished" doesn't finish before SSL_AuthCertificateComplete() was called.

https://public-artifacts.taskcluster.net/BL4oNqywQFW5_Wk5bo10Qw/0/public/logs/live_backing.log

Comment 18

[Martin Thomson [:mt:]]

Based on discussion elsewhere, we decided not to include the fix here.  The performance optimization is basically non-existent, since the server is able to send without receiving the client's Finished and the complexity is therefore unjustified.

I have patched the comment as Wan-Teh suggested:
https://hg.mozilla.org/projects/nss/rev/a50f3b4c1ec1