Subject: Re: [quicwg/base-drafts] Proposal to make Version Negotiation more like Retry to punt VN to QUICv2 (#2313)

From: Martin Thomson <[email-masked]>
Date: Sun, 10 Feb 2019 19:08:06 -0800

martinthomson requested changes on this pull request.

This needs an addition to the security considerations explaining that version downgrade protection is not provided.  While you might just excise the text and say that this is obvious, having downgrade protection is an expectation of a secure protocol.  That this does not provide that is - as we believe - acceptable so long as this is the only version in existence, or the worst version in existence, but it is nonetheless a possible surprise.

> @@ -1143,40 +1125,30 @@ expectation that it will eventually receive an Initial packet.
 
 ## Handling Version Negotiation Packets {#handle-vn}
 
-When the client receives a Version Negotiation packet, it first checks that the
-Destination and Source Connection ID fields match the Source and Destination
-Connection ID fields in a packet that the client sent.  If this check fails, the
-packet MUST be discarded.
+How a client reacts to receiving a Version Negotiation packet is left as future
+work defined by future versions of QUIC.  Future versions of QUIC that define

This isn't right.  This version needs to define what to do.

I recommend having clients abandon a connection attempt in the absence of better advice.

-A client MUST NOT change the version it uses unless it is in response to a
-Version Negotiation packet from the server.  Once a client receives a packet
-from the server which is not a Version Negotiation packet, it MUST discard other
-Version Negotiation packets on the same connection.  Similarly, a client MUST
-ignore a Version Negotiation packet if it has already received and acted on a
-Version Negotiation packet.
+When a draft implementation receives a Version Negotiation packet, it MAY use
+it to attempt a new connection with one of the supported versions.

```suggestion
it to attempt a new connection with one of the versions listed in the packet.
```

> @@ -4010,14 +3903,6 @@ language from Section 3 of {{!TLS13=RFC8446}}.
    } TransportParameter;
 
    struct {
-      select (Handshake.msg_type) {
-         case client_hello:
-            QuicVersion initial_version;

You missed the definition of QuicVersion at the top of this block.

> @@ -4010,14 +3903,6 @@ language from Section 3 of {{!TLS13=RFC8446}}.
    } TransportParameter;
 
    struct {
-      select (Handshake.msg_type) {
-         case client_hello:
-            QuicVersion initial_version;
-
-         case encrypted_extensions:
-            QuicVersion negotiated_version;
-            QuicVersion supported_versions<4..2^8-4>;
-      };
       TransportParameter parameters<0..2^16-1>;
    } TransportParameters;

This can now be a straight typedef: `TransportParameter TransportParameters<0..2^16-1>;`

-- 
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/2313#pullrequestreview-201941919

---

From: ianswett <[email-masked]>
Date: Mon, 11 Feb 2019 06:32:16 -0800

ianswett commented on this pull request.

I think the paragraph starting with "The final version used for a connection..." is no longer necessary.

-- 
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/2313#pullrequestreview-202134920

---

From: David Schinazi <[email-masked]>
Date: Tue, 12 Feb 2019 14:04:49 -0800

@DavidSchinazi pushed 2 commits.

d53a096b911f487b68412a53bb731f5c1ff6a970  Merge branch 'master' into dschinazi_vn_with_cid
6c4033e7dd54137152deb84d797fe6acc5ced2bd  Version Negotiation: incorporate feedback from MT and Ian

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2313/files/3a0b08c258651e6f9924108baa9b494dfe3774c1..6c4033e7dd54137152deb84d797fe6acc5ced2bd

---

From: David Schinazi <[email-masked]>
Date: Tue, 12 Feb 2019 14:06:03 -0800

Added a commit which incorporate's @martinthomson and @ianswett's feedback

-- 
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/2313#issuecomment-462955523

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Feb 2019 14:37:40 -0800

A few suggestions, and one bit of text that needs to be fixed, otherwise this is good.

> @@ -1149,41 +1131,36 @@ expectation that it will eventually receive an Initial packet.

-When the client receives a Version Negotiation packet, it first checks that the
-Destination and Source Connection ID fields match the Source and Destination
-Connection ID fields in a packet that the client sent.  If this check fails, the
-packet MUST be discarded.
+When a client receives a Version Negotiation packet, it MUST abandon the
+current connection attempt.  Version Negotiation packets are designed to allow
+future versions of QUIC to negotiate the version in use between endpoints.
+It is therefore expected for future versions of QUIC to change how
+implementations of this version of QUIC react to Version Negotiation packets.

I don't think that this can be true.  The point here is that implementations of future versions of QUIC might change how they react to Version Negotiation packets when attempting connections with this version of QUIC.

> @@ -5406,6 +5280,17 @@ correct instance, it is better to send a stateless reset than wait for
 connections to time out.  However, this is acceptable only if the routing cannot
 be influenced by an attacker.
 
+## Version Downgrade {#version-downgrade}
+
+This document defines QUIC Version Negotiation packets {{version-negotiation}},
+which can be used to negotiate the QUIC version used between two endpoints.
+However, this document does not specify how this negotiation will be performed
+between this version and subsequent future versions.  In particular, Version
+Negotiation packets do not contain any mechanism to prevent version downgrade
+attacks.  Future version of QUIC that wish to use Version Negotiation packets

```suggestion
attacks.  Future versions of QUIC that use Version Negotiation packets
```

> @@ -5406,6 +5280,17 @@ correct instance, it is better to send a stateless reset than wait for
 connections to time out.  However, this is acceptable only if the routing cannot
 be influenced by an attacker.
 
+## Version Downgrade {#version-downgrade}
+
+This document defines QUIC Version Negotiation packets {{version-negotiation}},
+which can be used to negotiate the QUIC version used between two endpoints.
+However, this document does not specify how this negotiation will be performed
+between this version and subsequent future versions.  In particular, Version
+Negotiation packets do not contain any mechanism to prevent version downgrade
+attacks.  Future version of QUIC that wish to use Version Negotiation packets
+to negotiate the use of that version MUST define a mechanism that is robust

```suggestion
MUST define a mechanism that is robust
```

-- 
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/2313#pullrequestreview-202943803

---

From: David Schinazi <[email-masked]>
Date: Tue, 12 Feb 2019 16:17:51 -0800

@DavidSchinazi pushed 1 commit.

95d380159040255309fd6ff32a5ebc7808f89cd3  Apply MT suggestions from code review

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2313/files/6c4033e7dd54137152deb84d797fe6acc5ced2bd..95d380159040255309fd6ff32a5ebc7808f89cd3

---

From: David Schinazi <[email-masked]>
Date: Wed, 13 Feb 2019 00:18:32 +0000 (UTC)

6f035221d174ff93d096ad67bbdc1a915fd4907d  Tweak from MT comment

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2313/files/95d380159040255309fd6ff32a5ebc7808f89cd3..6f035221d174ff93d096ad67bbdc1a915fd4907d

---

From: David Schinazi <[email-masked]>
Date: Tue, 12 Feb 2019 16:19:07 -0800

Applied latest comments / changes from @martinthomson 

-- 
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/2313#issuecomment-462999667

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Feb 2019 16:21:02 -0800

@martinthomson pushed 1 commit.

54be05b2e2272bc8be25ae74b22d2b6a116e70e2  "expected for" annoyed me

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2313/files/6f035221d174ff93d096ad67bbdc1a915fd4907d..54be05b2e2272bc8be25ae74b22d2b6a116e70e2

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Feb 2019 16:22:00 -0800

2a2935cce998d132a4156f55f89b80f35df32df2  Reflow

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2313/files/54be05b2e2272bc8be25ae74b22d2b6a116e70e2..2a2935cce998d132a4156f55f89b80f35df32df2

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Feb 2019 16:22:09 -0800

Merged #2313 into master.

-- 
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/2313#event-2135337596