Merge pull request #962 from lucas-clemente/fix-871

send reserved version numbers

client_test.go

@@ -369,7 +369,7 @@ var _ = Describe("Client", func() {
 				Expect(firstSession.closeReason).To(Equal(errCloseSessionForNewVersion))
 				Consistently(func() bool { return secondSession.closed }).Should(BeFalse())
 				Expect(cl.connectionID).ToNot(BeEquivalentTo(0x1337))
-				Expect(negotiatedVersions).To(Equal([]protocol.VersionNumber{newVersion}))
+				Expect(negotiatedVersions).To(ContainElement(newVersion))
 				Expect(initialVersion).To(Equal(actualInitialVersion))
 
 				handshakeChan <- handshakeEvent{encLevel: protocol.EncryptionSecure}

internal/handshake/crypto_setup_server.go

@@ -431,7 +431,7 @@ func (h *cryptoSetupServer) handleCHLO(sni string, data []byte, cryptoData map[T
 	replyMap := h.params.getHelloMap()
 	// add crypto parameters
 	verTag := &bytes.Buffer{}
-	for _, v := range h.supportedVersions {
+	for _, v := range protocol.GetGreasedVersions(h.supportedVersions) {
 		utils.BigEndian.WriteUint32(verTag, uint32(v))
 	}
 	replyMap[TagPUBS] = ephermalKex.PublicKey()

internal/handshake/crypto_setup_server_test.go

@@ -313,12 +313,17 @@ var _ = Describe("Server Crypto Setup", func() {
 			})
 			Expect(err).ToNot(HaveOccurred())
 			Expect(response).To(HavePrefix("SHLO"))
-			Expect(response).To(ContainSubstring("ephermal pub"))
-			Expect(response).To(ContainSubstring("SNO\x00"))
+			message, err := ParseHandshakeMessage(bytes.NewReader(response))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(message.Data).To(HaveKeyWithValue(TagPUBS, []byte("ephermal pub")))
+			Expect(message.Data).To(HaveKey(TagSNO))
+			Expect(message.Data).To(HaveKey(TagVER))
+			// the supported versions should include one reserved version number
+			Expect(message.Data[TagVER]).To(HaveLen(4*len(supportedVersions) + 4))
 			for _, v := range supportedVersions {
 				b := &bytes.Buffer{}
 				utils.BigEndian.WriteUint32(b, uint32(v))
-				Expect(response).To(ContainSubstring(string(b.Bytes())))
+				Expect(message.Data[TagVER]).To(ContainSubstring(string(b.Bytes())))
 			}
 			Expect(checkedSecure).To(BeTrue())
 			Expect(checkedForwardSecure).To(BeTrue())

internal/handshake/tls_extension_handler_server.go

@@ -47,13 +47,14 @@ func (h *extensionHandlerServer) Send(hType mint.HandshakeType, el *mint.Extensi
 		// TODO(#855): generate a real token
 		transportParameter{statelessResetTokenParameterID, bytes.Repeat([]byte{42}, 16)},
 	)
-	supportedVersions := make([]uint32, len(h.supportedVersions))
-	for i, v := range h.supportedVersions {
-		supportedVersions[i] = uint32(v)
+	supportedVersions := protocol.GetGreasedVersions(h.supportedVersions)
+	versions := make([]uint32, len(supportedVersions))
+	for i, v := range supportedVersions {
+		versions[i] = uint32(v)
 	}
 	data, err := syntax.Marshal(encryptedExtensionsTransportParameters{
 		NegotiatedVersion: uint32(h.version),
-		SupportedVersions: supportedVersions,
+		SupportedVersions: versions,
 		Parameters:        transportParams,
 	})
 	if err != nil {

internal/handshake/tls_extension_handler_server_test.go

@@ -45,7 +45,8 @@ var _ = Describe("TLS Extension Handler, for the server", func() {
 
 		It("adds TransportParameters to the EncryptedExtensions message", func() {
 			handler.version = 666
-			handler.supportedVersions = []protocol.VersionNumber{13, 37, 42}
+			versions := []protocol.VersionNumber{13, 37, 42}
+			handler.supportedVersions = versions
 			err := handler.Send(mint.HandshakeTypeEncryptedExtensions, &el)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(el).To(HaveLen(1))
@@ -55,8 +56,12 @@ var _ = Describe("TLS Extension Handler, for the server", func() {
 			eetp := &encryptedExtensionsTransportParameters{}
 			_, err = syntax.Unmarshal(ext.data, eetp)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(eetp.SupportedVersions).To(Equal([]uint32{13, 37, 42}))
 			Expect(eetp.NegotiatedVersion).To(BeEquivalentTo(666))
+			// the SupportedVersions will contain one reserved version number
+			Expect(eetp.SupportedVersions).To(HaveLen(len(versions) + 1))
+			for _, version := range versions {
+				Expect(eetp.SupportedVersions).To(ContainElement(uint32(version)))
+			}
 		})
 	})
 

internal/protocol/version.go

@@ -1,11 +1,13 @@
 package protocol
 
 import (
+	"crypto/rand"
+	"encoding/binary"
 	"fmt"
 )
 
 // VersionNumber is a version number as int
-type VersionNumber int
+type VersionNumber int32
 
 // gQUIC version range as defined in the wiki: https://github.com/quicwg/base-drafts/wiki/QUIC-Versions
 const (
@@ -112,3 +114,22 @@ func ChooseSupportedVersion(ours, theirs []VersionNumber) (VersionNumber, bool)
 	}
 	return 0, false
 }
+
+// generateReservedVersion generates a reserved version number (v & 0x0f0f0f0f == 0x0a0a0a0a)
+func generateReservedVersion() VersionNumber {
+	b := make([]byte, 4)
+	_, _ = rand.Read(b) // ignore the error here. Failure to read random data doesn't break anything
+	return VersionNumber((binary.BigEndian.Uint32(b) | 0x0a0a0a0a) & 0xfafafafa)
+}
+
+// GetGreasedVersions adds one reserved version number to a slice of version numbers, at a random position
+func GetGreasedVersions(supported []VersionNumber) []VersionNumber {
+	b := make([]byte, 1)
+	_, _ = rand.Read(b) // ignore the error here. Failure to read random data doesn't break anything
+	randPos := int(b[0]) % (len(supported) + 1)
+	greased := make([]VersionNumber, len(supported)+1)
+	copy(greased, supported[:randPos])
+	greased[randPos] = generateReservedVersion()
+	copy(greased[randPos+1:], supported[randPos:])
+	return greased
+}

internal/protocol/version_test.go

@@ -6,6 +6,10 @@ import (
 )
 
 var _ = Describe("Version", func() {
+	isReservedVersion := func(v VersionNumber) bool {
+		return v&0x0f0f0f0f == 0x0a0a0a0a
+	}
+
 	// version numbers taken from the wiki: https://github.com/quicwg/base-drafts/wiki/QUIC-Versions
 	It("has the right gQUIC version number", func() {
 		Expect(Version39).To(BeEquivalentTo(0x51303339))
@@ -16,6 +20,11 @@ var _ = Describe("Version", func() {
 		Expect(VersionTLS.UsesTLS()).To(BeTrue())
 	})
 
+	It("versions don't have reserved version numbers", func() {
+		Expect(isReservedVersion(Version39)).To(BeFalse())
+		Expect(isReservedVersion(VersionTLS)).To(BeFalse())
+	})
+
 	It("has the right string representation", func() {
 		Expect(Version39.String()).To(Equal("gQUIC 39"))
 		Expect(VersionTLS.String()).To(ContainSubstring("TLS"))
@@ -105,4 +114,47 @@ var _ = Describe("Version", func() {
 			Expect(ok).To(BeFalse())
 		})
 	})
+
+	Context("reserved versions", func() {
+		It("adds a greased version if passed an empty slice", func() {
+			greased := GetGreasedVersions([]VersionNumber{})
+			Expect(greased).To(HaveLen(1))
+			Expect(isReservedVersion(greased[0])).To(BeTrue())
+		})
+
+		It("creates greased lists of version numbers", func() {
+			supported := []VersionNumber{10, 18, 29}
+			for _, v := range supported {
+				Expect(isReservedVersion(v)).To(BeFalse())
+			}
+			var greasedVersionFirst, greasedVersionLast, greasedVersionMiddle int
+			// check that
+			// 1. the greased version sometimes appears first
+			// 2. the greased version sometimes appears in the middle
+			// 3. the greased version sometimes appears last
+			// 4. the supported versions are kept in order
+			for i := 0; i < 100; i++ {
+				greased := GetGreasedVersions(supported)
+				Expect(greased).To(HaveLen(4))
+				var j int
+				for i, v := range greased {
+					if isReservedVersion(v) {
+						if i == 0 {
+							greasedVersionFirst++
+						}
+						if i == len(greased)-1 {
+							greasedVersionLast++
+						}
+						greasedVersionMiddle++
+						continue
+					}
+					Expect(supported[j]).To(Equal(v))
+					j++
+				}
+			}
+			Expect(greasedVersionFirst).ToNot(BeZero())
+			Expect(greasedVersionLast).ToNot(BeZero())
+			Expect(greasedVersionMiddle).ToNot(BeZero())
+		})
+	})
 })

internal/wire/header_test.go

@@ -129,7 +129,10 @@ var _ = Describe("Header", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(hdr.isPublicHeader).To(BeTrue())
 			Expect(hdr.ConnectionID).To(Equal(protocol.ConnectionID(0x42)))
-			Expect(hdr.SupportedVersions).To(Equal(versions))
+			// in addition to the versions, the supported versions might contain a reserved version number
+			for _, version := range versions {
+				Expect(hdr.SupportedVersions).To(ContainElement(version))
+			}
 		})
 
 		It("parses an IETF draft style Version Negotiation Packet", func() {
@@ -141,7 +144,11 @@ var _ = Describe("Header", func() {
 			Expect(hdr.IsVersionNegotiation).To(BeTrue())
 			Expect(hdr.ConnectionID).To(Equal(protocol.ConnectionID(0x42)))
 			Expect(hdr.PacketNumber).To(Equal(protocol.PacketNumber(0x77)))
-			Expect(hdr.SupportedVersions).To(Equal(versions))
+			Expect(hdr.Version).To(BeZero())
+			// in addition to the versions, the supported versions might contain a reserved version number
+			for _, version := range versions {
+				Expect(hdr.SupportedVersions).To(ContainElement(version))
+			}
 		})
 	})
 

internal/wire/ietf_header_test.go

@@ -28,7 +28,9 @@ var _ = Describe("IETF draft Header", func() {
 				Expect(h.Version).To(BeZero())
 				Expect(h.ConnectionID).To(Equal(protocol.ConnectionID(0x1234567890)))
 				Expect(h.PacketNumber).To(Equal(protocol.PacketNumber(0x1337)))
-				Expect(h.SupportedVersions).To(Equal(versions))
+				for _, v := range versions {
+					Expect(h.SupportedVersions).To(ContainElement(v))
+				}
 			})
 
 			It("errors if it contains versions of the wrong length", func() {
@@ -42,7 +44,8 @@ var _ = Describe("IETF draft Header", func() {
 			It("errors if the version list is emtpy", func() {
 				versions := []protocol.VersionNumber{0x22334455}
 				data := ComposeVersionNegotiation(0x1234567890, 0x1337, versions)
-				_, err := parseHeader(bytes.NewReader(data[:len(data)-4]), protocol.PerspectiveServer)
+				// remove 8 bytes (two versions), since ComposeVersionNegotiation also added a reserved version number
+				_, err := parseHeader(bytes.NewReader(data[:len(data)-8]), protocol.PerspectiveServer)
 				Expect(err).To(MatchError("InvalidVersionNegotiationPacket: empty version list"))
 			})
 		})

internal/wire/public_header_test.go

@@ -97,14 +97,18 @@ var _ = Describe("Public Header", func() {
 				return data
 			}
 
-			It("parses version negotiation packets sent by the server", func() {
-				b := bytes.NewReader(ComposeGQUICVersionNegotiation(0x1337, protocol.SupportedVersions))
+			It("parses", func() {
+				versions := []protocol.VersionNumber{0x13, 0x37}
+				b := bytes.NewReader(ComposeGQUICVersionNegotiation(0x1337, versions))
 				hdr, err := parsePublicHeader(b, protocol.PerspectiveServer)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(hdr.VersionFlag).To(BeTrue())
 				Expect(hdr.Version).To(BeZero()) // unitialized
 				Expect(hdr.IsVersionNegotiation).To(BeTrue())
-				Expect(hdr.SupportedVersions).To(Equal(protocol.SupportedVersions))
+				// in addition to the versions, the supported versions might contain a reserved version number
+				for _, version := range versions {
+					Expect(hdr.SupportedVersions).To(ContainElement(version))
+				}
 				Expect(b.Len()).To(BeZero())
 			})
 

internal/wire/version_negotiation.go

@@ -21,9 +21,7 @@ func ComposeGQUICVersionNegotiation(connID protocol.ConnectionID, versions []pro
 		utils.Errorf("error composing version negotiation packet: %s", err.Error())
 		return nil
 	}
-	for _, v := range versions {
-		utils.BigEndian.WriteUint32(fullReply, uint32(v))
-	}
+	writeVersions(fullReply, versions)
 	return fullReply.Bytes()
 }
 
@@ -48,8 +46,14 @@ func ComposeVersionNegotiation(
 		utils.Errorf("error composing version negotiation packet: %s", err.Error())
 		return nil
 	}
-	for _, v := range versions {
-		utils.BigEndian.WriteUint32(fullReply, uint32(v))
-	}
+	writeVersions(fullReply, versions)
 	return fullReply.Bytes()
 }
+
+// writeVersions writes the versions for a Version Negotiation Packet.
+// It inserts one reserved version number at a random position.
+func writeVersions(buf *bytes.Buffer, supported []protocol.VersionNumber) {
+	for _, v := range protocol.GetGreasedVersions(supported) {
+		utils.BigEndian.WriteUint32(buf, uint32(v))
+	}
+}

internal/wire/version_negotiation_test.go

@@ -16,7 +16,11 @@ var _ = Describe("Version Negotiation Packets", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(hdr.VersionFlag).To(BeTrue())
 		Expect(hdr.ConnectionID).To(Equal(protocol.ConnectionID(0x1337)))
-		Expect(hdr.SupportedVersions).To(Equal(versions))
+		// the supported versions should include one reserved version number
+		Expect(hdr.SupportedVersions).To(HaveLen(len(versions) + 1))
+		for _, version := range versions {
+			Expect(hdr.SupportedVersions).To(ContainElement(version))
+		}
 	})
 
 	It("writes in IETF draft style", func() {
@@ -27,6 +31,11 @@ var _ = Describe("Version Negotiation Packets", func() {
 		Expect(hdr.IsVersionNegotiation).To(BeTrue())
 		Expect(hdr.ConnectionID).To(Equal(protocol.ConnectionID(0x1337)))
 		Expect(hdr.PacketNumber).To(Equal(protocol.PacketNumber(0x42)))
-		Expect(hdr.SupportedVersions).To(Equal(versions))
+		Expect(hdr.Version).To(BeZero())
+		// the supported versions should include one reserved version number
+		Expect(hdr.SupportedVersions).To(HaveLen(len(versions) + 1))
+		for _, version := range versions {
+			Expect(hdr.SupportedVersions).To(ContainElement(version))
+		}
 	})
 })