diff --git a/client.go b/client.go index adc078cde..e2c362123 100644 --- a/client.go +++ b/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 } diff --git a/integrationtests/self/handshake_rtt_test.go b/integrationtests/self/handshake_rtt_test.go index c2223f08c..48c723ed5 100644 --- a/integrationtests/self/handshake_rtt_test.go +++ b/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()) diff --git a/internal/handshake/crypto_setup_client.go b/internal/handshake/crypto_setup_client.go index 013309339..87b83ddd9 100644 --- a/internal/handshake/crypto_setup_client.go +++ b/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 } } diff --git a/internal/handshake/crypto_setup_client_test.go b/internal/handshake/crypto_setup_client_test.go index 07f094b32..ae9cf03c4 100644 --- a/internal/handshake/crypto_setup_client_test.go +++ b/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() { diff --git a/internal/handshake/tls_extension_handler_client.go b/internal/handshake/tls_extension_handler_client.go index 59f423102..4187804e2 100644 --- a/internal/handshake/tls_extension_handler_client.go +++ b/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 diff --git a/internal/handshake/tls_extension_handler_client_test.go b/internal/handshake/tls_extension_handler_client_test.go index 787ca6df9..a19ee94c9 100644 --- a/internal/handshake/tls_extension_handler_client_test.go +++ b/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) diff --git a/internal/protocol/version.go b/internal/protocol/version.go index 124e4a333..a25fbbbce 100644 --- a/internal/protocol/version.go +++ b/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 } diff --git a/internal/protocol/version_test.go b/internal/protocol/version_test.go index 12eb8f074..4fa8d4c45 100644 --- a/internal/protocol/version_test.go +++ b/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()) }) }) })