Skip to content

Comments

TLSv1.3: Interoperable 1-RTT handshake#2157

Closed
mattcaswell wants to merge 30 commits intoopenssl:masterfrom
mattcaswell:tls1_3-cert-verify
Closed

TLSv1.3: Interoperable 1-RTT handshake#2157
mattcaswell wants to merge 30 commits intoopenssl:masterfrom
mattcaswell:tls1_3-cert-verify

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Dec 29, 2016

Checklist
  • tests are added or updated
  • CLA is signed
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:

  • By interoperable I mean we can complete a simple 1-RTT handshake with picotls, with OpenSSL either as the client or the server. I have not tried it with other libraries.
  • This does not support HelloRetryRequest yet. If we encounter one of those the connection will fail.
  • No resumption. NewSessionTicket messages are ignored.
  • We currently only send one key_share. By default this is for X25519 (although this can be changed). Picotls currently expects a P-256 key share by default so we need to work around that for now.
  • No client auth. Connections will fail if you try it.
  • This only supports RSA certificates at the moment

To try it out clone picotls and build it according to the instructions:
https://github.com/h2o/picotls

To run picotls as a server:

./cli -c <certfile> -k <keyfile> 127.0.0.1 8443

and OpenSSL as the client:

openssl s_client -connect 127.0.0.1:8443 -curves P-256

To run OpenSSL as the server:

openssl s_server -named_curve P-256 -accept 127.0.0.1:8443

And picotls as the client:

./cli 127.0.0.1 8443

@davidben
Copy link
Contributor

Testing against BoringSSL deployments, I noticed two problems:

  • You send {3, 4} in the record layer rather than {3, 1}. TLS 1.3 freezes the record-layer version at {3, 1}. (If I fix this, it interops with BoringSSL though! :-D)
  • This PR breaks TLS 1.2. The WG decided that, by advertising RSA-PSS, you are also offering to do it in TLS 1.2. The new signature algorithm setup is meant to be backported. Google servers already deploy RSA-PSS at TLS 1.2. Chrome and Firefox's upcoming releases will as well.

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.

@mattcaswell
Copy link
Member Author

@davidben Awesome! Thanks for the feedback...I'll work on fixing those issues.

@mattcaswell mattcaswell force-pushed the tls1_3-cert-verify branch 2 times, most recently from 8005359 to 8ec902b Compare January 5, 2017 14:46
@mattcaswell
Copy link
Member Author

Branch updated to add various tests. Also fixes the issues identified by @davidben. I have successfully managed to create TLSv1.3 connections with the google sandbox server as well as Chrome Canary. Once #2020 goes in, I will remove the WIP label from this PR.

@mattcaswell mattcaswell changed the title WIP: TLSv1.3: Interoperable 1-RTT handshake TLSv1.3: Interoperable 1-RTT handshake Jan 6, 2017
@mattcaswell
Copy link
Member Author

Now that #2020 has gone in, I have rebased this and removed the WIP label. This is now ready for review.

@richsalz
Copy link
Contributor

richsalz commented Jan 7, 2017

Please look at #2052

@martinthomson
Copy link

FWIW, this worked with NSS first time. Tested:

  • openssl s_client -connect 127.0.0.1:4433 against selfserv -d ~/ssl_gtests -n rsa2048 -p 4433 -V tls1.3: -v
  • tstclnt -h 127.0.0.1 -p 4433 -V tls1.3: -v -o -d ~/ssl_gtests against openssl s_server -accept 127.0.0.1:4433
    Where server.pem contains a cert and key for openssl and ~/ssl_gtests is an NSS database containing an RSA key and certificate called 'rsa2048'.

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.

Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 once you rebase

@mattcaswell
Copy link
Member Author

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.
levitte pushed a commit that referenced this pull request Jan 10, 2017
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
The extensions refactor made this function redundant so we can remove it.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
The sigalgs work has made some old lookup tables and functions redundant
so remove them.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
We were not incrementing the sequence number every time we sent/received
a record.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
We can't handle these messages yet, so ignore them for now.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
This also acts as a test for the bug fixed in the previous commit.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
levitte pushed a commit that referenced this pull request Jan 10, 2017
Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
Check that signatures actually work, and that an incorrect signature
results in a handshake failure.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
Declare a variable as static to silence the warning

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
The SHA1 sigalgs were inadvertently missed off in the sigalgs refactor.

Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
levitte pushed a commit that referenced this pull request Jan 10, 2017
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)
levitte pushed a commit that referenced this pull request Jan 10, 2017
Reviewed-by: Rich Salz <[email protected]>
(Merged from #2157)
@mattcaswell
Copy link
Member Author

And.....pushed to master!!! Thanks @richsalz, @levitte, @davidben and @martinthomson for reviewing and testing this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants