Subject: Re: [quicwg/base-drafts] Move negotiated_version (#959)

From: Marten Seemann <[email-masked]>
Date: Fri, 01 Dec 2017 08:51:15 +0000 (UTC)

marten-seemann commented on this pull request.

> @@ -1228,45 +1228,49 @@ version as part of the transport parameters (see {{transport-parameters}}).  As
 a result, modification of version negotiation packets by an attacker can be
 detected.
 
-The client includes two fields in the transport parameters:
-
-* The negotiated_version is the version that was finally selected for use.  This
-  MUST be identical to the value that is on the packet that carries the
-  ClientHello.  A server that receives a negotiated_version that does not match
-  the version of QUIC that is in use MUST terminate the connection with a
-  VERSION_NEGOTIATION_ERROR error code.
-
-* The initial_version is the version that the client initially attempted to use.
-  If the server did not send a version negotiation packet {{packet-version}},
-  this will be identical to the negotiated_version.
+The client includes the initial_version field in its transport parameters.  The
+initial_version is the version that the client initially attempted to use.  If
+the server did not send a version negotiation packet {{packet-version}}, this
+will be identical to the negotiated_version.

To the negotiated_version in the encrypted_extensions.

> +stateless server) uses a different process. If the initial_version matches the
+version of QUIC that is in use, a stateless server can accept the value.
+
+If the initial_version is different from the version of QUIC that is in use, a
+stateless server MUST check that it would have sent a version negotiation packet
+if it had received a packet with the indicated initial_version.  If a server
+would have accepted the version included in the initial_version and the value
+differs from the QUIC version that is in use, the server MUST terminate the
+connection with a VERSION_NEGOTIATION_ERROR error.
+
+The server includes both the version of QUIC that is in use and a list of the
+QUIC versions that the server supports.
+
+The negotiated_version field is the version that is in use.  This MUST be set by
+the server to the value that is on the Initial packet that it accepts (not an
+Initial packet that triggers a Retry or Version Negotiation packet).  A client

Why the Retry packet?

-- 
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/959#pullrequestreview-80433372

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 23:27:54 +0000 (UTC)

martinthomson commented on this pull request.

Both are (potentially) stateless from the server perspective.  You *could* save the packet number in the Retry cookie, but I don't think we need to require that servers do that.  Note that the size of this cookie is strictly limited.  It has to fit into the padding space in the Initial packet.

-- 
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/959#discussion_r154540920

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 23:50:24 +0000 (UTC)

I'm not allowed to use TLS message names, but I'll use "server transport parameters".

-- 
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/959#discussion_r154541689

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 23:53:59 +0000 (UTC)

@martinthomson pushed 2 commits.

a5ca4b8  Explain where negotiated_version comes from
8488f8b  Remove run-on sentence that runs on

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/959/files/8884c01f1b0b0a193f821a0d600c4cedc9ea33ac..8488f8bdc2f4cfab422db430744535561711af03

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 23:54:27 +0000 (UTC)

@huitema, can you review this PR please?

-- 
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/959#issuecomment-348831084

---

From: Christian Huitema <[email-masked]>
Date: Sun, 03 Dec 2017 17:35:53 -0800

huitema approved this pull request.

Looks good enough, but the text in section 7.4.4 could be improved.

>              QuicVersion initial_version;
 
          case encrypted_extensions:
+            QuicVersion negotiated_version;
             QuicVersion supported_versions<4..2^8-4>;
 
          case new_session_ticket:

LGTM. 

>  supported_versions list and - if version negotiation was performed - that it
 would have selected the negotiated version.  A client MUST terminate the
-connection with a VERSION_NEGOTIATION_ERROR error code if the
-negotiated_version value is not included in the supported_versions list.  A
-client MUST terminate with a VERSION_NEGOTIATION_ERROR error code if
-version negotiation occurred but it would have selected a different version
-based on the value of the supported_versions list.
+connection with a VERSION_NEGOTIATION_ERROR error code if the current QUIC
+version is not listed in the supported_versions list.  A client MUST terminate
+with a VERSION_NEGOTIATION_ERROR error code if version negotiation occurred but
+it would have selected a different version based on the value of the
+supported_versions list.

That's OK, but I had to read the text several times, so it probably should be clearer. How about something like that:

The whole point is to prevent version downgrade by a third party, by locking the version ID inside TLS extensions protected by the TLS protocol. The two types of downgrade attacks are:

1) MITM spoofing the client initial, replacing the original value by a downgrade value;
2) MITM dropping the client initial and sending a spoofed version negotiation packet that proposes the downgrade value, or alternately MITM intercepting a genuine negotiation packet and reducing the proposed list to a downgrade value.

Both types will be detected. In the first case, the server will see the initial version in the transport parameters sent by the client, and detect that it should have been accepted. In the second case, the client will see that the transport parameters of the server include a version that it would have preferred over the downgrade version. 

-- 
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/959#pullrequestreview-80722568

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 19:35:27 -0800

@martinthomson pushed 1 commit.

3359f1e  Add a brief intro

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/959/files/8488f8bdc2f4cfab422db430744535561711af03..3359f1e13a507c35c2082e2f55deee86b3591004

---

From: janaiyengar <[email-masked]>
Date: Mon, 04 Dec 2017 06:10:27 +0000 (UTC)

janaiyengar approved this pull request.

Still LGTM.

-- 
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/959#pullrequestreview-80744543

---

From: Martin Thomson <[email-masked]>
Date: Sun, 03 Dec 2017 23:54:06 -0800

Merged #959.

-- 
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/959#event-1369910668