From a1a4f35096876dfa5843474f1765db284f799897 Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Thu, 2 Nov 2017 18:42:09 +0700
Subject: [PATCH] send initial version in the CHLO, not the current version

This commit fixes two bugs:
1. In the CHLO, we need to send the initial QUIC version. It will differ
from the current version if version negotiation was performed.
2. The session setup was using the wrong version (current version, and
not the initial version), such that we would have sent the wrong version
in the TLS handshake as well.
---
 client.go                                      | 1 +
 internal/handshake/crypto_setup_client.go      | 5 ++++-
 internal/handshake/crypto_setup_client_test.go | 4 +++-
 session.go                                     | 5 +++--
 session_test.go                                | 1 +
 5 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/client.go b/client.go
index e2c36212..e408e5a0 100644
--- a/client.go
+++ b/client.go
@@ -353,6 +353,7 @@ func (c *client) handleVersionNegotiationPacket(hdr *wire.Header) error {
 
 func (c *client) createNewSession(initialVersion protocol.VersionNumber, negotiatedVersions []protocol.VersionNumber) error {
 	var err error
+	utils.Debugf("createNewSession with initial version %s", initialVersion)
 	c.session, c.handshakeChan, err = newClientSession(
 		c.conn,
 		c.hostname,
diff --git a/internal/handshake/crypto_setup_client.go b/internal/handshake/crypto_setup_client.go
index 87b83ddd..c923bbc3 100644
--- a/internal/handshake/crypto_setup_client.go
+++ b/internal/handshake/crypto_setup_client.go
@@ -23,6 +23,7 @@ type cryptoSetupClient struct {
 	hostname           string
 	connID             protocol.ConnectionID
 	version            protocol.VersionNumber
+	initialVersion     protocol.VersionNumber
 	negotiatedVersions []protocol.VersionNumber
 
 	cryptoStream io.ReadWriter
@@ -74,6 +75,7 @@ func NewCryptoSetupClient(
 	params *TransportParameters,
 	paramsChan chan<- TransportParameters,
 	aeadChanged chan<- protocol.EncryptionLevel,
+	initialVersion protocol.VersionNumber,
 	negotiatedVersions []protocol.VersionNumber,
 ) (CryptoSetup, error) {
 	nullAEAD, err := crypto.NewNullAEAD(protocol.PerspectiveClient, connID, version)
@@ -92,6 +94,7 @@ func NewCryptoSetupClient(
 		nullAEAD:           nullAEAD,
 		paramsChan:         paramsChan,
 		aeadChanged:        aeadChanged,
+		initialVersion:     initialVersion,
 		negotiatedVersions: negotiatedVersions,
 		divNonceChan:       make(chan []byte),
 	}, nil
@@ -423,7 +426,7 @@ func (h *cryptoSetupClient) getTags() (map[Tag][]byte, error) {
 	}
 
 	versionTag := make([]byte, 4)
-	binary.BigEndian.PutUint32(versionTag, uint32(h.version))
+	binary.BigEndian.PutUint32(versionTag, uint32(h.initialVersion))
 	tags[TagVER] = versionTag
 
 	if len(h.stk) > 0 {
diff --git a/internal/handshake/crypto_setup_client_test.go b/internal/handshake/crypto_setup_client_test.go
index ae9cf03c..a88df546 100644
--- a/internal/handshake/crypto_setup_client_test.go
+++ b/internal/handshake/crypto_setup_client_test.go
@@ -105,7 +105,7 @@ var _ = Describe("Client Crypto Setup", func() {
 
 		stream = newMockStream()
 		certManager = &mockCertManager{}
-		version := protocol.Version37
+		version := protocol.Version39
 		// use a buffered channel here, so that we can parse a SHLO without having to receive the TransportParameters to avoid blocking
 		paramsChan = make(chan TransportParameters, 1)
 		aeadChanged = make(chan protocol.EncryptionLevel, 2)
@@ -118,6 +118,7 @@ var _ = Describe("Client Crypto Setup", func() {
 			&TransportParameters{IdleTimeout: protocol.DefaultIdleTimeout},
 			paramsChan,
 			aeadChanged,
+			protocol.Version37,
 			nil,
 		)
 		Expect(err).ToNot(HaveOccurred())
@@ -490,6 +491,7 @@ var _ = Describe("Client Crypto Setup", func() {
 		})
 
 		It("has the right values for an inchoate CHLO", func() {
+			Expect(cs.version).ToNot(Equal(cs.initialVersion)) // make sure we can test that TagVER actually has the initial version, and not the current version
 			cs.hostname = "sni-hostname"
 			certManager.commonCertificateHashes = []byte("common certs")
 			tags, err := cs.getTags()
diff --git a/session.go b/session.go
index a40b845f..d6dc5c4b 100644
--- a/session.go
+++ b/session.go
@@ -148,7 +148,7 @@ var newClientSession = func(
 	connectionID protocol.ConnectionID,
 	tlsConf *tls.Config,
 	config *Config,
-	initialVersion protocol.VersionNumber, // needed for validation of the version negotaion over TLS
+	initialVersion protocol.VersionNumber,
 	negotiatedVersions []protocol.VersionNumber, // needed for validation of the GQUIC version negotiaton
 ) (packetHandler, <-chan handshakeEvent, error) {
 	s := &session{
@@ -158,7 +158,7 @@ var newClientSession = func(
 		version:      v,
 		config:       config,
 	}
-	return s.setup(nil, hostname, tlsConf, v, negotiatedVersions)
+	return s.setup(nil, hostname, tlsConf, initialVersion, negotiatedVersions)
 }
 
 func (s *session) setup(
@@ -261,6 +261,7 @@ func (s *session) setup(
 				transportParams,
 				paramsChan,
 				aeadChanged,
+				initialVersion,
 				negotiatedVersions,
 			)
 		}
diff --git a/session_test.go b/session_test.go
index fa7808d1..93bd39c8 100644
--- a/session_test.go
+++ b/session_test.go
@@ -1522,6 +1522,7 @@ var _ = Describe("Client Session", func() {
 			_ *handshake.TransportParameters,
 			_ chan<- handshake.TransportParameters,
 			aeadChangedP chan<- protocol.EncryptionLevel,
+			_ protocol.VersionNumber,
 			_ []protocol.VersionNumber,
 		) (handshake.CryptoSetup, error) {
 			aeadChanged = aeadChangedP