Subject: [quicwg/base-drafts] Prohibit TLS 1.3 middlebox compatibility mode (#3595)

From: Martin Thomson <[email-masked]>
Date: Wed, 22 Apr 2020 16:12:59 -0700

Closes #3594.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3595

-- Commit Summary --

  * Prohibit TLS 1.3 middlebox compatibility mode

-- File Changes --

    M draft-ietf-quic-tls.md (26)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3595.patch
https://github.com/quicwg/base-drafts/pull/3595.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595

---

From: Kazuho Oku <[email-masked]>
Date: Wed, 22 Apr 2020 21:12:01 -0700

@kazuho commented on this pull request.

> @@ -1631,6 +1632,21 @@ PROTOCOL_VIOLATION.
 As a result, EndOfEarlyData does not appear in the TLS handshake transcript.

+## Prohibit TLS Middlebox Compatibility Mode {#compat-mode}
+
+Appendix D.4 of {{!TLS13}} describes an alteration to the TLS 1.3 handshake as
+a workaround for bugs in some middleboxes. The TLS 1.3 middlebox compatibility
+mode involves setting the legacy_session_id field to a 32-byte value in the
+ClientHello and ServerHello, then sending a change_cipher_spec record. Both
+field and record carry no semantic content and are ignored.
+
+This mode has no use in QUIC as it only applies to middleboxes that interfere
+with TLS over TCP. A client MUST NOT request the use of the TLS 1.3
+compatibility mode. A server MUST treat the receipt of a TLS ClientHello that
+requests the use of the TLS 1.3 middlebox compatibility mode as a connection
+error of type PROTOCOL_VIOLATION.

Use of MUST NOT for client is fine, but I would prefer using SHOULD or MAY on the server-side.

In a server-side TLS 1.3 stack, it is entirely reasonable to implement Compatibility Mode at the record layer in a reactive manner. In your record layer, when you receive the handshake traffic secret from the handshake layer, you inject CCS depending on the length of SH.legacy_session_id that has been retained.

Use of MUST NOT for the server breaks TLS stacks implemented that way.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#pullrequestreview-398746203

---

From: Marten Seemann <[email-masked]>
Date: Thu, 23 Apr 2020 00:30:42 -0700

@marten-seemann commented on this pull request.

> In your record layer, when you receive the handshake traffic secret from the handshake layer, you inject CCS depending on the length of SH.legacy_session_id that has been retained. At the moment, there's nothing that has to be done at the handshake layer on the server-side.

Assume that you're not validating the empty `legacy_session_id` on the server side, how would you prevent the injection of CCS then?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#discussion_r413577840

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Apr 2020 00:53:32 -0700

@martinthomson commented on this pull request.

I can do SHOULD.

The way our stack works is that we let TLS consume the ClientHello and then ask it for a response.  That produces a CCS if the client asked for one.  We turn that into an error (we used to crash).  Any implementation could, as I think Kazuho suggests, just drop the CCS, which might be easier in cases where your interface only passes handshake records.  We have a generic record callback, so detecting record type 20 is easy.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#discussion_r413593052

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Apr 2020 00:54:14 -0700

@martinthomson pushed 1 commit.

cf939cc1e1dd4c40e46856fb335c8208c4a23b5e  Servers don't have to enforce

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595/files/d4baa8a6c01f5d30ff0c145693bd257717b59e2e..cf939cc1e1dd4c40e46856fb335c8208c4a23b5e

---

From: Marten Seemann <[email-masked]>
Date: Thu, 23 Apr 2020 00:55:49 -0700

Should we use a more specific error code than PROTOCOL_VIOLATION? Something in the 0x1XX range?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#issuecomment-618243236

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Apr 2020 01:00:27 -0700

I couldn't see something more applicable.  You could fall back to a generic TLS alert, like handshake_failure, but the error is really a QUIC one.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#issuecomment-618245488

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 23 Apr 2020 01:04:15 -0700

@marten-seemann 
> Assume that you're not validating the empty legacy_session_id on the server side, how would you prevent the injection of CCS then?

As @martinthomson says, our code that emits ChangeCipherSpec would check if the underlying protocol is the TLS record protocol. If that's something else, a TLS alert is generated. See https://github.com/h2o/picotls/blob/master/lib/picotls.c#L1480-L1484.

@martinthomson 
> You could fall back to a generic TLS alert, like handshake_failure, but the error is really a QUIC one.

I'm not sure if I share that view. We already have certain requirements on how TLS is used, and we use TLS error codes to communicate errors (e.g., use of unexpected_message when a TLS Key Update message is received).

FWIW, the code above in picotls sends unexpected_message (I can change that for sure, I just merged it).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#issuecomment-618247412

---

From: Martin Thomson <[email-masked]>
Date: Thu, 23 Apr 2020 02:13:57 -0700

> the code above in picotls sends unexpected_message 

That seems fine, but in my view the fault was with the ClientHello, not the CCS.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#issuecomment-618283358

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 23 Apr 2020 02:19:13 -0700

> That seems fine, but in my view the fault was with the ClientHello, not the CCS.

I agree that the fault is with ClientHello, it's merely a coincidence that we detect that problem when trying to build particular portion of the response. In case of picotls, return of this error will be propagated to the application alongside a partially built buffer, so QUIC stacks using picotls would just send the TLS error code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/3595#issuecomment-618286127