TLSv1.3: Interoperable 1-RTT handshake#2157
TLSv1.3: Interoperable 1-RTT handshake#2157mattcaswell wants to merge 30 commits intoopenssl:masterfrom
Conversation
|
Testing against BoringSSL deployments, I noticed two problems:
The BoringSSL test suite would have caught all of this, but I haven't tried it yet. (Update to the latest revision first. The one you're at implement draft 16, not draft 18.) We've also got enough of a deployment by now you don't need to do anything complicated to test against it:
For the second, perhaps an intermediate step to make the signature algorithms stuff properly negotiated? Teach TLS 1.2 and proto-TLS-1.3 about RSA-PSS, then teach proto-TLS-1.3 that PKCS#1 is forbidden and about ECDSA's hash/curve matching. We actually did that before changing the handshake since it was a nice self-contained change like the record layer. |
|
@davidben Awesome! Thanks for the feedback...I'll work on fixing those issues. |
8005359 to
8ec902b
Compare
8ec902b to
299eadc
Compare
|
Now that #2020 has gone in, I have rebased this and removed the WIP label. This is now ready for review. |
|
Please look at #2052 |
|
FWIW, this worked with NSS first time. Tested:
I tried P-256, x25519 and P-384, which all worked. ECDSA (openssl s_client) fails as expected. Not a lot else to try based on the description. If there is anything you want me to poke at, let me know and I can give it a spin. Code inspection suggests that there are plenty of work left to do, but this is looking pretty promising. |
299eadc to
9b14df5
Compare
|
I rebased this. Also added a couple of commits with tweaks for Travis/Appveyor issues. Let's see how travis/appveyor do. |
In TLSv1.2 an individual sig alg is represented by 1 byte for the hash and 1 byte for the signature. In TLSv1.3 each sig alg is represented by two bytes, where the two bytes together represent a single hash and signature combination. This converts the internal representation of sigalgs to use a single int for the pair, rather than a pair of bytes.
The extensions refactor made this function redundant so we can remove it.
We had an extra layer of indirection in looking up hashes and sigs based on sigalgs which is now no longer necessary. This removes it.
The sigalgs work has made some old lookup tables and functions redundant so remove them.
We were not incrementing the sequence number every time we sent/received a record.
We need to use the length of the handshake hash for the length of the finished key.
We can't handle these messages yet, so ignore them for now.
A misreading of the TLS1.3 spec meant we were using the handshake hashes up to and including the Client Finished to calculate the client application traffic secret. We should be only use up until the Server Finished.
In TLSv1.3 we must use PSS based sig algs for RSA signing. Ignore any shared sig algs which are PKCS1 based.
TLSv1.3 freezes the record layer version and ensures that it is always set to TLSv1.0. Some implementations check this.
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
In TLSv1.2 an individual sig alg is represented by 1 byte for the hash and 1 byte for the signature. In TLSv1.3 each sig alg is represented by two bytes, where the two bytes together represent a single hash and signature combination. This converts the internal representation of sigalgs to use a single int for the pair, rather than a pair of bytes. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
The extensions refactor made this function redundant so we can remove it. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
We had an extra layer of indirection in looking up hashes and sigs based on sigalgs which is now no longer necessary. This removes it. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
The sigalgs work has made some old lookup tables and functions redundant so remove them. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
We were not incrementing the sequence number every time we sent/received a record. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
We need to use the length of the handshake hash for the length of the finished key. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
We can't handle these messages yet, so ignore them for now. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
A misreading of the TLS1.3 spec meant we were using the handshake hashes up to and including the Client Finished to calculate the client application traffic secret. We should be only use up until the Server Finished. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
In TLSv1.3 we must use PSS based sig algs for RSA signing. Ignore any shared sig algs which are PKCS1 based. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
TLSv1.3 freezes the record layer version and ensures that it is always set to TLSv1.0. Some implementations check this. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
This also acts as a test for the bug fixed in the previous commit. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
TLSv1.3 introduces PSS based sigalgs. Offering these in a TLSv1.3 client implies that the client is prepared to accept these sigalgs even in TLSv1.2. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Previously SKE in TLSProxy only knew about one anonymous ciphersuite so there was never a signature. Extend that to include a ciphersuite that is not anonymous. This also fixes a bug where the existing SKE processing was checking against the wrong anon ciphersuite value. This has a knock on impact on the sslskewith0p test. The bug meant the test was working...but entirely by accident! Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
This enables us to make changes to in-flight TLSv1.3 messages that appear after the ServerHello. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Check that signatures actually work, and that an incorrect signature results in a handshake failure. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Declare a variable as static to silence the warning Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
The siglen value needs to be initialised prior to it being read in the call to EVP_DigestSignFinal later in this function. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
The SHA1 sigalgs were inadvertently missed off in the sigalgs refactor. Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
The length passed to tls1_set_sigalgs() is a multiple of two and there are two char entries in the list for each sigalg. When we set client_sigalgslen or conf_sigalgslen this is the number of ints in the list where there is one entry per sigalg (i.e. half the length of the list passed to the function). Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
Reviewed-by: Rich Salz <[email protected]> (Merged from #2157)
|
And.....pushed to master!!! Thanks @richsalz, @levitte, @davidben and @martinthomson for reviewing and testing this PR! |
Checklist
Description of change
The objective of this PR is to get a simple, inter-operable TLSv1.3 1-RTT handshake working. It is currently marked as WIP, primarily because I need to add a number of tests to this.
Some caveats:
To try it out clone picotls and build it according to the instructions:
https://github.com/h2o/picotls
To run picotls as a server:
and OpenSSL as the client:
To run OpenSSL as the server:
And picotls as the client: