TLSv1.3: Use the supported_versions extension for version negotiation by mattcaswell · Pull Request #1808 · openssl/openssl

mattcaswell · 2016-10-31T00:53:12Z

Checklist

tests are added or updated

Description of change

TLSv1.3 does version negotiation differently to <=TL1.2. The ClientHello.version field is now always set to TLSv1.2, and instead we have a new supported_versions extension that lists the version numbers that we are willing to negotiate. This PR adds support for the new extension. We already have some existing test coverage for version neg in test_ssl_new (02-protocol-version.conf). In addition to that I've added some extra tests in the form of a new TLSProxy test.

This builds on all the previous TLSv1.3 PRs, i.e. #1804, #1805 and #1807. Please ignore the first 6 commits in this PR and only review the last 5 commits!!! (I wish we had a way of not showing those...its kind of annoying)

levitte · 2016-10-31T12:26:43Z

(I wish we had a way of not showing those...its kind of annoying)

Having them approved and merged first usually does the trick ;-)

levitte · 2016-10-31T13:45:57Z

ssl/statem/statem_clnt.c

This isn't properly indented, should be aligned with pkt

levitte · 2016-10-31T13:50:57Z

+1 on the commits that belong in this PR, modulo my one comment

kroeckx · 2016-10-31T14:47:19Z

There are problems reported by travis.

mattcaswell · 2016-10-31T14:56:04Z

I suspect the travis problems are inherited from #1807. I just tried to fix those, but then make update failed. I've just tried again, and we'll see if that fixes it. If it does, I'll rebase this one on top of those changes.

kroeckx · 2016-10-31T16:32:13Z

ssl/t1_lib.c

I guess it might be more compatible to not send it, so I think you should just remove that TODO.

Ok - agreed.

kroeckx · 2016-10-31T16:42:28Z

include/openssl/tls1.h

Maybe it should be renamed to TLS1_3_VERSION_DRAFT18.

I guess we never plan to support multiple versions ...

Yeah, the idea is that when a new draft comes out, I've just got one define to update, and the version is updated everywhere we use it.

kroeckx · 2016-10-31T16:49:58Z

ssl/t1_trce.c

Is this tested?

I've tried it from the command line using "-trace" and it works - if that's what you mean. There is no trace "test" at the moment.

kroeckx · 2016-10-31T16:51:12Z

ssl/statem/statem_clnt.c

kroeckx · 2016-10-31T16:53:28Z

ssl/statem/statem_lib.c

I think it makes sense to always look at this if it's present.

I had a discussion with @davidben, about this. I'm now erring towards ignoring this if server version <1.3. There are some weird things about this. What happens if we get a ClientHello.version == TLS1.2, but max version in supported versions list is TLS1.1? What should the ClientHello.version be set to when using this extension and not using TLS1.3, and what do we do if the extension and supported_versions list are conflicting in some way?

The way I look at this is that the if this extension is send, the client is using it to send it's list of supported protocols. If the server doesn't enable TLS 1.3 support, we can still look at what the clients wants.

In your case hello.version == TLS1.2 and TLS 1.1 in the extension, it looks like it's a client that doesn't support TLS 1.2 or 1.3 and we should look at it like one that supports TLS 1.1. We might decide never to send it (and you just removed the TODO), but other clients might decide to always send this extension.

We should either ignore the hello.version in case this extension is present, or insist that it's set to TLS 1.2.

I guess that by only sending and checking this when TLS 1.3 is enabled, this is only tested when both sides actually support TLS 1.3, so we probably have no good coverage of this in the test suite.

There is an explicit test for this in 70-test_sslversions.t - see "Test 6"

Ah, didn't look at that commit yet.

I think this issue should be raised on the TLS WG email list.

kroeckx · 2016-10-31T16:57:51Z

ssl/statem/statem_lib.c

Maybe you want PACKET_as_length_prefixed_1() instead?

Good point. Fixed!

kroeckx · 2016-10-31T17:06:06Z

ssl/statem/statem_lib.c

This overrides what we got from the client hello ...

Yes. If the client is asking for >=TLS1.3 in s->client_version, but they haven't sent supported_versions, then the maximum we negotiate is TLS1.2. That's as per the spec.

kroeckx · 2016-10-31T17:07:47Z

ssl/statem/statem_lib.c

Should we check that the hello->version == TLS1_2_VERSION if the extension is present?

I'm not sure, I can't see anything in the spec about what to do in that case. It does say this:

"If the extension is not present, they MUST negotiate TLS 1.2 or prior as specified in [RFC5246], even if ClientHello.legacy_version is 0x0304 or later."

i.e. if hello->version == TLSv1.3, but supported_versions is not present then negotiate TLS1.2.

I think we might encounter implementations which implement <=TLS1.2 but still send supported_versions....it doesn't say anywhere that you shouldn't do that.

In a conversation I had with @davidben he said "As for servers, the intent was that if you see supported_versions, you totally ignore ClientHello.version and evaluate that other list."

What you quote there is not what we do, we only look at it if we support TLS 1.3 ...

Anyway, in Client Hello for legacy_version it says: this field MUST be set to 0x0303, which was the version number for TLS 1.2

Yes, that's what it says must be sent for a TLS1.3 client, but what should happen if a TLS1.1. client sends it? Presumably it should set it to TLS1.1 because many servers will not understand the extension. Therefore I think we have to ignore the legacy client version altogether if supported_versions is present.

There's another complication. What client_version should we use in the RSA pre-master secret calculation if we end up negotiating <TLS1.3. The one in ClientHello.version or the maximum one in supported_versions? E.g. if ClientHello.version == TLS1.1, but the client sends TLS1.2 in a supported_versions extension...if we take the policy that we ignore ClientHello.version if supported_versions is present, but an RSA ciphersuite is selected which client_version are we then using?

kroeckx · 2016-10-31T17:20:06Z

ssl/statem/statem_lib.c

Shouldn't this only be compared when best_vers != 0?

When best_vers == 0, then version_cmp(s, candidate_vers, best_vers) <= 0 will return false, so we don't hit continue...and so we will then set best_vers = candidate_vers which is what we want - so I think this is correct? That might not be right for when we implement DTLSv1.3 I suppose...but we don't get here if we're using DTLS at the moment.

kroeckx · 2016-10-31T17:39:57Z

ssl/statem/statem_lib.c

method can be NULL, ssl_method_error() can't deal with that.

kroeckx · 2016-10-31T17:48:42Z

ssl/statem/statem_lib.c

Can you move that ; to the next line?

and replace the bare semicolon with continue;

kroeckx · 2016-10-31T21:55:11Z

ssl/t1_lib.c

Given the comment on the TLS list, It looks like we need to limit this to TLS1_2_VERSION too.

There doesn't seem consensus on that yet (ekr didn't like it). So I've not done that at the moment.

Needs a TODO comment to that effect?

kroeckx · 2016-10-31T22:00:33Z

ssl/statem/statem_lib.c

Given the comment on the TLS list, we should ignore versions smaller than TLS 1.2 here.

As above - not done yet.

needs a TODO comment then?

kroeckx · 2016-10-31T22:44:33Z

ssl/statem/statem_lib.c

I think this check is not good enough to check that TLS 1.3 (or higher) is enabled, it could have been disabled using SSL_OP_NO_TLSv1_3. I don't think we currently have any function that can tell you if a protocol >= TLS1.3 is supported on the server side, it's really what this function is checking itself.

I've removed this check, so that supported_versions is always looked at if it is present and takes precedence over legacy_version.

ekasper · 2016-10-31T23:46:26Z

Please ignore the first 6 commits in this PR and only review the last 5 commits!!! (I wish we had a way of not showing those...its kind of annoying)

In the 'Files changed' view, choose 'Changes from' in the dropdown menu, and choose a commit, or Shift + click to choose a range of commits.

mattcaswell · 2016-10-31T23:54:16Z

In the 'Files changed' view, choose 'Changes from' in the dropdown menu, and choose a commit, or Shift + click to choose a range of commits.

Oh...that's nice!

mattcaswell · 2016-11-01T00:36:01Z

Various commits added to address the feedback so far.

mattcaswell · 2016-11-01T00:41:42Z

The supported_versions discussion on the TLS list looks like it might rumble on for a while. For the moment I've just implemented this so that supported_versions is only added as a client if we are prepared to negotiate TLS1.3. From the server side if it is present, we always look at it, and it takes precedence over the legacy_version field. That I think is the closest interpretation of what the spec currently says. If it changes based on the current discussions, we can update it then.

kroeckx · 2016-11-01T10:05:36Z

test/recipes/70-test_sslvertol.t

I would like to see the following tests:

We send a TLS 1.4 client hello with the supported_versions extension (and get TLS 1.3)

We send a TLS 1.3 client hello without the supported_versions extension (and get TLS 1.2)

By some readings if we send a 1.4 client hello with supported_versions extension, we should receive a fatal alert. But I agree that the behavior when receiving such a client hello should be tested.

@kaduk Errr....no that's definitely not right. Unless you mean if you set the legacy_version to 1.4? Just sending 1.4 inside the supported_versions extension should be fine and negotiate 1.3 (assuming 1.3 is also there). Obviously sending 1.4 inside supported_versions without 1.3 as well (or anything lower) will fail.

@kroeckx tests added.

I meant legacy_version of 1.4 ... does "a 1.4 client hello" mean something else? (If it does, how could we send a 1.4 client hello without the supported_versions extension?)

kroeckx · 2016-11-01T10:08:06Z

test/ssl-tests/17-renegotiate.conf.in

Is there a reason for this MaxProtocol?

Not having it causes the tests to fail. The reason is this check in ssl_choose_server_version()

switch (server_version) { default: if (version_cmp(s, client_version, s->version) < 0) return SSL_R_WRONG_SSL_VERSION;

When renegotiating, s->version is set to TLS1.3, but we always set the legacy version to TLS1.2, so this falls over. However, renegotiation is not allowed in TLS1.3. I've just not got as far as actually disabling it yet. So the reneg tests are restricted to only go up to TLS1.2. I've also just added a TODO to remind us to go back and disable TLS1.3 reneg.

kroeckx · 2016-11-01T10:11:21Z

ssl/statem/statem_lib.c


                method = vent->smeth();
-                if (ssl_method_error(s, method) == 0) {
+                if (method != NULL && ssl_method_error(s, method) == 0) {


I guess my comment wasn't good enough. It's smeth that can be NULL. You don't want to call smeth()

Doh!! Fixed.

mattcaswell · 2016-11-02T11:45:24Z

New commits pushed to address the feedback from @kroeckx

kroeckx · 2016-11-05T12:03:40Z

Should we have a target in travis that has enable-tls1_3 in it?

kroeckx · 2016-11-05T12:12:13Z

ssl/t1_lib.c

I'm wondering if it's useful to have a function to start a new extension.

Maybe. Seems a bit over kill to me.

kroeckx · 2016-11-05T12:17:05Z

test/recipes/70-test_sslversions.t

I'm unsure how TLS 1.2 is disabled here.

I guess NO_TLS1_3 is misleading, because it just does TLS 1.0 and 1.1

I renamed it to TLS1_1_AND_1_0_ONLY

kroeckx · 2016-11-05T12:26:41Z

So the changes took fine to me (last 11 commits). I looked at the tests, they look fine for me, but don't mark me as having reviewed them.

mattcaswell · 2016-11-07T15:43:57Z

Commit added to address the feedback above. I also found a couple of spots where we weren't checking TLS1_3_VERSION_DRAFT correctly, so I added a commit to fix that too.

…tension

We can end up with a NULL SSL_METHOD function if a method has been disabled. If that happens then we shouldn't call vent->smeth().

If supported_versions is present it takes precedence.

Replace a bare ";" with "continue;" for the body of a for loop.

Send a TLS1.4 ClientHello with supported_versions and get TLS1.3 Send a TLS1.3 ClientHello without supported_versions and get TLS1.2

Renegotiation does not exist in TLS1.3, so we need to disable it at some point.

mattcaswell · 2016-11-09T09:43:26Z

This has now been rebased on latest master. All other PRs that this one is dependant on have now been merged - so all commits are now in scope for review.

There were a few places where we weren't checking to see if we were using the draft TLS1.3 version or not.

No need to have a supported versions table and a versions table. They should be the same.

mattcaswell · 2016-11-09T09:54:04Z

Just spotted a redundant table in the trace code, so I added a commit to fix that.

richsalz · 2016-11-09T13:47:49Z

ssl/statem/statem_clnt.c

     */
-    if (!WPACKET_put_bytes_u16(pkt, s->client_version)
+    if (!WPACKET_put_bytes_u16(pkt,
+                               (!SSL_IS_DTLS(s)


break out that second parameter (the nested ternary operator) into a separate variable, for clarity.

richsalz · 2016-11-09T13:49:13Z

ssl/statem/statem_lib.c

needs a TODO comment then?

richsalz · 2016-11-09T13:50:14Z

ssl/statem/statem_lib.c

+                s->client_version = candidate_vers;
+            if (version_cmp(s, candidate_vers, best_vers) <= 0)
+                continue;
+            for (vent = table;


not for this PR, but we really should come up with a consistent way of walking through tables: OSS_NELEM or a sentinel

richsalz · 2016-11-09T13:51:20Z

ssl/statem/statem_srvr.c


-    if (!WPACKET_put_bytes_u16(pkt, s->version)
+    /* TODO(TLS1.3): Remove the DRAFT conditional before release */
+    if (!WPACKET_put_bytes_u16(pkt, (s->version == TLS1_3_VERSION)


again i think a local variable instead of the nested ?: is better, but here it's not as bad, so that's only a PLEASE not a MUST :)

Done, although this will get removed anyway before final release when we're not having to worry about draft versions anymore.

richsalz · 2016-11-09T13:51:59Z

ssl/t1_lib.c

        return 0;
    }

+    if (!SSL_IS_DTLS(s) && s->version >= TLS1_3_VERSION) {


hm. I think this is the third time I've seen that kind of if expression ... macro?

richsalz · 2016-11-09T13:52:18Z

ssl/t1_lib.c

Needs a TODO comment to that effect?

richsalz · 2016-11-09T13:53:34Z

ssl/t1_trce.c

    {TLS1_2_VERSION, "TLS 1.2"},
    {TLS1_3_VERSION, "TLS 1.3"},
+    /* TODO(TLS1.3): Remove this line before release */
+    {TLS1_3_VERSION_DRAFT, TLS1_3_VERSION_DRAFT_TXT},


is this the only place the (stupid, we hates them) _TXT is used? Even if not, everything else in this table has the string constants.

Yes. The reason is so that I have only one place to update if we go from draft 18 to draft 19, i.e. in tls1.h:

/* TODO(TLS1.3) REMOVE ME: Version indicator for draft -18 */ # define TLS1_3_VERSION_DRAFT 0x7f12 # define TLS1_3_VERSION_DRAFT_TXT "TLS 1.3 (draft 18)"

This is temporary anyway. By the time of release we shouldn't be using a draft version and it all gets deleted.

Added some TODOs, refactored a couple of things and added a SSL_IS_TLS13() macro.

mattcaswell · 2016-11-09T14:47:01Z

New commit pushed addressing feedback.

richsalz · 2016-11-09T14:59:49Z

although, now that you have that SSL_IS_TLS13 macro, my request for a local variable isn't as important. :) your call.

mattcaswell · 2016-11-09T16:10:42Z

Pushed. Thanks.

levitte reviewed Oct 31, 2016

View reviewed changes

ssl/statem/statem_clnt.c Outdated

Copy link

Member

levitte Oct 31, 2016

Choose a reason for hiding this comment

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

This isn't properly indented, should be aligned with pkt

levitte added the reviewed label Oct 31, 2016

kroeckx reviewed Oct 31, 2016

View reviewed changes

mattcaswell force-pushed the tls1_3-version-neg branch from a8dc41d to 65a49da Compare November 1, 2016 00:35

kroeckx reviewed Nov 1, 2016

View reviewed changes

mattcaswell force-pushed the tls1_3-version-neg branch from c27fabb to a9e07d7 Compare November 2, 2016 11:44

kroeckx reviewed Nov 5, 2016

View reviewed changes

mattcaswell added 12 commits November 9, 2016 09:32

Add the ability to send the supported_versions extension

3811a7f

Ensure that the -trace option can interpret the supported_versions ex…

1687335

…tension

Add server side support for supported_versions extension

8589fb9

Update TLS1.3 draft version numbers for latest draft

6f05ffd

Add a test for the supported_versions extension

3bc1dd5

Address some review feedback comments for supported_versions

1377c90

Ensure that vent->smeth != NULL before we call vent->smeth()

caec0be

We can end up with a NULL SSL_METHOD function if a method has been disabled. If that happens then we shouldn't call vent->smeth().

Look at the supported_versions extension even if the server <TLS1.3

e9d4087

If supported_versions is present it takes precedence.

A style tweak based on feedback received

7c17f00

Replace a bare ";" with "continue;" for the body of a for loop.

Add some more version tests

a8ef1fd

Send a TLS1.4 ClientHello with supported_versions and get TLS1.3 Send a TLS1.3 ClientHello without supported_versions and get TLS1.2

Add a TODO(TLS1.3) about renegotation

df9fcec

Renegotiation does not exist in TLS1.3, so we need to disable it at some point.

Give the test with only TLS1.1 and TLS1.0 a better name

c65b6af

mattcaswell force-pushed the tls1_3-version-neg branch from 1c7ad27 to a2d7f79 Compare November 9, 2016 09:42

mattcaswell added 2 commits November 9, 2016 09:52

Fix some missing checks for TLS1_3_VERSION_DRAFT

dae302f

There were a few places where we weren't checking to see if we were using the draft TLS1.3 version or not.

Remove some redundant trace code

e7e66ad

No need to have a supported versions table and a versions table. They should be the same.

mattcaswell force-pushed the tls1_3-version-neg branch from a2d7f79 to e7e66ad Compare November 9, 2016 09:53

richsalz suggested changes Nov 9, 2016

View reviewed changes

Address some supported_versions review comments

144ebe7

Added some TODOs, refactored a couple of things and added a SSL_IS_TLS13() macro.

richsalz approved these changes Nov 9, 2016

View reviewed changes

mattcaswell closed this Nov 9, 2016

Uh oh!

Conversation

mattcaswell commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

levitte commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kroeckx commented Oct 31, 2016

Uh oh!

mattcaswell commented Oct 31, 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!

kroeckx Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mattcaswell commented Oct 31, 2016 •

edited

Loading

levitte commented Oct 31, 2016 •

edited

Loading

kroeckx Oct 31, 2016 •

edited

Loading