Middlebox changes2 hrr #1091

ekr · 2017-11-06T18:16:45Z

No description provided.

nharper · 2017-11-06T19:13:38Z

draft-ietf-tls-tls13.md

           CipherSuite cipher_suite;
+           uint8 legacy_compression_method = 0;
           Extension extensions<6..2^16-1>;
       } ServerHello;

 version


nit: legacy_version

Yea, I don't really like it either to have two version fields.

Me neither, but it's that or an insecure version fallback. Aesthetics and cleanliness are valuable, but not at the cost of security.

But the same problem may repeat with future middleboxes.

Future middleboxes should understand TLSv1.3... If they don't, then it's the manufacturers' fault.

We have a few local middleboxes to test with. The setup is nowhere near exhaustive (the variance on these things is huge), but it did allow us to iterate on this locally with a number of designs before testing a few out into the wild. This PR came out of what we've learned from all those experiments.

Variants that didn't fix the ServerHello version didn't even survive the local pass. :-(

So do you know why these individual middleboxes did it?

Also consider that most users know or can find out if they are using middleboxes or not, so an "insecure version fallback" is not the only alternative.

Mostly the only way to know is to measure, and that's just a network attacker.

And is getting into an arms race with middleboxes really a good idea?

nharper · 2017-11-06T19:34:22Z

draft-ietf-tls-tls13.md

@@ -3924,7 +3979,7 @@ by an encrypted body, which itself contains a type and optional padding.

       struct {
           ContentType opaque_type = 23; /* application_data */
-           ProtocolVersion legacy_record_version = 0x0301; /* TLS v1.x */
+           ProtocolVersion legacy_record_version = 0x0303; /* TLS v1.2 */


I'm assuming this was changed to 0x0303 so that middleboxes which view the ClientHello and ServerHello and assume TLS 1.2 was negotiated can verify that the TLSCiphertext.record_version is 0x0303 as expected.

Is it worth adding language either here or in #middlebox explaining why the 0x0301 (in TLSPlaintext) and 0x0303 values were chosen?

Is it worth adding language ...

Probably.

It's not 0x0301 for plaintext and 0x0303 for ciphertext. That wouldn't get through the middleboxes anyway. It's 0x0301 for ClientHello (a long-standing quirk from TLS 1.1) and 0x0303 for everything else. And yeah, you're right that it's to match TLS 1.2.

tmshort · 2017-11-06T20:35:51Z

draft-ietf-tls-tls13.md

+
+- The server sends a dummy change_cipher_spec record immediately
+  after its first handshake message. This may either be after a
+  ServerHello or a HelloRetryRequest.


I thought these were being combined?

I opted to treat it as a separate message for simplicity.

tomato42 · 2017-11-07T15:06:31Z

draft-ietf-tls-tls13.md

+
+- The client always provides a non-empty session ID in the ClientHello.
+
+- If not offering early data, the client sends a dummy


what does "dummy" mean here? It's a term not used anywhere else in the draft.

Does the CCS has to be always sent? If not, what the other side is supposed to do when it doesn't receive it? What if it receives it at different time? What if it receives multiple CCS messages one after another?

The receiver ignores all CCS records during the handshake, without any care as to where they do or don't appear. It's in the record layer section.

This is based on implementor feedback. Our initial prototype had the receiver enforce the CCS positions, but the handshake/CCS synchronization on the read side is a bit of a mess in TLS 1.2. There was a desire from some folks to not complicate their TLS 1.3 state machined with this. Discarding them at the record layer, on the other hand, is straightforward. (Additionally, it allows any implementation to unilaterally disable compatibility mode if it is deployed somewhere these middlebox issues are less common.)

Perhaps specify that a "dummy CCS record" is one consisting of a single 0x01 byte.

I meant that "dummy" is obvious to us, in this discussion, in the context of this PR. Not necessarily to somebody reading the spec in 5 years.

I don't think dummy is confusing here.

mattcaswell · 2017-11-07T16:43:21Z

draft-ietf-tls-tls13.md

+condition prior to attempting to deprotect the record. An
+implementation which receives any other change_cipher_spec value or
+which receives a protected change_cipher_spec record MUST abort the
+handshake with an "illegal_parameter" alert. After the handshake is


"unexpected_message" seems more appropriate

Lekensteyn · 2017-11-11T01:01:56Z

draft-ietf-tls-tls13.md

@@ -1653,7 +1653,7 @@ processed and transmitted as specified by the current active connection state.
           hello_verify_request_RESERVED(3),
           new_session_ticket(4),
           end_of_early_data(5),
-           hello_retry_request(6),
+           hello_retry_request_RESERVED(6),           


nit: trailing space

Lekensteyn · 2017-11-11T01:07:03Z

draft-ietf-tls-tls13.md

+  "supported_versions" extension ({{supported-versions}}),
+  and the legacy_version field MUST
+  be set to 0x0303, which is the version number for TLS 1.2.
+  (See {{backward-compatibility}} for details about backward compatibility.)


Should anything be said about the order of the SV extension relative to other extensions? The definition of extensions might change between versions.

Down at line 2050 we (now?) have:

+Upon receipt of a HelloRetryRequest, the client MUST perform the
+checks specified in {{server-hello}} and then process the
+extensions, starting with determining the version using
+"supported_versions". Clients MUST abort the handshake with

No, I don't think we need to order it. You just make two passes.

kaduk · 2017-11-11T03:51:12Z

draft-ietf-tls-tls13.md

+  this field is echoed even if the client's value corresponded to
+  a cached pre-TLS 1.3 session which the server has chosen not
+  to resume. A client which receives a legacy_session_id field
+  which does not match what it sent in the ClientHello


nit: s/which/that/

kaduk · 2017-11-11T03:52:13Z

draft-ietf-tls-tls13.md

+: The contents of the client's legacy_session_id field. Note that
+  this field is echoed even if the client's value corresponded to
+  a cached pre-TLS 1.3 session which the server has chosen not
+  to resume. A client which receives a legacy_session_id field


We should consider saying something about how the TLS 1.3-aware client must do the version check before the resumption check, in case some existing TLS 1.2 code does a resumption/sesion-id check early in processing and would be confused about what protocol is in use.

kaduk · 2017-11-11T03:53:36Z

draft-ietf-tls-tls13.md

+  In TLS 1.3, the TLS server indicates its version using the
+  "supported_versions" extension ({{supported-versions}}),
+  and the legacy_version field MUST
+  be set to 0x0303, which is the version number for TLS 1.2.


Do we want to specify a client alert to send when receiving some other value?

+1. In case legacy_version is not set to 0x0303 and supported_versions is present, the connection should fail with illegal_parameter

though it probably should be placed in the legacy_version description to mirror how Client Hello is described

Actually, we should ignore it to match how we handle ClientHello.legacy_version

And that's what the text already says.

kaduk · 2017-11-11T04:29:08Z

draft-ietf-tls-tls13.md

 cipher_suite
 : The single cipher suite selected by the server from the list in
  ClientHello.cipher_suites. A client which receives a cipher suite
  that was not offered MUST abort the handshake with an "illegal_parameter"
  alert.

+legacy_compression_method
+: A single byte of value 0.


What should a client do if it receives something else?

illegal_parameter seems like a correct choice

Yes, but we don't need to say it here because illegal_parameter's definitions says o.

kaduk · 2017-11-11T23:54:06Z

draft-ietf-tls-tls13.md

+                    ProtocolVersion versions<2..254>;
+
+               case server_hello:
+                    ProtocolVersion selected_version;


There is probably some editorial (?) work to be done where we still talk about HRR all over the place (in the extensions table), etc., but there is no longer a hello_retry_request message type for the actual protocol elements.

kaduk · 2017-11-11T23:54:41Z

draft-ietf-tls-tls13.md

-in previous versions of TLS.
+A server which negotiates TLS 1.3 MUST respond by sending a
+"supported_versions" extension containing the selected version value
+(0x0304). It MUST set the ServerHello.version field to 0x0303 (TLS


legacy_version

kaduk · 2017-11-11T23:57:22Z

draft-ietf-tls-tls13.md

+which receives a protected change_cipher_spec record MUST abort the
+handshake with an "illegal_parameter" alert. After the handshake is
+complete, change_cipher_spec MUST be treated as an unexpected record
+type.


I wonder how much more complicated implementations will have to be to reject CCS post-handshake (as opposed to ignoring it always).

Not very it seems.

kaduk · 2017-11-11T23:58:19Z

draft-ietf-tls-tls13.md

@@ -3924,7 +3979,7 @@ by an encrypted body, which itself contains a type and optional padding.

       struct {
           ContentType opaque_type = 23; /* application_data */
-           ProtocolVersion legacy_record_version = 0x0301; /* TLS v1.x */
+           ProtocolVersion legacy_record_version = 0x0303; /* TLS v1.2 */


Is it worth adding language ...

Probably.

davidben · 2017-11-17T11:15:37Z

draft-ietf-tls-tls13.md

-       struct {
-           ProtocolVersion server_version;
-           CipherSuite cipher_suite;
-           Extension extensions<2..2^16-1>;


One detail I noticed as I was sorting out some of our tests: at one point we had text which disallowed no-op HelloRetryRequests. When HRR got moved to extensions, this turned into just a rule that the extensions block is non-empty. (That plus no unsolicited extensions did the trick.)

This is gone now, but it actually wouldn't have sufficed anyway because every HRR has a supported_versions extension now. So we probably need to go back to explicit text here somewhere.

This is on line 2053.

davidben · 2017-11-17T11:18:18Z

draft-ietf-tls-tls13.md

+
+- If not offering early data, the client sends a dummy
+  change_cipher_spec record immediately before its second flight. This
+  may either be before its second ClientHello or before its encrypted


A somewhat irksome consequence of this: if you do completely stateless HRR, you need to skip the CCS before parsing your "first" (actually second) ClientHello and picking the version.

(Then again, we only need this over TCP which is stateful...)

Lekensteyn · 2017-11-24T12:24:01Z

draft-ietf-tls-tls13.md

+HelloRetryRequest and send a second updated ClientHello. The
+HelloRetryRequest extensions defined in this specification are:
+
+- supported_versions (see {{supported_versions}})


Reference must be {{supported-versions}} or the build fails.

ekr added 9 commits October 30, 2017 15:22

Mandatory changes

c038296

ChangeCipherSpec

e04cd7f

Martin's comments

512aafc

Comments from David Benjamin

44591e7

Add HRR

1dc054e

Updated davidben comments

a93161b

Minor nit from MT

94ebf66

Block code point

deabf64

Don't require that legacy_session_id be random

ea43633

nharper reviewed Nov 6, 2017

View reviewed changes

tmshort reviewed Nov 6, 2017

View reviewed changes

tomato42 reviewed Nov 7, 2017

View reviewed changes

mattcaswell reviewed Nov 7, 2017

View reviewed changes

mattcaswell mentioned this pull request Nov 9, 2017

TLSv1.3 draft 22 openssl/openssl#4701

Closed

Lekensteyn reviewed Nov 11, 2017

View reviewed changes

kaduk reviewed Nov 11, 2017

View reviewed changes

kaduk reviewed Nov 12, 2017

View reviewed changes

dcooper16 mentioned this pull request Nov 16, 2017

TLS 1.3 testssl/testssl.sh#432

Closed

davidben reviewed Nov 17, 2017

View reviewed changes

Lekensteyn reviewed Nov 24, 2017

View reviewed changes

ekr added 4 commits November 24, 2017 09:09

Fix ref

79a2280

Review comments

f55df44

Editorial

e9406c6

Clarify HRR

adaccbe

ekr merged commit c764288 into tlswg:master Nov 24, 2017


		- The client always provides a non-empty session ID in the ClientHello.

		- If not offering early data, the client sends a dummy

Middlebox changes2 hrr #1091

Middlebox changes2 hrr #1091

Uh oh!

Conversation

ekr commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhong Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhong Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Nov 7, 2017 •

edited

Loading

yuhong Nov 8, 2017 •

edited

Loading

yuhong Nov 11, 2017 •

edited

Loading