Merge pull request #927 from lucas-clemente/fix-925

fix client handshake failure when the server supports unknown versions

client.go

@@ -329,8 +329,8 @@ func (c *client) handleVersionNegotiationPacket(hdr *wire.Header) error {
 
 	c.receivedVersionNegotiationPacket = true
 
-	newVersion := protocol.ChooseSupportedVersion(c.config.Versions, hdr.SupportedVersions)
-	if newVersion == protocol.VersionUnsupported {
+	newVersion, ok := protocol.ChooseSupportedVersion(c.config.Versions, hdr.SupportedVersions)
+	if !ok {
 		return qerr.InvalidVersion
 	}
 

integrationtests/self/handshake_rtt_test.go

@@ -100,7 +100,18 @@ var _ = Describe("Handshake RTT tests", func() {
 	It("does version negotiation in 1 RTT", func() {
 		Expect(len(protocol.SupportedVersions)).To(BeNumerically(">", 1))
 		// the server doesn't support the highest supported version, which is the first one the client will try
-		serverConfig.Versions = protocol.SupportedVersions[1:]
+		serverConfig.Versions = []protocol.VersionNumber{protocol.SupportedVersions[1]}
+		runServerAndProxy()
+		_, err := quic.DialAddr(proxy.LocalAddr().String(), &tls.Config{InsecureSkipVerify: true}, nil)
+		Expect(err).ToNot(HaveOccurred())
+		expectDurationInRTTs(4)
+	})
+
+	It("does version negotiation, when the server supports more versions than the client", func() {
+		Expect(len(protocol.SupportedVersions)).To(BeNumerically(">", 1))
+		// the server doesn't support the highest supported version, which is the first one the client will try
+		// but it supports a bunch of versions that the client doesn't speak
+		serverConfig.Versions = []protocol.VersionNumber{protocol.SupportedVersions[1], 7, 8, 9}
 		runServerAndProxy()
 		_, err := quic.DialAddr(proxy.LocalAddr().String(), &tls.Config{InsecureSkipVerify: true}, nil)
 		Expect(err).ToNot(HaveOccurred())

internal/handshake/crypto_setup_client.go

@@ -283,24 +283,21 @@ func (h *cryptoSetupClient) handleSHLOMessage(cryptoData map[Tag][]byte) (*Trans
 }
 
 func (h *cryptoSetupClient) validateVersionList(verTags []byte) bool {
-	if len(h.negotiatedVersions) == 0 {
+	numNegotiatedVersions := len(h.negotiatedVersions)
+	if numNegotiatedVersions == 0 {
 		return true
 	}
-	if len(verTags)%4 != 0 || len(verTags)/4 != len(h.negotiatedVersions) {
+	if len(verTags)%4 != 0 || len(verTags)/4 != numNegotiatedVersions {
 		return false
 	}
 
 	b := bytes.NewReader(verTags)
-	for _, negotiatedVersion := range h.negotiatedVersions {
-		verTag, err := utils.BigEndian.ReadUint32(b)
+	for i := 0; i < numNegotiatedVersions; i++ {
+		v, err := utils.BigEndian.ReadUint32(b)
 		if err != nil { // should never occur, since the length was already checked
 			return false
 		}
-		ver := protocol.VersionNumber(verTag)
-		if !protocol.IsSupportedVersion(protocol.SupportedVersions, ver) {
-			ver = protocol.VersionUnsupported
-		}
-		if ver != negotiatedVersion {
+		if protocol.VersionNumber(v) != h.negotiatedVersions[i] {
 			return false
 		}
 	}

internal/handshake/crypto_setup_client_test.go

@@ -194,9 +194,9 @@ var _ = Describe("Client Crypto Setup", func() {
 				Expect(cs.validateVersionList([]byte{0})).To(BeTrue())
 			})
 
-			It("detects a downgrade attack if the number of versions is unequal", func() {
+			It("detects a downgrade attack if the number of versions is not equal", func() {
 				cs.negotiatedVersions = []protocol.VersionNumber{protocol.VersionWhatever}
-				Expect(cs.validateVersionList(bytes.Repeat([]byte{'f'}, 8))).To(BeFalse())
+				Expect(cs.validateVersionList(bytes.Repeat([]byte{'f'}, 2*4))).To(BeFalse())
 			})
 
 			It("detects a downgrade attack", func() {
@@ -208,17 +208,7 @@ var _ = Describe("Client Crypto Setup", func() {
 
 			It("errors if the version tags are invalid", func() {
 				cs.negotiatedVersions = []protocol.VersionNumber{protocol.VersionWhatever}
-				Expect(cs.validateVersionList([]byte{0, 1, 2})).To(BeFalse())
-			})
-
-			It("doesn't care about unsupported versions", func() {
-				ver := protocol.SupportedVersions[0]
-				cs.negotiatedVersions = []protocol.VersionNumber{protocol.VersionUnsupported, ver, protocol.VersionUnsupported}
-				b := &bytes.Buffer{}
-				b.Write([]byte{0, 0, 0, 0})
-				utils.BigEndian.WriteUint32(b, uint32(ver))
-				b.Write([]byte{0x13, 0x37, 0x13, 0x37})
-				Expect(cs.validateVersionList(b.Bytes())).To(BeTrue())
+				Expect(cs.validateVersionList([]byte{0, 1, 2})).To(BeFalse()) // 1 byte too short
 			})
 
 			It("returns the right error when detecting a downgrade attack", func() {

internal/handshake/tls_extension_handler_client.go

@@ -89,8 +89,11 @@ func (h *extensionHandlerClient) Receive(hType mint.HandshakeType, el *mint.Exte
 		return qerr.Error(qerr.VersionNegotiationMismatch, "current version not included in the supported versions")
 	}
 	// if version negotiation was performed, check that we would have selected the current version based on the supported versions sent by the server
-	if h.version != h.initialVersion && h.version != protocol.ChooseSupportedVersion(h.supportedVersions, serverSupportedVersions) {
-		return qerr.Error(qerr.VersionNegotiationMismatch, "would have picked a different version")
+	if h.version != h.initialVersion {
+		negotiatedVersion, ok := protocol.ChooseSupportedVersion(h.supportedVersions, serverSupportedVersions)
+		if !ok || h.version != negotiatedVersion {
+			return qerr.Error(qerr.VersionNegotiationMismatch, "would have picked a different version")
+		}
 	}
 
 	// check that the server sent the stateless reset token

internal/handshake/tls_extension_handler_client_test.go

@@ -167,7 +167,9 @@ var _ = Describe("TLS Extension Handler, for the client", func() {
 				handler.supportedVersions = []protocol.VersionNumber{43, 42, 41}
 				serverSupportedVersions := []protocol.VersionNumber{42, 43}
 				// check that version negotiation would have led us to pick version 43
-				Expect(protocol.ChooseSupportedVersion(handler.supportedVersions, serverSupportedVersions)).To(Equal(protocol.VersionNumber(43)))
+				ver, ok := protocol.ChooseSupportedVersion(handler.supportedVersions, serverSupportedVersions)
+				Expect(ok).To(BeTrue())
+				Expect(ver).To(Equal(protocol.VersionNumber(43)))
 				ssv := make([]uint32, len(serverSupportedVersions))
 				for i, v := range serverSupportedVersions {
 					ssv[i] = uint32(v)
@@ -188,7 +190,9 @@ var _ = Describe("TLS Extension Handler, for the client", func() {
 				handler.supportedVersions = []protocol.VersionNumber{43, 42, 41}
 				serverSupportedVersions := []protocol.VersionNumber{42, 43}
 				// check that version negotiation would have led us to pick version 43
-				Expect(protocol.ChooseSupportedVersion(handler.supportedVersions, serverSupportedVersions)).To(Equal(protocol.VersionNumber(43)))
+				ver, ok := protocol.ChooseSupportedVersion(handler.supportedVersions, serverSupportedVersions)
+				Expect(ok).To(BeTrue())
+				Expect(ver).To(Equal(protocol.VersionNumber(43)))
 				ssv := make([]uint32, len(serverSupportedVersions))
 				for i, v := range serverSupportedVersions {
 					ssv[i] = uint32(v)

internal/protocol/version.go

@@ -18,10 +18,9 @@ const (
 	Version37 VersionNumber = gquicVersion0 + 3*0x100 + 0x7 + iota
 	Version38
 	Version39
-	VersionTLS         VersionNumber = 101
-	VersionWhatever    VersionNumber = 0 // for when the version doesn't matter
-	VersionUnsupported VersionNumber = -1
-	VersionUnknown     VersionNumber = -2
+	VersionTLS      VersionNumber = 101
+	VersionWhatever VersionNumber = 0 // for when the version doesn't matter
+	VersionUnknown  VersionNumber = -1
 )
 
 // SupportedVersions lists the versions that the server supports
@@ -41,8 +40,6 @@ func (vn VersionNumber) String() string {
 	switch vn {
 	case VersionWhatever:
 		return "whatever"
-	case VersionUnsupported:
-		return "unsupported"
 	case VersionUnknown:
 		return "unknown"
 	case VersionTLS:
@@ -79,15 +76,15 @@ func IsSupportedVersion(supported []VersionNumber, v VersionNumber) bool {
 
 // ChooseSupportedVersion finds the best version in the overlap of ours and theirs
 // ours is a slice of versions that we support, sorted by our preference (descending)
-// theirs is a slice of versions offered by the peer. The order does not matter
-// if no suitable version is found, it returns VersionUnsupported
-func ChooseSupportedVersion(ours, theirs []VersionNumber) VersionNumber {
+// theirs is a slice of versions offered by the peer. The order does not matter.
+// The bool returned indicates if a matching version was found.
+func ChooseSupportedVersion(ours, theirs []VersionNumber) (VersionNumber, bool) {
 	for _, ourVer := range ours {
 		for _, theirVer := range theirs {
 			if ourVer == theirVer {
-				return ourVer
+				return ourVer, true
 			}
 		}
 	}
-	return VersionUnsupported
+	return 0, false
 }

internal/protocol/version_test.go

@@ -26,7 +26,6 @@ var _ = Describe("Version", func() {
 		Expect(Version39.String()).To(Equal("gQUIC 39"))
 		Expect(VersionTLS.String()).To(ContainSubstring("TLS"))
 		Expect(VersionWhatever.String()).To(Equal("whatever"))
-		Expect(VersionUnsupported.String()).To(Equal("unsupported"))
 		Expect(VersionUnknown.String()).To(Equal("unknown"))
 		// check with unsupported version numbers from the wiki
 		Expect(VersionNumber(0x51303039).String()).To(Equal("gQUIC 9"))
@@ -63,22 +62,31 @@ var _ = Describe("Version", func() {
 		It("finds the supported version", func() {
 			supportedVersions := []VersionNumber{1, 2, 3}
 			other := []VersionNumber{6, 5, 4, 3}
-			Expect(ChooseSupportedVersion(supportedVersions, other)).To(Equal(VersionNumber(3)))
+			ver, ok := ChooseSupportedVersion(supportedVersions, other)
+			Expect(ok).To(BeTrue())
+			Expect(ver).To(Equal(VersionNumber(3)))
 		})
 
 		It("picks the preferred version", func() {
 			supportedVersions := []VersionNumber{2, 1, 3}
 			other := []VersionNumber{3, 6, 1, 8, 2, 10}
-			Expect(ChooseSupportedVersion(supportedVersions, other)).To(Equal(VersionNumber(2)))
+			ver, ok := ChooseSupportedVersion(supportedVersions, other)
+			Expect(ok).To(BeTrue())
+			Expect(ver).To(Equal(VersionNumber(2)))
+		})
+
+		It("says when no matching version was found", func() {
+			_, ok := ChooseSupportedVersion([]VersionNumber{1}, []VersionNumber{2})
+			Expect(ok).To(BeFalse())
 		})
 
 		It("handles empty inputs", func() {
-			supportedVersions := []VersionNumber{102, 101}
-			Expect(ChooseSupportedVersion(supportedVersions, nil)).To(Equal(VersionUnsupported))
-			Expect(ChooseSupportedVersion(supportedVersions, []VersionNumber{})).To(Equal(VersionUnsupported))
-			supportedVersions = []VersionNumber{}
-			Expect(ChooseSupportedVersion(supportedVersions, []VersionNumber{1, 2})).To(Equal(VersionUnsupported))
-			Expect(ChooseSupportedVersion(supportedVersions, []VersionNumber{})).To(Equal(VersionUnsupported))
+			_, ok := ChooseSupportedVersion([]VersionNumber{102, 101}, []VersionNumber{})
+			Expect(ok).To(BeFalse())
+			_, ok = ChooseSupportedVersion([]VersionNumber{}, []VersionNumber{1, 2})
+			Expect(ok).To(BeFalse())
+			_, ok = ChooseSupportedVersion([]VersionNumber{}, []VersionNumber{})
+			Expect(ok).To(BeFalse())
 		})
 	})
 })