TLSv1.3: Add support for the key_share extension by mattcaswell · Pull Request #1848 · openssl/openssl

mattcaswell · 2016-11-04T13:17:23Z

Checklist

tests are added or updated
CLA is signed

Description of change

This adds support for generating and processing the key_share extension on both the client and the server. There are a few restrictions with this current version which will be fixed in later PRs:

We just generate one key_share for our most preferred group. Really it should be configurable as to how many we want to send.
There is no support yet for the HelloRetryRequest message. Therefore, on the server side if the offered key_share is not suitable then we abort the connection. Similarly on the client side if the server responds with a HelloRetryRequest then we will fail.

The key_share extension does the job of key_exchange which in <=TLSv1.2 is done by the ServerKeyExchange and ClientKeyExchange messages. The TLSv1.3 code in master is still essentially TLSv1.2, so this PR also removes the generation and processing of the SKE and CKE messages. The result is a protocol that is neither TLSv1.2 nor TLSv1.3 but somewhere in the middle. Over time more PRs will move it closer to "real" TLSv1.3.

This PR includes the commits from PRs: #1807, #1808, #1825 and #1847. Therefore please only look at the last 11 commits.

mattcaswell · 2016-11-09T10:19:53Z

Rebased on the latest master to pick up some of the commits from other PRs that have gone in. This PR is still dependant on #1808 and #1825, so please only look at the last 11 commits.

mattcaswell · 2016-11-09T16:31:48Z

Rebased again, so now this PR is only dependent on the one commit from #1825. So please ignore the first commit, everything else is in scope.

mattcaswell · 2016-11-10T15:47:12Z

Rebased this again. All commits are for review now. There is one test failure which is unrelated to this PR (its in current master) and will be fixed by #1894.

mattcaswell · 2016-11-10T15:54:32Z

#1894 got merged quickly so rebased again to pick it up!

richsalz · 2016-11-10T17:55:47Z

ssl/statem/statem_clnt.c

I thought we already resolved this :)

Oops. This was written before we came to that resolution. Fixed.

richsalz · 2016-11-10T17:57:27Z

ssl/statem/statem_clnt.c

make the test the same as line 419?

It really should be s->s3->tmp.cert_req != 0. cert_req of 0 means "we are not sending a Certificate message". A cert_req of 1 means "we are sending a Certificate message, and it has a certificate in it", a cert_req of 2 means "we are sending a Certificate message, but it has no Certificate in it" (don't shoot the messenger that's the way the existing code works).

richsalz · 2016-11-10T17:58:34Z

ssl/statem/statem_clnt.c

remove else after return

richsalz · 2016-11-10T17:58:53Z

ssl/statem/statem_clnt.c

remove else after return

richsalz · 2016-11-10T18:01:17Z

ssl/statem/statem_clnt.c

I prefer "/* FALLTHROUGH */" but probably because I'm an old fogey: http://wiki.c2.com/?LintComments
Not a blocker.

richsalz · 2016-11-10T18:24:17Z

ssl/t1_lib.c

don't need the curly braces. I would do

return i == num_groups ? 0 : 1; or return i < num_groups;

richsalz · 2016-11-10T18:28:52Z

ssl/t1_lib.c

should we have a utility function

# define SSLalerterr(a, ap, f, r) (ap=a, SSLerr(f, r), 0)

And then we could do

return SSLalerterr(al, SSL_AD_INTERNAL_ERROR, SSL_F_SCAN_CLIENTHELLO_TLSEXT, SSL_R_LENGTH_MISMATCH)

Obviously a separate MR. But would you be interested in that?

Maybe...I'm not averse to someone else picking it up, put it that way ;-)

richsalz · 2016-11-10T18:30:01Z

ssl/t1_lib.c

didn't you create a macro for this?

Yes (after I first created this PR). Fixed now.

richsalz · 2016-11-10T18:31:03Z

ssl/t1_lib.c

move this down to just before line 2865 (with no extra blank line)?

richsalz · 2016-11-10T18:33:49Z

test/recipes/70-test_tlsextms.t

maybe $numtests++ if !disabled("tls1_3"); ? your decision.

kaduk · 2016-11-13T01:25:22Z

ssl/statem/statem_clnt.c

A "TODO(TLS1.3): implement the real 1.3 state machine instead of 1.2-that-calls-itself-1.3" would have saved me a bit of confusion.

kaduk · 2016-11-13T01:25:37Z

ssl/statem/statem_srvr.c

It seems like TODO(TLS1.3) is the convention

kaduk · 2016-11-13T01:29:15Z

ssl/statem/statem_clnt.c

Should we assert that this does not happen (or have an error case for it)?

Well...we do (see the default case below)

kaduk · 2016-11-13T01:36:32Z

ssl/t1_enc.c

It's also unclear that s->hit is the right thing to be checking here -- I think external (non-resumption) PSK would trigger this conditional as it stasnds.

kaduk · 2016-11-13T01:37:37Z

ssl/t1_lib.c

You can has hash

kaduk · 2016-11-13T02:10:24Z

ssl/t1_lib.c

Is it too soon to be worrying about == vs. >=?

Should be SSL_IS_TLS13(s)

kaduk · 2016-11-13T02:12:29Z

ssl/t1_lib.c

Should the loop body move into a helper function?

kaduk · 2016-11-13T02:14:58Z

ssl/t1_lib.c

Do the 'get_curvelist's need to be inside the loop?

I'm not sure I follow. We are looping through each of the key_shares sent from the client. For each one we need to check that (1) the group also appears in the supported_groups sent by the client and (2) that it is for a group we can use. If the first fails we abort, and if the second fails then we have to continue the loop on the next key_share...so, yes, it needs to be in the loop?

I was thinking that the values stored in &curves and &num_curves are not going to change for a given s and given second argument, so we could have separate local variables for the peer's supported_groups and what we support, and then just make two calls to tls1_get_curvelist (outside the loop), instead of two times the number of key_shares calls.

kaduk · 2016-11-13T02:16:46Z

ssl/t1_lib.c

This comment is too imprecise; is it "Check that the group for this key share is in the supported_groups that the client sent"? (That is probably too long, but for clarity here.)

fwiw i think it's fine.

You wouldn't even add an "if" to make it grammatical?

sure. who cares. but sure.

I changed it to "Check if this share is in supported_groups sent from client"

kaduk · 2016-11-13T02:17:12Z

ssl/t1_lib.c

How about "Check if this share is for a group we can use"?

mattcaswell · 2016-11-14T13:35:55Z

ssl/statem/statem_clnt.c

mattcaswell · 2016-11-14T13:36:15Z

ssl/statem/statem_clnt.c

Oops. This was written before we came to that resolution. Fixed.

mattcaswell · 2016-11-14T13:39:48Z

ssl/statem/statem_clnt.c

tlsext_ticket_expected is boolean, whilst cert_req is not - hence the difference

mattcaswell · 2016-11-14T13:42:34Z

ssl/statem/statem_clnt.c

It really should be s->s3->tmp.cert_req != 0. cert_req of 0 means "we are not sending a Certificate message". A cert_req of 1 means "we are sending a Certificate message, and it has a certificate in it", a cert_req of 2 means "we are sending a Certificate message, but it has no Certificate in it" (don't shoot the messenger that's the way the existing code works).

mattcaswell · 2016-11-14T13:43:28Z

ssl/statem/statem_clnt.c

We still need the == 1 here (it can be 2)

mattcaswell · 2016-11-14T14:46:03Z

test/recipes/70-test_tlsextms.t

mattcaswell · 2016-11-14T14:49:28Z

ssl/statem/statem_srvr.c

I changed this to SSL_IS_TLS13(s)

mattcaswell · 2016-11-14T14:50:30Z

ssl/t1_lib.c

Changed this (and the instance below to SSL_IS_TLS13(s)

mattcaswell · 2016-11-14T14:51:18Z

ssl/t1_lib.c

Changed to SSL_IS_TLS13(s)

mattcaswell · 2016-11-14T14:52:50Z

ssl/t1_lib.c

Changed to SSL_IS_TLS13(s)

mattcaswell · 2016-11-14T15:00:34Z

New commit pushed addressing feedback comments.

richsalz · 2016-11-14T18:35:29Z

We should never do DH key exchange; only ECDH. I'd prefer we didnt even have that comment, but okay. Just one or two open questions then I can plus-one.

levitte

make update has complaints:

$ $make update
ssl/t1_lib.c:1531:add_client_key_share_ext:ssl_add_serverhello_tlsext
ssl/t1_lib.c:1538:add_client_key_share_ext:ssl_add_serverhello_tlsext
ssl/t1_lib.c:1547:add_client_key_share_ext:ssl_add_serverhello_tlsext
ssl/t1_lib.c:1554:add_client_key_share_ext:ssl_add_serverhello_tlsext
ssl/t1_lib.c:1565:add_client_key_share_ext:ssl_add_serverhello_tlsext
ssl/t1_lib.c:1980:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:1986:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:1996:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2011:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2017:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2025:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2038:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2049:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2063:process_key_share_ext:ssl_scan_clienthello_tlsext
ssl/t1_lib.c:2076:process_key_share_ext:ssl_scan_clienthello_tlsext
FATAL: error discrepancy
make: *** [errors] Error 1

In this commit we just generate the extension on the client side, but don't yet do anything with it. Subsequent commits, will add the server side capability. At the moment we hard code a single key_share. In the future we should make this configurable.

At the moment the server doesn't yet do anything with this information. We still need to send the server's key_share info back to the client. That will happen in subsequent commits.

This is a temporary fix for while we are still using the old session resumption logic in the TLSv1.3 code. Due to differences in EXTMS support we can't resume a <=TLSv1.2 session in a TLSv1.3 connection (the EXTMS consistency check causes the connection to abort). This causes test failures. Ultimately we will rewrite the session resumption logic for TLSv1.3 so this problem will go away. But until then we need a quick fix to keep the tests happy.

The previous commits put in place the logic to exchange key_share data. We now need to do something with that information. In <= TLSv1.2 the equivalent of the key_share extension is the ServerKeyExchange and ClientKeyExchange messages. With key_share those two messages are no longer necessary. The commit removes the SKE and CKE messages from the TLSv1.3 state machine. TLSv1.3 is completely different to TLSv1.2 in the messages that it sends and the transitions that are allowed. Therefore, rather than extend the existing <=TLS1.2 state transition functions, we create a whole new set for TLSv1.3. Intially these are still based on the TLSv1.2 ones, but over time they will be amended. The new TLSv1.3 transitions remove SKE and CKE completely. There's also some cleanup for some stuff which is not relevant to TLSv1.3 and is easy to remove, e.g. the DTLS support (we're not doing DTLSv1.3 yet) and NPN. I also disable EXTMS for TLSv1.3. Using it was causing some added complexity, so rather than fix it I removed it, since eventually it will not be needed anyway.

Numerous style issues as well as references to TLS1_3_VERSION instead of SSL_IS_TLS13(s)

No need to continually get the list of supported curves for the client and server. Just do it once.

mattcaswell · 2016-11-15T17:56:41Z

@kaduk:

I was thinking that the values stored in &curves and &num_curves are not going to change for a given s and given second argument, so we could have separate local variables for the peer's supported_groups and what we support, and then just make two calls to tls1_get_curvelist (outside the loop), instead of two times the number of key_shares calls.

I can't locate this comment anymore in github. Anyway, I moved the two tls1_get_curvelist() calls outside of the loop.

mattcaswell · 2016-11-15T17:57:12Z

New commits pushed, addressing all feedback and fixing make update. I also rebased on latest master.

richsalz · 2016-11-15T20:33:04Z

+1

mattcaswell · 2016-11-16T10:08:41Z

My s->hit remark must have been intended for the other inline comment where Rich was complaining about the name looking like excrement; sorry for the mis-click.

Gah! Github has totally screwed up the threading on this particular discussion, and its quite difficult to follow....however, I think you are referring to this comment:

It's also unclear that s->hit is the right thing to be checking here -- I think external (non-resumption) PSK would trigger this conditional as it stasnds.

Which (I think) is in reference to this code in statem_srvr.c:

    if (s->version == TLS1_3_VERSION && s->s3->peer_tmp == NULL && !s->hit) {

There is no PSK support yet for TLSv1.3. We'll sort that out when we get to it.

mattcaswell · 2016-11-16T10:12:52Z

Pushed. Thanks.

mattcaswell force-pushed the tls1_3-key-share branch from a92298b to f5fa234 Compare November 9, 2016 10:16

mattcaswell force-pushed the tls1_3-key-share branch from f5fa234 to 0d9800a Compare November 9, 2016 16:30

mattcaswell force-pushed the tls1_3-key-share branch from 0d9800a to 16b18bb Compare November 10, 2016 15:46

mattcaswell force-pushed the tls1_3-key-share branch from 16b18bb to 07fb0ac Compare November 10, 2016 15:54

richsalz suggested changes Nov 10, 2016

View reviewed changes

kaduk reviewed Nov 13, 2016

View reviewed changes

mattcaswell commented Nov 14, 2016

View reviewed changes

richsalz approved these changes Nov 14, 2016

View reviewed changes

levitte requested changes Nov 15, 2016

View reviewed changes

mattcaswell added 14 commits November 15, 2016 17:51

Add the key_share processing to the server side

48059e1

At the moment the server doesn't yet do anything with this information. We still need to send the server's key_share info back to the client. That will happen in subsequent commits.

Update the trace code to know about the key_share extension

282429e

Add key_share info to the ServerHello

2a9edd1

Add processing of the key_share received in the ServerHello

ddd89b9

Validate that the provided key_share is in supported_groups

715dc6f

Ensure the whole key_share extension is well formatted

04bd946

Check key_exchange data length is not 0

598e576

Add some tests for the key_share extension

ca7576b

Fixed various style issues in the key_share code

fc6fc61

Numerous style issues as well as references to TLS1_3_VERSION instead of SSL_IS_TLS13(s)

Move getting the curvelist for client and server out of the loop

597709b

No need to continually get the list of supported curves for the client and server. Just do it once.

Fix a typo in a comment

1e4b922

mattcaswell force-pushed the tls1_3-key-share branch from 1d8e770 to 1e4b922 Compare November 15, 2016 17:54

mattcaswell mentioned this pull request Nov 15, 2016

TLSv1.3: Update the state machine to be closer to TLSv1.3 #1932

Closed

2 tasks

richsalz added branch: master Applies to master branch reviewed labels Nov 15, 2016

richsalz closed this Nov 15, 2016

richsalz reopened this Nov 15, 2016

mattcaswell closed this Nov 16, 2016

Uh oh!

Comments

Conversation

mattcaswell commented Nov 4, 2016

Checklist

Description of change

Uh oh!

mattcaswell commented Nov 9, 2016

Uh oh!

mattcaswell commented Nov 9, 2016

Uh oh!

mattcaswell commented Nov 10, 2016

Uh oh!

mattcaswell commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment