From d44c81de7ac0c251c91988f3b299b517a0221468 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 23 Mar 2019 11:06:13 +0100 Subject: [PATCH] remove verification of version negotiation --- internal/handshake/crypto_setup.go | 12 +- internal/handshake/crypto_setup_test.go | 50 ++---- internal/handshake/tls_extension.go | 96 ------------ internal/handshake/tls_extension_handler.go | 2 + internal/handshake/tls_extension_test.go | 69 --------- .../handshake/transport_parameter_test.go | 73 +++++---- internal/handshake/transport_parameters.go | 24 ++- session.go | 45 +----- session_test.go | 142 ++---------------- 9 files changed, 103 insertions(+), 410 deletions(-) delete mode 100644 internal/handshake/tls_extension.go delete mode 100644 internal/handshake/tls_extension_test.go diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index eda65e48..6dbaa562 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -112,7 +112,7 @@ func NewCryptoSetupClient( handshakeStream io.Writer, oneRTTStream io.Writer, connID protocol.ConnectionID, - chtp *ClientHelloTransportParameters, + tp *TransportParameters, handleParams func([]byte), tlsConf *tls.Config, logger utils.Logger, @@ -122,7 +122,7 @@ func NewCryptoSetupClient( handshakeStream, oneRTTStream, connID, - chtp.Marshal(), + tp, handleParams, tlsConf, logger, @@ -141,7 +141,7 @@ func NewCryptoSetupServer( handshakeStream io.Writer, oneRTTStream io.Writer, connID protocol.ConnectionID, - eetp *EncryptedExtensionsTransportParameters, + tp *TransportParameters, handleParams func([]byte), tlsConf *tls.Config, logger utils.Logger, @@ -151,7 +151,7 @@ func NewCryptoSetupServer( handshakeStream, oneRTTStream, connID, - eetp.Marshal(), + tp, handleParams, tlsConf, logger, @@ -169,7 +169,7 @@ func newCryptoSetup( handshakeStream io.Writer, oneRTTStream io.Writer, connID protocol.ConnectionID, - paramBytes []byte, // the marshaled transport parameters + tp *TransportParameters, handleParams func([]byte), tlsConf *tls.Config, logger utils.Logger, @@ -179,7 +179,7 @@ func newCryptoSetup( if err != nil { return nil, nil, err } - extHandler := newExtensionHandler(paramBytes, perspective) + extHandler := newExtensionHandler(tp.Marshal(), perspective) cs := &cryptoSetup{ initialStream: initialStream, initialSealer: initialSealer, diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go index 614b7bc0..1069a98a 100644 --- a/internal/handshake/crypto_setup_test.go +++ b/internal/handshake/crypto_setup_test.go @@ -84,10 +84,7 @@ var _ = Describe("Crypto Setup TLS", func() { &bytes.Buffer{}, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: protocol.VersionTLS, - SupportedVersions: []protocol.VersionNumber{protocol.VersionTLS}, - }, + &TransportParameters{}, func([]byte) {}, tlsConf, utils.DefaultLogger.WithPrefix("server"), @@ -114,10 +111,7 @@ var _ = Describe("Crypto Setup TLS", func() { sHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: protocol.VersionTLS, - SupportedVersions: []protocol.VersionNumber{protocol.VersionTLS}, - }, + &TransportParameters{}, func([]byte) {}, testdata.GetTLSConfig(), utils.DefaultLogger.WithPrefix("server"), @@ -150,10 +144,7 @@ var _ = Describe("Crypto Setup TLS", func() { sHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: protocol.VersionTLS, - SupportedVersions: []protocol.VersionNumber{protocol.VersionTLS}, - }, + &TransportParameters{}, func([]byte) {}, testdata.GetTLSConfig(), utils.DefaultLogger.WithPrefix("server"), @@ -180,10 +171,7 @@ var _ = Describe("Crypto Setup TLS", func() { sHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: protocol.VersionTLS, - SupportedVersions: []protocol.VersionNumber{protocol.VersionTLS}, - }, + &TransportParameters{}, func([]byte) {}, testdata.GetTLSConfig(), utils.DefaultLogger.WithPrefix("server"), @@ -261,9 +249,7 @@ var _ = Describe("Crypto Setup TLS", func() { cHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &ClientHelloTransportParameters{ - InitialVersion: protocol.VersionTLS, - }, + &TransportParameters{}, func([]byte) {}, clientConf, utils.DefaultLogger.WithPrefix("client"), @@ -277,11 +263,7 @@ var _ = Describe("Crypto Setup TLS", func() { sHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: protocol.VersionTLS, - SupportedVersions: []protocol.VersionNumber{protocol.VersionTLS}, - Parameters: TransportParameters{StatelessResetToken: &token}, - }, + &TransportParameters{StatelessResetToken: &token}, func([]byte) {}, serverConf, utils.DefaultLogger.WithPrefix("server"), @@ -322,9 +304,7 @@ var _ = Describe("Crypto Setup TLS", func() { cHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &ClientHelloTransportParameters{ - InitialVersion: protocol.VersionTLS, - }, + &TransportParameters{}, func([]byte) {}, &tls.Config{InsecureSkipVerify: true}, utils.DefaultLogger.WithPrefix("client"), @@ -360,7 +340,7 @@ var _ = Describe("Crypto Setup TLS", func() { cHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &ClientHelloTransportParameters{Parameters: *cTransportParameters}, + cTransportParameters, func(p []byte) { sTransportParametersRcvd = p }, clientConf, utils.DefaultLogger.WithPrefix("client"), @@ -378,7 +358,7 @@ var _ = Describe("Crypto Setup TLS", func() { sHandshakeStream, ioutil.Discard, protocol.ConnectionID{}, - &EncryptedExtensionsTransportParameters{Parameters: *sTransportParameters}, + sTransportParameters, func(p []byte) { cTransportParametersRcvd = p }, testdata.GetTLSConfig(), utils.DefaultLogger.WithPrefix("server"), @@ -395,13 +375,13 @@ var _ = Describe("Crypto Setup TLS", func() { }() Eventually(done).Should(BeClosed()) Expect(cTransportParametersRcvd).ToNot(BeNil()) - chtp := &ClientHelloTransportParameters{} - Expect(chtp.Unmarshal(cTransportParametersRcvd)).To(Succeed()) - Expect(chtp.Parameters.IdleTimeout).To(Equal(cTransportParameters.IdleTimeout)) + clTP := &TransportParameters{} + Expect(clTP.Unmarshal(cTransportParametersRcvd, protocol.PerspectiveClient)).To(Succeed()) + Expect(clTP.IdleTimeout).To(Equal(cTransportParameters.IdleTimeout)) Expect(sTransportParametersRcvd).ToNot(BeNil()) - eetp := &EncryptedExtensionsTransportParameters{} - Expect(eetp.Unmarshal(sTransportParametersRcvd)).To(Succeed()) - Expect(eetp.Parameters.IdleTimeout).To(Equal(sTransportParameters.IdleTimeout)) + srvTP := &TransportParameters{} + Expect(srvTP.Unmarshal(sTransportParametersRcvd, protocol.PerspectiveServer)).To(Succeed()) + Expect(srvTP.IdleTimeout).To(Equal(sTransportParameters.IdleTimeout)) }) }) }) diff --git a/internal/handshake/tls_extension.go b/internal/handshake/tls_extension.go deleted file mode 100644 index 86283705..00000000 --- a/internal/handshake/tls_extension.go +++ /dev/null @@ -1,96 +0,0 @@ -package handshake - -import ( - "bytes" - "encoding/binary" - "errors" - "fmt" - - "github.com/lucas-clemente/quic-go/internal/protocol" - "github.com/lucas-clemente/quic-go/internal/utils" -) - -const quicTLSExtensionType = 0xffa5 - -// The ClientHelloTransportParameters are the transport parameters sent in the ClientHello. -type ClientHelloTransportParameters struct { - InitialVersion protocol.VersionNumber - Parameters TransportParameters -} - -// Marshal the transport parameters -func (p *ClientHelloTransportParameters) Marshal() []byte { - const lenOffset = 4 - b := &bytes.Buffer{} - utils.BigEndian.WriteUint32(b, uint32(p.InitialVersion)) - b.Write([]byte{0, 0}) // length. Will be replaced later - p.Parameters.marshal(b) - data := b.Bytes() - binary.BigEndian.PutUint16(data[lenOffset:lenOffset+2], uint16(len(data)-lenOffset-2)) - return data -} - -// Unmarshal the transport parameters -func (p *ClientHelloTransportParameters) Unmarshal(data []byte) error { - if len(data) < 6 { - return errors.New("transport parameter data too short") - } - p.InitialVersion = protocol.VersionNumber(binary.BigEndian.Uint32(data[:4])) - paramsLen := int(binary.BigEndian.Uint16(data[4:6])) - data = data[6:] - if len(data) != paramsLen { - return fmt.Errorf("expected transport parameters to be %d bytes long, have %d", paramsLen, len(data)) - } - return p.Parameters.unmarshal(data, protocol.PerspectiveClient) -} - -// EncryptedExtensionsTransportParameters are the transport parameters sent in the EncryptedExtensions. -type EncryptedExtensionsTransportParameters struct { - NegotiatedVersion protocol.VersionNumber - SupportedVersions []protocol.VersionNumber - Parameters TransportParameters -} - -// Marshal the transport parameters -func (p *EncryptedExtensionsTransportParameters) Marshal() []byte { - b := &bytes.Buffer{} - utils.BigEndian.WriteUint32(b, uint32(p.NegotiatedVersion)) - b.WriteByte(uint8(4 * len(p.SupportedVersions))) - for _, v := range p.SupportedVersions { - utils.BigEndian.WriteUint32(b, uint32(v)) - } - lenOffset := b.Len() - b.Write([]byte{0, 0}) // length. Will be replaced later - p.Parameters.marshal(b) - data := b.Bytes() - binary.BigEndian.PutUint16(data[lenOffset:lenOffset+2], uint16(len(data)-lenOffset-2)) - return data -} - -// Unmarshal the transport parameters -func (p *EncryptedExtensionsTransportParameters) Unmarshal(data []byte) error { - if len(data) < 5 { - return errors.New("transport parameter data too short") - } - p.NegotiatedVersion = protocol.VersionNumber(binary.BigEndian.Uint32(data[:4])) - numVersions := int(data[4]) - if numVersions%4 != 0 { - return fmt.Errorf("invalid length for version list: %d", numVersions) - } - numVersions /= 4 - data = data[5:] - if len(data) < 4*numVersions+2 /*length field for the parameter list */ { - return errors.New("transport parameter data too short") - } - p.SupportedVersions = make([]protocol.VersionNumber, numVersions) - for i := 0; i < numVersions; i++ { - p.SupportedVersions[i] = protocol.VersionNumber(binary.BigEndian.Uint32(data[:4])) - data = data[4:] - } - paramsLen := int(binary.BigEndian.Uint16(data[:2])) - data = data[2:] - if len(data) != paramsLen { - return fmt.Errorf("expected transport parameters to be %d bytes long, have %d", paramsLen, len(data)) - } - return p.Parameters.unmarshal(data, protocol.PerspectiveServer) -} diff --git a/internal/handshake/tls_extension_handler.go b/internal/handshake/tls_extension_handler.go index 73dba9b2..590aafd1 100644 --- a/internal/handshake/tls_extension_handler.go +++ b/internal/handshake/tls_extension_handler.go @@ -5,6 +5,8 @@ import ( "github.com/marten-seemann/qtls" ) +const quicTLSExtensionType = 0xffa5 + type extensionHandler struct { ourParams []byte paramsChan chan []byte diff --git a/internal/handshake/tls_extension_test.go b/internal/handshake/tls_extension_test.go deleted file mode 100644 index e9f9b61f..00000000 --- a/internal/handshake/tls_extension_test.go +++ /dev/null @@ -1,69 +0,0 @@ -package handshake - -import ( - "math/rand" - "time" - - "github.com/lucas-clemente/quic-go/internal/protocol" - - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("QUIC TLS Extension", func() { - Context("Client Hello Transport Parameters", func() { - It("marshals and unmarshals", func() { - chtp := &ClientHelloTransportParameters{ - InitialVersion: 0x123456, - Parameters: TransportParameters{ - InitialMaxStreamDataUni: 0x42, - IdleTimeout: 0x1337 * time.Second, - }, - } - chtp2 := &ClientHelloTransportParameters{} - Expect(chtp2.Unmarshal(chtp.Marshal())).To(Succeed()) - Expect(chtp2.InitialVersion).To(Equal(chtp.InitialVersion)) - Expect(chtp2.Parameters.InitialMaxStreamDataUni).To(Equal(chtp.Parameters.InitialMaxStreamDataUni)) - Expect(chtp2.Parameters.IdleTimeout).To(Equal(chtp.Parameters.IdleTimeout)) - }) - - It("fuzzes", func() { - rand := rand.New(rand.NewSource(GinkgoRandomSeed())) - b := make([]byte, 100) - for i := 0; i < 1000; i++ { - rand.Read(b) - chtp := &ClientHelloTransportParameters{} - chtp.Unmarshal(b[:int(rand.Int31n(100))]) - } - }) - }) - - Context("Encrypted Extensions Transport Parameters", func() { - It("marshals and unmarshals", func() { - eetp := &EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 0x123456, - SupportedVersions: []protocol.VersionNumber{0x42, 0x4242}, - Parameters: TransportParameters{ - InitialMaxStreamDataBidiLocal: 0x42, - IdleTimeout: 0x1337 * time.Second, - }, - } - eetp2 := &EncryptedExtensionsTransportParameters{} - Expect(eetp2.Unmarshal(eetp.Marshal())).To(Succeed()) - Expect(eetp2.NegotiatedVersion).To(Equal(eetp.NegotiatedVersion)) - Expect(eetp2.SupportedVersions).To(Equal(eetp.SupportedVersions)) - Expect(eetp2.Parameters.InitialMaxStreamDataBidiLocal).To(Equal(eetp.Parameters.InitialMaxStreamDataBidiLocal)) - Expect(eetp2.Parameters.IdleTimeout).To(Equal(eetp.Parameters.IdleTimeout)) - }) - - It("fuzzes", func() { - rand := rand.New(rand.NewSource(GinkgoRandomSeed())) - b := make([]byte, 100) - for i := 0; i < 1000; i++ { - rand.Read(b) - chtp := &EncryptedExtensionsTransportParameters{} - chtp.Unmarshal(b[:int(rand.Int31n(100))]) - } - }) - }) -}) diff --git a/internal/handshake/transport_parameter_test.go b/internal/handshake/transport_parameter_test.go index f295f7a3..84cc932e 100644 --- a/internal/handshake/transport_parameter_test.go +++ b/internal/handshake/transport_parameter_test.go @@ -2,6 +2,7 @@ package handshake import ( "bytes" + "encoding/binary" "math" "math/rand" "time" @@ -13,6 +14,12 @@ import ( ) var _ = Describe("Transport Parameters", func() { + prependLength := func(tp []byte) []byte { + data := make([]byte, 2) + binary.BigEndian.PutUint16(data, uint16(len(tp))) + return append(data, tp...) + } + It("has a string representation", func() { p := &TransportParameters{ InitialMaxStreamDataBidiLocal: 0x1234, @@ -50,7 +57,7 @@ var _ = Describe("Transport Parameters", func() { return uint64(rand.Int63n(maxVals[int(rand.Int31n(4))])) } - It("marshals und unmarshals", func() { + It("marshals and unmarshals", func() { var token [16]byte rand.Read(token[:]) params := &TransportParameters{ @@ -66,11 +73,10 @@ var _ = Describe("Transport Parameters", func() { OriginalConnectionID: protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef}, AckDelayExponent: 13, } - b := &bytes.Buffer{} - params.marshal(b) + data := params.Marshal() p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(Succeed()) + Expect(p.Unmarshal(data, protocol.PerspectiveServer)).To(Succeed()) Expect(p.InitialMaxStreamDataBidiLocal).To(Equal(params.InitialMaxStreamDataBidiLocal)) Expect(p.InitialMaxStreamDataBidiRemote).To(Equal(params.InitialMaxStreamDataBidiRemote)) Expect(p.InitialMaxStreamDataUni).To(Equal(params.InitialMaxStreamDataUni)) @@ -84,13 +90,24 @@ var _ = Describe("Transport Parameters", func() { Expect(p.AckDelayExponent).To(Equal(uint8(13))) }) + It("errors if the transport parameters are too short to contain the length", func() { + Expect((&TransportParameters{}).Unmarshal([]byte{0}, protocol.PerspectiveClient)).To(MatchError("transport parameter data too short")) + }) + + It("errors if the transport parameters are too short to contain the length", func() { + data := make([]byte, 2) + binary.BigEndian.PutUint16(data, 42) + data = append(data, make([]byte, 41)...) + Expect((&TransportParameters{}).Unmarshal(data, protocol.PerspectiveClient)).To(MatchError("expected transport parameters to be 42 bytes long, have 41")) + }) + It("errors when the stateless_reset_token has the wrong length", func() { b := &bytes.Buffer{} utils.BigEndian.WriteUint16(b, uint16(statelessResetTokenParameterID)) utils.BigEndian.WriteUint16(b, 15) b.Write(make([]byte, 15)) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("wrong length for stateless_reset_token: 15 (expected 16)")) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(MatchError("wrong length for stateless_reset_token: 15 (expected 16)")) }) It("errors when the max_packet_size is too small", func() { @@ -99,7 +116,7 @@ var _ = Describe("Transport Parameters", func() { utils.BigEndian.WriteUint16(b, uint16(utils.VarIntLen(1199))) utils.WriteVarInt(b, 1199) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("invalid value for max_packet_size: 1199 (minimum 1200)")) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(MatchError("invalid value for max_packet_size: 1199 (minimum 1200)")) }) It("errors when disable_migration has content", func() { @@ -108,30 +125,26 @@ var _ = Describe("Transport Parameters", func() { utils.BigEndian.WriteUint16(b, 6) b.Write([]byte("foobar")) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("wrong length for disable_migration: 6 (expected empty)")) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(MatchError("wrong length for disable_migration: 6 (expected empty)")) }) It("errors when the ack_delay_exponenent is too large", func() { - b := &bytes.Buffer{} - (&TransportParameters{AckDelayExponent: 21}).marshal(b) + data := (&TransportParameters{AckDelayExponent: 21}).Marshal() p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("invalid value for ack_delay_exponent: 21 (maximum 20)")) + Expect(p.Unmarshal(data, protocol.PerspectiveServer)).To(MatchError("invalid value for ack_delay_exponent: 21 (maximum 20)")) }) It("doesn't send the ack_delay_exponent, if it has the default value", func() { - b := &bytes.Buffer{} - (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent}).marshal(b) - defaultLen := b.Len() - b.Reset() - (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent + 1}).marshal(b) - Expect(b.Len()).To(Equal(defaultLen + 2 /* parameter ID */ + 2 /* length field */ + 1 /* value */)) + dataDefault := (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent}).Marshal() + defaultLen := len(dataDefault) + data := (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent + 1}).Marshal() + Expect(len(data)).To(Equal(defaultLen + 2 /* parameter ID */ + 2 /* length field */ + 1 /* value */)) }) It("sets the default value for the ack_delay_exponent, when no value was sent", func() { - b := &bytes.Buffer{} - (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent}).marshal(b) + data := (&TransportParameters{AckDelayExponent: protocol.DefaultAckDelayExponent}).Marshal() p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(Succeed()) + Expect(p.Unmarshal(data, protocol.PerspectiveServer)).To(Succeed()) Expect(p.AckDelayExponent).To(BeEquivalentTo(protocol.DefaultAckDelayExponent)) }) @@ -143,7 +156,7 @@ var _ = Describe("Transport Parameters", func() { Expect(utils.VarIntLen(val)).ToNot(BeEquivalentTo(2)) utils.WriteVarInt(b, val) p := &TransportParameters{} - err := p.unmarshal(b.Bytes(), protocol.PerspectiveServer) + err := p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("inconsistent transport parameter length")) }) @@ -163,7 +176,7 @@ var _ = Describe("Transport Parameters", func() { utils.BigEndian.WriteUint16(b, uint16(utils.VarIntLen(0x42))) utils.WriteVarInt(b, 0x42) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(Succeed()) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(Succeed()) Expect(p.InitialMaxStreamDataBidiLocal).To(Equal(protocol.ByteCount(0x1337))) Expect(p.InitialMaxStreamDataBidiRemote).To(Equal(protocol.ByteCount(0x42))) }) @@ -183,7 +196,7 @@ var _ = Describe("Transport Parameters", func() { utils.BigEndian.WriteUint16(b, uint16(utils.VarIntLen(0x1337))) utils.WriteVarInt(b, 0x1337) p := &TransportParameters{} - err := p.unmarshal(b.Bytes(), protocol.PerspectiveServer) + err := p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("received duplicate transport parameter")) }) @@ -194,7 +207,7 @@ var _ = Describe("Transport Parameters", func() { utils.BigEndian.WriteUint16(b, 7) b.Write([]byte("foobar")) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("remaining length (6) smaller than parameter length (7)")) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(MatchError("remaining length (6) smaller than parameter length (7)")) }) It("errors if there's unprocessed data after reading", func() { @@ -204,25 +217,21 @@ var _ = Describe("Transport Parameters", func() { utils.WriteVarInt(b, 0x1337) b.Write([]byte("foo")) p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveServer)).To(MatchError("should have read all data. Still have 3 bytes")) + Expect(p.Unmarshal(prependLength(b.Bytes()), protocol.PerspectiveServer)).To(MatchError("should have read all data. Still have 3 bytes")) }) It("errors if the client sent a stateless_reset_token", func() { var token [16]byte params := &TransportParameters{StatelessResetToken: &token} - b := &bytes.Buffer{} - params.marshal(b) - p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveClient)).To(MatchError("client sent a stateless_reset_token")) + data := params.Marshal() + Expect((&TransportParameters{}).Unmarshal(data, protocol.PerspectiveClient)).To(MatchError("client sent a stateless_reset_token")) }) It("errors if the client sent a stateless_reset_token", func() { params := &TransportParameters{ OriginalConnectionID: protocol.ConnectionID{0xca, 0xfe}, } - b := &bytes.Buffer{} - params.marshal(b) - p := &TransportParameters{} - Expect(p.unmarshal(b.Bytes(), protocol.PerspectiveClient)).To(MatchError("client sent an original_connection_id")) + data := params.Marshal() + Expect((&TransportParameters{}).Unmarshal(data, protocol.PerspectiveClient)).To(MatchError("client sent an original_connection_id")) }) }) diff --git a/internal/handshake/transport_parameters.go b/internal/handshake/transport_parameters.go index 22fe0671..6810cbc3 100644 --- a/internal/handshake/transport_parameters.go +++ b/internal/handshake/transport_parameters.go @@ -2,6 +2,7 @@ package handshake import ( "bytes" + "encoding/binary" "errors" "fmt" "io" @@ -50,13 +51,22 @@ type TransportParameters struct { OriginalConnectionID protocol.ConnectionID } -func (p *TransportParameters) unmarshal(data []byte, sentBy protocol.Perspective) error { +// Unmarshal the transport parameters +func (p *TransportParameters) Unmarshal(data []byte, sentBy protocol.Perspective) error { + if len(data) < 2 { + return errors.New("transport parameter data too short") + } + length := binary.BigEndian.Uint16(data[:2]) + if len(data)-2 < int(length) { + return fmt.Errorf("expected transport parameters to be %d bytes long, have %d", length, len(data)-2) + } + // needed to check that every parameter is only sent at most once var parameterIDs []transportParameterID var readAckDelayExponent bool - r := bytes.NewReader(data) + r := bytes.NewReader(data[2:]) for r.Len() >= 4 { paramIDInt, _ := utils.BigEndian.ReadUint16(r) paramID := transportParameterID(paramIDInt) @@ -170,7 +180,11 @@ func (p *TransportParameters) readNumericTransportParameter( return nil } -func (p *TransportParameters) marshal(b *bytes.Buffer) { +// Marshal the transport parameters +func (p *TransportParameters) Marshal() []byte { + b := &bytes.Buffer{} + b.Write([]byte{0, 0}) // length. Will be replaced later + // initial_max_stream_data_bidi_local utils.BigEndian.WriteUint16(b, uint16(initialMaxStreamDataBidiLocalParameterID)) utils.BigEndian.WriteUint16(b, uint16(utils.VarIntLen(uint64(p.InitialMaxStreamDataBidiLocal)))) @@ -227,6 +241,10 @@ func (p *TransportParameters) marshal(b *bytes.Buffer) { utils.BigEndian.WriteUint16(b, uint16(p.OriginalConnectionID.Len())) b.Write(p.OriginalConnectionID.Bytes()) } + + data := b.Bytes() + binary.BigEndian.PutUint16(data[:2], uint16(b.Len()-2)) + return data } // String returns a string representation, intended for logging. diff --git a/session.go b/session.go index 68eb8fab..22ad7805 100644 --- a/session.go +++ b/session.go @@ -191,17 +191,12 @@ var newSession = func( initialStream := newCryptoStream() handshakeStream := newCryptoStream() oneRTTStream := newPostHandshakeCryptoStream(s.framer) - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: s.version, - SupportedVersions: protocol.GetGreasedVersions(conf.Versions), - Parameters: *params, - } cs, err := handshake.NewCryptoSetupServer( initialStream, handshakeStream, oneRTTStream, clientDestConnID, - eetp, + params, s.processTransportParameters, tlsConf, logger, @@ -263,16 +258,12 @@ var newClientSession = func( initialStream := newCryptoStream() handshakeStream := newCryptoStream() oneRTTStream := newPostHandshakeCryptoStream(s.framer) - chtp := &handshake.ClientHelloTransportParameters{ - InitialVersion: initialVersion, - Parameters: *params, - } cs, clientHelloWritten, err := handshake.NewCryptoSetupClient( initialStream, handshakeStream, oneRTTStream, s.destConnID, - chtp, + params, s.processTransportParameters, tlsConf, logger, @@ -949,27 +940,11 @@ func (s *session) processTransportParameters(data []byte) { } func (s *session) processTransportParametersForClient(data []byte) (*handshake.TransportParameters, error) { - eetp := handshake.EncryptedExtensionsTransportParameters{} - if err := eetp.Unmarshal(data); err != nil { + params := &handshake.TransportParameters{} + if err := params.Unmarshal(data, s.perspective.Opposite()); err != nil { return nil, err } - // check that the negotiated_version is the current version - if eetp.NegotiatedVersion != s.version { - return nil, qerr.Error(qerr.VersionNegotiationError, "current version doesn't match negotiated_version") - } - // check that the current version is included in the supported versions - if !protocol.IsSupportedVersion(eetp.SupportedVersions, s.version) { - return nil, qerr.Error(qerr.VersionNegotiationError, "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 s.version != s.initialVersion { - negotiatedVersion, ok := protocol.ChooseSupportedVersion(s.config.Versions, eetp.SupportedVersions) - if !ok || s.version != negotiatedVersion { - return nil, qerr.Error(qerr.VersionNegotiationError, "would have picked a different version") - } - } - params := &eetp.Parameters // check that the server sent a stateless reset token if params.StatelessResetToken == nil { return nil, errors.New("server didn't send stateless_reset_token") @@ -983,17 +958,11 @@ func (s *session) processTransportParametersForClient(data []byte) (*handshake.T } func (s *session) processTransportParametersForServer(data []byte) (*handshake.TransportParameters, error) { - chtp := handshake.ClientHelloTransportParameters{} - if err := chtp.Unmarshal(data); err != nil { + params := &handshake.TransportParameters{} + if err := params.Unmarshal(data, s.perspective.Opposite()); err != nil { return nil, err } - // 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 if and only if the initial version is not contained in the supported versions - if chtp.InitialVersion != s.version && protocol.IsSupportedVersion(s.config.Versions, chtp.InitialVersion) { - return nil, qerr.Error(qerr.VersionNegotiationError, "Client should have used the initial version") - } - return &chtp.Parameters, nil + return params, nil } func (s *session) sendPackets() error { diff --git a/session_test.go b/session_test.go index 9e17a1e9..41ab5e30 100644 --- a/session_test.go +++ b/session_test.go @@ -1225,13 +1225,9 @@ var _ = Describe("Session", func() { // marshaling always sets it to this value MaxPacketSize: protocol.MaxReceivePacketSize, } - chtp := &handshake.ClientHelloTransportParameters{ - InitialVersion: sess.version, - Parameters: *params, - } streamManager.EXPECT().UpdateLimits(params) packer.EXPECT().HandleTransportParameters(params) - sess.processTransportParameters(chtp.Marshal()) + sess.processTransportParameters(params.Marshal()) // make the go routine return streamManager.EXPECT().CloseWithError(gomock.Any()) sessionRunner.EXPECT().Retire(gomock.Any()) @@ -1240,26 +1236,6 @@ var _ = Describe("Session", func() { sess.Close() Eventually(sess.Context().Done()).Should(BeClosed()) }) - - It("accepts a valid version negotiation", func() { - sess.version = 42 - sess.config.Versions = []protocol.VersionNumber{13, 37, 42} - chtp := &handshake.ClientHelloTransportParameters{ - InitialVersion: 22, // this must be an unsupported version - } - _, err := sess.processTransportParametersForServer(chtp.Marshal()) - Expect(err).ToNot(HaveOccurred()) - }) - - It("erros when a version negotiation was performed, although we already support the initial version", func() { - sess.version = 42 - sess.config.Versions = []protocol.VersionNumber{13, 37, 42} - chtp := &handshake.ClientHelloTransportParameters{ - InitialVersion: 13, // this must be a supported version - } - _, err := sess.processTransportParametersForServer(chtp.Marshal()) - Expect(err).To(MatchError("VERSION_NEGOTIATION_ERROR: Client should have used the initial version")) - }) }) Context("keep-alives", func() { @@ -1686,124 +1662,28 @@ var _ = Describe("Client Session", func() { }) It("errors if the TransportParameters don't contain the stateless reset token", func() { - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: sess.version, - SupportedVersions: []protocol.VersionNumber{sess.version}, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) + params := &handshake.TransportParameters{} + _, err := sess.processTransportParametersForClient(params.Marshal()) Expect(err).To(MatchError("server didn't send stateless_reset_token")) }) It("errors if the TransportParameters contain an original_connection_id, although no Retry was performed", func() { - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: sess.version, - SupportedVersions: []protocol.VersionNumber{sess.version}, - Parameters: handshake.TransportParameters{ - OriginalConnectionID: protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad}, - StatelessResetToken: &[16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - }, + params := &handshake.TransportParameters{ + OriginalConnectionID: protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad}, + StatelessResetToken: &[16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) + _, err := sess.processTransportParametersForClient(params.Marshal()) Expect(err).To(MatchError("expected original_connection_id to equal (empty), is 0xdecafbad")) }) It("errors if the TransportParameters contain an original_connection_id, although no Retry was performed", func() { sess.origDestConnID = protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef} - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: sess.version, - SupportedVersions: []protocol.VersionNumber{sess.version}, - Parameters: handshake.TransportParameters{ - OriginalConnectionID: protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad}, - StatelessResetToken: &[16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - }, + params := &handshake.TransportParameters{ + OriginalConnectionID: protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad}, + StatelessResetToken: &[16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) + _, err := sess.processTransportParametersForClient(params.Marshal()) Expect(err).To(MatchError("expected original_connection_id to equal 0xdeadbeef, is 0xdecafbad")) }) - - Context("Version Negotiation", func() { - var params handshake.TransportParameters - - BeforeEach(func() { - params = handshake.TransportParameters{ - StatelessResetToken: &[16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}, - OriginalConnectionID: sess.origDestConnID, - } - }) - - It("accepts a valid version negotiation", func() { - sess.initialVersion = 13 - sess.version = 37 - sess.config.Versions = []protocol.VersionNumber{13, 37, 42} - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 37, - SupportedVersions: []protocol.VersionNumber{36, 37, 38}, - Parameters: params, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) - Expect(err).ToNot(HaveOccurred()) - }) - - It("errors if the current version doesn't match negotiated_version", func() { - sess.initialVersion = 13 - sess.version = 37 - sess.config.Versions = []protocol.VersionNumber{13, 37, 42} - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 38, - SupportedVersions: []protocol.VersionNumber{36, 37, 38}, - Parameters: params, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) - Expect(err).To(MatchError("VERSION_NEGOTIATION_ERROR: current version doesn't match negotiated_version")) - }) - - It("errors if the current version is not contained in the server's supported versions", func() { - sess.version = 42 - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 42, - SupportedVersions: []protocol.VersionNumber{43, 44}, - Parameters: params, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) - Expect(err).To(MatchError("VERSION_NEGOTIATION_ERROR: current version not included in the supported versions")) - }) - - It("errors if version negotiation was performed, but would have picked a different version based on the supported version list", func() { - sess.version = 42 - sess.initialVersion = 41 - sess.config.Versions = []protocol.VersionNumber{43, 42, 41} - serverSupportedVersions := []protocol.VersionNumber{42, 43} - // check that version negotiation would have led us to pick version 43 - ver, ok := protocol.ChooseSupportedVersion(sess.config.Versions, serverSupportedVersions) - Expect(ok).To(BeTrue()) - Expect(ver).To(Equal(protocol.VersionNumber(43))) - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 42, - SupportedVersions: serverSupportedVersions, - Parameters: params, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) - Expect(err).To(MatchError("VERSION_NEGOTIATION_ERROR: would have picked a different version")) - }) - - It("doesn't error if it would have picked a different version based on the supported version list, if no version negotiation was performed", func() { - sess.version = 42 - sess.initialVersion = 42 // version == initialVersion means no version negotiation was performed - sess.config.Versions = []protocol.VersionNumber{43, 42, 41} - serverSupportedVersions := []protocol.VersionNumber{42, 43} - // check that version negotiation would have led us to pick version 43 - ver, ok := protocol.ChooseSupportedVersion(sess.config.Versions, serverSupportedVersions) - Expect(ok).To(BeTrue()) - Expect(ver).To(Equal(protocol.VersionNumber(43))) - eetp := &handshake.EncryptedExtensionsTransportParameters{ - NegotiatedVersion: 42, - SupportedVersions: serverSupportedVersions, - Parameters: params, - } - _, err := sess.processTransportParametersForClient(eetp.Marshal()) - Expect(err).ToNot(HaveOccurred()) - }) - }) - }) })