From 7c8715616e0c0aa610ee8c49323394471c447553 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 5 Dec 2017 10:04:44 +0700 Subject: [PATCH] update validation of version negotiation The negotiated_version parameter was recently moved from the client_hello TLS handshake message to the encrypted_extensions. --- internal/handshake/tls_extension.go | 6 ++--- .../handshake/tls_extension_handler_client.go | 9 ++++--- .../tls_extension_handler_client_test.go | 22 ++++++++++++++-- .../handshake/tls_extension_handler_server.go | 11 +++----- .../tls_extension_handler_server_test.go | 25 ++++++------------- 5 files changed, 40 insertions(+), 33 deletions(-) diff --git a/internal/handshake/tls_extension.go b/internal/handshake/tls_extension.go index 7e56e92b..15a96bb1 100644 --- a/internal/handshake/tls_extension.go +++ b/internal/handshake/tls_extension.go @@ -24,12 +24,12 @@ type transportParameter struct { } type clientHelloTransportParameters struct { - NegotiatedVersion uint32 // actually a protocol.VersionNumber - InitialVersion uint32 // actually a protocol.VersionNumber - Parameters []transportParameter `tls:"head=2"` + InitialVersion uint32 // actually a protocol.VersionNumber + Parameters []transportParameter `tls:"head=2"` } type encryptedExtensionsTransportParameters struct { + NegotiatedVersion uint32 // actually a protocol.VersionNumber SupportedVersions []uint32 `tls:"head=1"` // actually a protocol.VersionNumber Parameters []transportParameter `tls:"head=2"` } diff --git a/internal/handshake/tls_extension_handler_client.go b/internal/handshake/tls_extension_handler_client.go index 4187804e..506b0db2 100644 --- a/internal/handshake/tls_extension_handler_client.go +++ b/internal/handshake/tls_extension_handler_client.go @@ -45,9 +45,8 @@ func (h *extensionHandlerClient) Send(hType mint.HandshakeType, el *mint.Extensi } data, err := syntax.Marshal(clientHelloTransportParameters{ - NegotiatedVersion: uint32(h.version), - InitialVersion: uint32(h.initialVersion), - Parameters: h.params.getTransportParameters(), + InitialVersion: uint32(h.initialVersion), + Parameters: h.params.getTransportParameters(), }) if err != nil { return err @@ -84,6 +83,10 @@ func (h *extensionHandlerClient) Receive(hType mint.HandshakeType, el *mint.Exte for i, v := range eetp.SupportedVersions { serverSupportedVersions[i] = protocol.VersionNumber(v) } + // check that the negotiated_version is the current version + if protocol.VersionNumber(eetp.NegotiatedVersion) != h.version { + return qerr.Error(qerr.VersionNegotiationMismatch, "current version doesn't match negotiated_version") + } // check that the current version is included in the supported versions if !protocol.IsSupportedVersion(serverSupportedVersions, h.version) { return qerr.Error(qerr.VersionNegotiationMismatch, "current version not included in the supported versions") diff --git a/internal/handshake/tls_extension_handler_client_test.go b/internal/handshake/tls_extension_handler_client_test.go index a19ee94c..c70b9fdc 100644 --- a/internal/handshake/tls_extension_handler_client_test.go +++ b/internal/handshake/tls_extension_handler_client_test.go @@ -38,7 +38,6 @@ var _ = Describe("TLS Extension Handler, for the client", func() { It("adds TransportParameters to the ClientHello", func() { handler.initialVersion = 13 - handler.version = 37 err := handler.Send(mint.HandshakeTypeClientHello, &el) Expect(err).ToNot(HaveOccurred()) Expect(el).To(HaveLen(1)) @@ -49,7 +48,6 @@ var _ = Describe("TLS Extension Handler, for the client", func() { _, err = syntax.Unmarshal(ext.data, chtp) Expect(err).ToNot(HaveOccurred()) Expect(chtp.InitialVersion).To(BeEquivalentTo(13)) - Expect(chtp.NegotiatedVersion).To(BeEquivalentTo(37)) }) }) @@ -140,6 +138,7 @@ var _ = Describe("TLS Extension Handler, for the client", func() { handler.supportedVersions = []protocol.VersionNumber{13, 37, 42} body, err := syntax.Marshal(encryptedExtensionsTransportParameters{ Parameters: parameterMapToList(parameters), + NegotiatedVersion: 37, SupportedVersions: []uint32{36, 37, 38}, }) Expect(err).ToNot(HaveOccurred()) @@ -149,9 +148,26 @@ var _ = Describe("TLS Extension Handler, for the client", func() { Expect(err).ToNot(HaveOccurred()) }) + It("errors if the current version doesn't match negotiated_version", func() { + handler.initialVersion = 13 + handler.version = 37 + handler.supportedVersions = []protocol.VersionNumber{13, 37, 42} + body, err := syntax.Marshal(encryptedExtensionsTransportParameters{ + Parameters: parameterMapToList(parameters), + NegotiatedVersion: 38, + SupportedVersions: []uint32{36, 37, 38}, + }) + Expect(err).ToNot(HaveOccurred()) + err = el.Add(&tlsExtensionBody{data: body}) + Expect(err).ToNot(HaveOccurred()) + err = handler.Receive(mint.HandshakeTypeEncryptedExtensions, &el) + Expect(err).To(MatchError("VersionNegotiationMismatch: current version doesn't match negotiated_version")) + }) + It("errors if the current version is not contained in the server's supported versions", func() { handler.version = 42 body, err := syntax.Marshal(encryptedExtensionsTransportParameters{ + NegotiatedVersion: 42, SupportedVersions: []uint32{43, 44}, }) Expect(err).ToNot(HaveOccurred()) @@ -175,6 +191,7 @@ var _ = Describe("TLS Extension Handler, for the client", func() { ssv[i] = uint32(v) } body, err := syntax.Marshal(encryptedExtensionsTransportParameters{ + NegotiatedVersion: 42, SupportedVersions: ssv, }) Expect(err).ToNot(HaveOccurred()) @@ -199,6 +216,7 @@ var _ = Describe("TLS Extension Handler, for the client", func() { } body, err := syntax.Marshal(encryptedExtensionsTransportParameters{ Parameters: parameterMapToList(parameters), + NegotiatedVersion: 42, SupportedVersions: ssv, }) Expect(err).ToNot(HaveOccurred()) diff --git a/internal/handshake/tls_extension_handler_server.go b/internal/handshake/tls_extension_handler_server.go index 49830d8d..0324f852 100644 --- a/internal/handshake/tls_extension_handler_server.go +++ b/internal/handshake/tls_extension_handler_server.go @@ -52,6 +52,7 @@ func (h *extensionHandlerServer) Send(hType mint.HandshakeType, el *mint.Extensi supportedVersions[i] = uint32(v) } data, err := syntax.Marshal(encryptedExtensionsTransportParameters{ + NegotiatedVersion: uint32(h.version), SupportedVersions: supportedVersions, Parameters: transportParams, }) @@ -80,15 +81,11 @@ func (h *extensionHandlerServer) Receive(hType mint.HandshakeType, el *mint.Exte return err } initialVersion := protocol.VersionNumber(chtp.InitialVersion) - negotiatedVersion := protocol.VersionNumber(chtp.NegotiatedVersion) - // check that the negotiated version is the version we're currently using - if negotiatedVersion != h.version { - return qerr.Error(qerr.VersionNegotiationMismatch, "Inconsistent negotiated version") - } + // perform the stateless version negotiation validation: // make sure that we would have sent a Version Negotiation Packet if the client offered the initial version - // this is the case when the initial version is not contained in the supported versions - if initialVersion != negotiatedVersion && protocol.IsSupportedVersion(h.supportedVersions, initialVersion) { + // this is the case if and only if the initial version is not contained in the supported versions + if initialVersion != h.version && protocol.IsSupportedVersion(h.supportedVersions, initialVersion) { return qerr.Error(qerr.VersionNegotiationMismatch, "Client should have used the initial version") } diff --git a/internal/handshake/tls_extension_handler_server_test.go b/internal/handshake/tls_extension_handler_server_test.go index 27689536..7945bb8b 100644 --- a/internal/handshake/tls_extension_handler_server_test.go +++ b/internal/handshake/tls_extension_handler_server_test.go @@ -44,6 +44,7 @@ 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} err := handler.Send(mint.HandshakeTypeEncryptedExtensions, &el) Expect(err).ToNot(HaveOccurred()) @@ -55,6 +56,7 @@ var _ = Describe("TLS Extension Handler, for the server", func() { _, 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)) }) }) @@ -122,9 +124,8 @@ var _ = Describe("TLS Extension Handler, for the server", func() { It("accepts a ClientHello, when no version negotiation was performed", func() { handler.version = 42 body, err := syntax.Marshal(clientHelloTransportParameters{ - NegotiatedVersion: 42, - InitialVersion: 42, - Parameters: parameterMapToList(parameters), + InitialVersion: 42, + Parameters: parameterMapToList(parameters), }) Expect(err).ToNot(HaveOccurred()) err = el.Add(&tlsExtensionBody{data: body}) @@ -137,9 +138,8 @@ var _ = Describe("TLS Extension Handler, for the server", func() { handler.version = 42 handler.supportedVersions = []protocol.VersionNumber{13, 37, 42} body, err := syntax.Marshal(clientHelloTransportParameters{ - NegotiatedVersion: 42, - InitialVersion: 22, // this must be an unsupported version - Parameters: parameterMapToList(parameters), + InitialVersion: 22, // this must be an unsupported version + Parameters: parameterMapToList(parameters), }) Expect(err).ToNot(HaveOccurred()) err = el.Add(&tlsExtensionBody{data: body}) @@ -148,22 +148,11 @@ var _ = Describe("TLS Extension Handler, for the server", func() { Expect(err).ToNot(HaveOccurred()) }) - It("errors when the NegotiatedVersion field doesn't match the current version", func() { - handler.version = 42 - body, err := syntax.Marshal(clientHelloTransportParameters{NegotiatedVersion: 43}) - Expect(err).ToNot(HaveOccurred()) - err = el.Add(&tlsExtensionBody{data: body}) - Expect(err).ToNot(HaveOccurred()) - err = handler.Receive(mint.HandshakeTypeClientHello, &el) - Expect(err).To(MatchError("VersionNegotiationMismatch: Inconsistent negotiated version")) - }) - It("errros when a version negotiation was performed, although we already support the inital version", func() { handler.supportedVersions = []protocol.VersionNumber{11, 12, 13} handler.version = 13 body, err := syntax.Marshal(clientHelloTransportParameters{ - NegotiatedVersion: 13, - InitialVersion: 11, // this is an supported version + InitialVersion: 11, // this is an supported version }) Expect(err).ToNot(HaveOccurred()) err = el.Add(&tlsExtensionBody{data: body})