1057463 - (tls13) Implement TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Eric Rescorla (:ekr)]

This is a meta-bug for implementing TLS 1.3

Comment 1

[Gervase Markham [:gerv]]

Can this bug now be resolved?

Gerv

Comment 2

[Eric Rescorla (:ekr)]

No.

Comment 3

[Eric Rescorla (:ekr)]

Experimental 1-RTT implementation landed at https://hg.mozilla.org/projects/nss/rev/2716b2d7fe80

Comment 4

[Eric Rescorla (:ekr)]

Reviewed on Rietveld: https://codereview.appspot.com/276360043

Comment 5

[Eric Rescorla (:ekr)]

Landed anti-downgrade code at:
https://hg.mozilla.org/projects/nss/rev/887332f51c8d

Reviewed at: 
https://codereview.appspot.com/289900043/

Comment 6

[Martin Thomson [:mt:]]

We should probably have separate bugs for separate things, like the anti-downgrade stuff (too late for that now).

Comment 7

[Kai Engert [:KaiE:]]

It would be nice to have commit URLs added to bugs after they are landed (we don't have that fancy pulsebot automation for NSPR/NSS).

https://hg.mozilla.org/projects/nss/rev/2716b2d7fe80 seems to have broken the build, because it adds unused variables.

tls13con.c: In function ‘tls13_SetHsState’:
tls13con.c:153:17: error: unused variable ‘new_state_name’ [-Werror=unused-variable]
     const char *new_state_name = tls13_HandshakeState(ws);
                 ^
tls13con.c: In function ‘tls13_DeriveTrafficKeys’:
tls13con.c:1015:24: error: unused variable ‘n’ [-Werror=unused-variable]
     EXPAND_TRAFFIC_KEY(kHkdfPurposeClientWriteKey, client.write_key);
                        ^
tls13con.c:1016:24: error: unused variable ‘n’ [-Werror=unused-variable]
     EXPAND_TRAFFIC_KEY(kHkdfPurposeServerWriteKey, server.write_key);
                        ^
tls13con.c:1017:24: error: unused variable ‘n’ [-Werror=unused-variable]
     EXPAND_TRAFFIC_IV(kHkdfPurposeClientWriteIv, client.write_iv);
                        ^
tls13con.c:1018:24: error: unused variable ‘n’ [-Werror=unused-variable]
     EXPAND_TRAFFIC_IV(kHkdfPurposeServerWriteIv, server.write_iv);
                        ^

Comment 8

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #7)
> It would be nice to have commit URLs added to bugs after they are landed (we
> don't have that fancy pulsebot automation for NSPR/NSS).

In c3


> https://hg.mozilla.org/projects/nss/rev/2716b2d7fe80 seems to have broken
> the build, because it adds unused variables.
> 
> tls13con.c: In function ‘tls13_SetHsState’:
> tls13con.c:153:17: error: unused variable ‘new_state_name’
> [-Werror=unused-variable]
>      const char *new_state_name = tls13_HandshakeState(ws);
>                  ^
> tls13con.c: In function ‘tls13_DeriveTrafficKeys’:
> tls13con.c:1015:24: error: unused variable ‘n’ [-Werror=unused-variable]
>      EXPAND_TRAFFIC_KEY(kHkdfPurposeClientWriteKey, client.write_key);
>                         ^
> tls13con.c:1016:24: error: unused variable ‘n’ [-Werror=unused-variable]
>      EXPAND_TRAFFIC_KEY(kHkdfPurposeServerWriteKey, server.write_key);
>                         ^
> tls13con.c:1017:24: error: unused variable ‘n’ [-Werror=unused-variable]
>      EXPAND_TRAFFIC_IV(kHkdfPurposeClientWriteIv, client.write_iv);
>                         ^
> tls13con.c:1018:24: error: unused variable ‘n’ [-Werror=unused-variable]
>      EXPAND_TRAFFIC_IV(kHkdfPurposeServerWriteIv, server.write_iv);

Yes, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1245341

Now that I have r+/f+ I am going to land it.

Comment 9

[Kai Engert [:KaiE:]]

Fails to build on Windows:
https://bot.nss-crypto.org:8011/builders/1-win-x32-DBG/builds/621/steps/shell_1/logs/stdio

Comment 10

[Kai Engert [:KaiE:]]

d:/slavedir/1-win-x32-DBG/hg/nss/external_tests/ssl_gtest/tls_hkdf_unittest.cc(124) : error C2057: expected constant expression
d:/slavedir/1-win-x32-DBG/hg/nss/external_tests/ssl_gtest/tls_hkdf_unittest.cc(124) : error C2466: cannot allocate an array of constant size 0
d:/slavedir/1-win-x32-DBG/hg/nss/external_tests/ssl_gtest/tls_hkdf_unittest.cc(124) : error C2133: 'output' : unknown size
d:/slavedir/1-win-x32-DBG/hg/nss/external_tests/ssl_gtest/tls_hkdf_unittest.cc(129) : error C2070: 'uint8_t []': illegal sizeof operand
d:/slavedir/1-win-x32-DBG/hg/nss/external_tests/ssl_gtest/tls_hkdf_unittest.cc(131) : error C2070: 'uint8_t []': illegal sizeof operand

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1057463-Fix-tls_hkdf_unittest.cc-build-failures-.patch

That should fix the tls_hkdf_unittest.cc build failures on Windows.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8716249 [details] [diff] [review]
0001-Bug-1057463-Fix-tls_hkdf_unittest.cc-build-failures-.patch

I think Kai could review this too.

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 8716249 [details] [diff] [review]
0001-Bug-1057463-Fix-tls_hkdf_unittest.cc-build-failures-.patch

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

LGTM

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8716249 [details] [diff] [review]
0001-Bug-1057463-Fix-tls_hkdf_unittest.cc-build-failures-.patch

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

Comment 15

[Wan-Teh Chang]

Comment on attachment 8716249 [details] [diff] [review]
0001-Bug-1057463-Fix-tls_hkdf_unittest.cc-build-failures-.patch

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

::: external_tests/ssl_gtest/tls_hkdf_unittest.cc
@@ +120,5 @@
>    void HkdfExpandLabel(ScopedPK11SymKey* prk, SSLHashType base_hash,
>                         const uint8_t *session_hash, size_t session_hash_len,
>                         const char *label, size_t label_len,
>                         const DataBuffer& expected) {
> +    std::vector<uint8_t> output(expected.len());

Hmm... this seems to mean Visual C++ 2013 doesn't support
a plain array whose size is initialized by a variable.
(That is a C99 feature.)

@@ +125,5 @@
>  
>      SECStatus rv = tls13_HkdfExpandLabelRaw(prk->get(), base_hash,
>                                              session_hash, session_hash_len,
>                                              label, label_len,
> +                                            &output[0], output.size());

Optional: In C++11, we can use output.data() instead of &output[0].

Comment 16

[ggramaiz]

We should add a dependence on #973755, as the TLS 1.3 current draft references TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 and TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 as "should implement" cipher suites.

[1] https://tools.ietf.org/html/draft-ietf-tls-tls13-11

Comment 17

[Kai Engert [:KaiE:]]

I found an unnecessary assertion, after this change:
https://hg.mozilla.org/projects/nss/rev/2716b2d7fe80#l15.470

We now have

  if (something && backupHash) {
    PORT_Assert(!backupHash);
    ...

Looks like the assert can be removed.

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #17)
>   if (something && backupHash) {
>     PORT_Assert(!backupHash);
> 
> Looks like the assert can be removed.

Actually, the code seems wrong.
I filed bug 1272443.

Comment 19

[Dennis Clarke]

Seems like this is ongoing work. As of 27 Aug 2018 there is not actual TLS 1.3 support in place. Seem that draft 28 of the protocol spec works but not the final actual protocol spec. The very latest nightly Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0 built from https://hg.mozilla.org/mozilla-central/rev/e4e2245fc1428c33db179ca2f242b922ad988d79 does not work.  Can NOT reach site www.tls13.net which does run the actual TLS 1.3 protocol and has been checked with openssl s_client.  So maybe by September some time.

Comment 20

[Dennis Clarke]

works like a charm for TLS 1.3 on rev/b75561ff5ffec3164338952adfe58620e5e3bc1d

Comment 21

[Dennis Clarke]

I think this can be closed now.
With rev/9419be649effc5bc67eb3d6fce1db46caa7fae7e now there are
no problems seen dealing with Apache webservers running with
OpenSSL 1.1.1b and TLS v1.3 and a variety of ciphers.

--
Dennis Clarke
RISC-V/SPARC/PPC/ARM/CISC
UNIX and Linux spoken
GreyBeard and suspenders optional