From 797dfa57a11772ea82b7bf16b71f7107e488eeab Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 21 May 2018 11:00:01 +0800 Subject: [PATCH 1/2] disable 6 byte packet number in Public Headers Chrome never sends 6 byte packet numbers in Public Headers. --- internal/wire/header_test.go | 6 ++--- internal/wire/public_header.go | 12 ++++++---- internal/wire/public_header_test.go | 36 +++++++++++++---------------- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/internal/wire/header_test.go b/internal/wire/header_test.go index 77999826..b897266b 100644 --- a/internal/wire/header_test.go +++ b/internal/wire/header_test.go @@ -82,7 +82,7 @@ var _ = Describe("Header", func() { DestConnectionID: connID, SrcConnectionID: connID, PacketNumber: 0x1337, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, }).writePublicHeader(buf, protocol.PerspectiveClient, versionPublicHeader) Expect(err).ToNot(HaveOccurred()) hdr, err := ParseHeaderSentByClient(bytes.NewReader(buf.Bytes())) @@ -101,7 +101,7 @@ var _ = Describe("Header", func() { DestConnectionID: connID, SrcConnectionID: connID, PacketNumber: 0x1337, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, DiversificationNonce: bytes.Repeat([]byte{'f'}, 32), }).writePublicHeader(buf, protocol.PerspectiveServer, versionPublicHeader) Expect(err).ToNot(HaveOccurred()) @@ -122,7 +122,7 @@ var _ = Describe("Header", func() { DestConnectionID: protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}, SrcConnectionID: protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}, PacketNumber: 0x1337, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen2, }).writePublicHeader(buf, protocol.PerspectiveClient, versionPublicHeader) Expect(err).ToNot(HaveOccurred()) _, err = ParseHeaderSentByClient(bytes.NewReader(buf.Bytes()[0:12])) diff --git a/internal/wire/public_header.go b/internal/wire/public_header.go index 33a0eba2..ceaeba70 100644 --- a/internal/wire/public_header.go +++ b/internal/wire/public_header.go @@ -16,6 +16,7 @@ var ( errReceivedOmittedConnectionID = qerr.Error(qerr.InvalidPacketHeader, "receiving packets with omitted ConnectionID is not supported") errInvalidConnectionID = qerr.Error(qerr.InvalidPacketHeader, "connection ID cannot be 0") errGetLengthNotForVersionNegotiation = errors.New("PublicHeader: GetLength cannot be called for VersionNegotiation packets") + errInvalidPacketNumberLen6 = errors.New("invalid packet number length: 6 bytes") ) // writePublicHeader writes a Public Header. @@ -58,8 +59,6 @@ func (h *Header) writePublicHeader(b *bytes.Buffer, pers protocol.Perspective, _ publicFlagByte |= 0x10 case protocol.PacketNumberLen4: publicFlagByte |= 0x20 - case protocol.PacketNumberLen6: - publicFlagByte |= 0x30 } } b.WriteByte(publicFlagByte) @@ -86,7 +85,7 @@ func (h *Header) writePublicHeader(b *bytes.Buffer, pers protocol.Perspective, _ case protocol.PacketNumberLen4: utils.BigEndian.WriteUint32(b, uint32(h.PacketNumber)) case protocol.PacketNumberLen6: - utils.BigEndian.WriteUint48(b, uint64(h.PacketNumber)&(1<<48-1)) + return errInvalidPacketNumberLen6 default: return errors.New("PublicHeader: PacketNumberLen not set") } @@ -120,7 +119,7 @@ func parsePublicHeader(b *bytes.Reader, packetSentBy protocol.Perspective) (*Hea if header.hasPacketNumber(packetSentBy) { switch publicFlagByte & 0x30 { case 0x30: - header.PacketNumberLen = protocol.PacketNumberLen6 + return nil, errInvalidPacketNumberLen6 case 0x20: header.PacketNumberLen = protocol.PacketNumberLen4 case 0x10: @@ -211,8 +210,11 @@ func (h *Header) getPublicHeaderLength(pers protocol.Perspective) (protocol.Byte } length := protocol.ByteCount(1) // 1 byte for public flags + if h.PacketNumberLen == protocol.PacketNumberLen6 { + return 0, errInvalidPacketNumberLen6 + } if h.hasPacketNumber(pers) { - if h.PacketNumberLen != protocol.PacketNumberLen1 && h.PacketNumberLen != protocol.PacketNumberLen2 && h.PacketNumberLen != protocol.PacketNumberLen4 && h.PacketNumberLen != protocol.PacketNumberLen6 { + if h.PacketNumberLen != protocol.PacketNumberLen1 && h.PacketNumberLen != protocol.PacketNumberLen2 && h.PacketNumberLen != protocol.PacketNumberLen4 { return 0, errPacketNumberLenNotSet } length += protocol.ByteCount(h.PacketNumberLen) diff --git a/internal/wire/public_header_test.go b/internal/wire/public_header_test.go index f1685320..0e506e9f 100644 --- a/internal/wire/public_header_test.go +++ b/internal/wire/public_header_test.go @@ -163,13 +163,10 @@ var _ = Describe("Public Header", func() { Expect(b.Len()).To(BeZero()) }) - It("accepts 6-byte packet numbers", func() { + It("rejects 6-byte packet numbers", func() { b := bytes.NewReader([]byte{0x38, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0x23, 0x42, 0xad, 0xfb, 0xca, 0xde}) - hdr, err := parsePublicHeader(b, protocol.PerspectiveClient) - Expect(err).ToNot(HaveOccurred()) - Expect(hdr.PacketNumber).To(Equal(protocol.PacketNumber(0x2342adfbcade))) - Expect(hdr.PacketNumberLen).To(Equal(protocol.PacketNumberLen6)) - Expect(b.Len()).To(BeZero()) + _, err := parsePublicHeader(b, protocol.PerspectiveClient) + Expect(err).To(MatchError(errInvalidPacketNumberLen6)) }) }) }) @@ -181,11 +178,11 @@ var _ = Describe("Public Header", func() { DestConnectionID: connID, SrcConnectionID: connID, PacketNumber: 2, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, } err := hdr.writePublicHeader(b, protocol.PerspectiveServer, versionBigEndian) Expect(err).ToNot(HaveOccurred()) - Expect(b.Bytes()).To(Equal([]byte{0x38, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0, 0, 0, 0, 0, 2})) + Expect(b.Bytes()).To(Equal([]byte{0x28, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0, 0, 0, 2})) }) It("writes a sample header as a client", func() { @@ -194,11 +191,11 @@ var _ = Describe("Public Header", func() { DestConnectionID: connID, SrcConnectionID: connID, PacketNumber: 0x1337, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen2, } err := hdr.writePublicHeader(b, protocol.PerspectiveClient, versionBigEndian) Expect(err).ToNot(HaveOccurred()) - Expect(b.Bytes()).To(Equal([]byte{0x38, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0x0, 0x0, 0x0, 0x0, 0x13, 0x37})) + Expect(b.Bytes()).To(Equal([]byte{0x18, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0x13, 0x37})) }) It("refuses to write a Public Header if the source and destination connection IDs are not matching", func() { @@ -207,7 +204,7 @@ var _ = Describe("Public Header", func() { DestConnectionID: protocol.ConnectionID{1, 2, 3, 4, 5, 6, 7, 8}, SrcConnectionID: protocol.ConnectionID{8, 7, 6, 5, 4, 3, 2, 1}, PacketNumber: 0x1337, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, } err := hdr.writePublicHeader(b, protocol.PerspectiveClient, versionBigEndian) Expect(err).To(MatchError("PublicHeader: SrcConnectionID must be equal to DestConnectionID")) @@ -380,11 +377,11 @@ var _ = Describe("Public Header", func() { DestConnectionID: connID, SrcConnectionID: connID, PacketNumber: 0xdecafbad, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, } length, err := hdr.getPublicHeaderLength(protocol.PerspectiveServer) Expect(err).ToNot(HaveOccurred()) - Expect(length).To(Equal(protocol.ByteCount(1 + 8 + 6))) // 1 byte public flag, 8 bytes connectionID, and packet number + Expect(length).To(Equal(protocol.ByteCount(1 + 8 + 4))) // 1 byte public flag, 8 bytes connectionID, and packet number }) It("gets the lengths of a packet sent by the client with the VersionFlag set", func() { @@ -393,13 +390,13 @@ var _ = Describe("Public Header", func() { SrcConnectionID: connID, OmitConnectionID: true, PacketNumber: 0xdecafbad, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, VersionFlag: true, Version: versionBigEndian, } length, err := hdr.getPublicHeaderLength(protocol.PerspectiveClient) Expect(err).ToNot(HaveOccurred()) - Expect(length).To(Equal(protocol.ByteCount(1 + 4 + 6))) // 1 byte public flag, 4 version number, and packet number + Expect(length).To(Equal(protocol.ByteCount(1 + 4 + 4))) // 1 byte public flag, 4 version number, and packet number }) It("gets the length of a packet with longest packet number length and omitted connectionID", func() { @@ -408,11 +405,11 @@ var _ = Describe("Public Header", func() { SrcConnectionID: connID, OmitConnectionID: true, PacketNumber: 0xDECAFBAD, - PacketNumberLen: protocol.PacketNumberLen6, + PacketNumberLen: protocol.PacketNumberLen4, } length, err := hdr.getPublicHeaderLength(protocol.PerspectiveServer) Expect(err).ToNot(HaveOccurred()) - Expect(length).To(Equal(protocol.ByteCount(1 + 6))) // 1 byte public flag, and packet number + Expect(length).To(Equal(protocol.ByteCount(1 + 4))) // 1 byte public flag, and packet number }) It("gets the length of a packet 2 byte packet number length ", func() { @@ -503,7 +500,7 @@ var _ = Describe("Public Header", func() { Expect(b.Bytes()).To(Equal([]byte{0x28, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0xde, 0xca, 0xfb, 0xad})) }) - It("writes a header with a 6-byte packet number", func() { + It("refuses to write a header with a 6-byte packet number", func() { b := &bytes.Buffer{} hdr := Header{ DestConnectionID: connID, @@ -512,8 +509,7 @@ var _ = Describe("Public Header", func() { PacketNumberLen: protocol.PacketNumberLen6, } err := hdr.writePublicHeader(b, protocol.PerspectiveServer, version) - Expect(err).ToNot(HaveOccurred()) - Expect(b.Bytes()).To(Equal([]byte{0x38, 0x4c, 0xfa, 0x9f, 0x9b, 0x66, 0x86, 0x19, 0xf6, 0x13, 0x37, 0xde, 0xca, 0xfb, 0xad})) + Expect(err).To(MatchError(errInvalidPacketNumberLen6)) }) }) }) From 372463db49758db30ace123b82f276a4ec562f39 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 21 May 2018 11:27:15 +0800 Subject: [PATCH 2/2] don't pass the client's version to the header parser --- client.go | 2 +- internal/wire/header.go | 9 +++------ internal/wire/header_test.go | 14 +++++++------- packet_packer_test.go | 4 ++-- server_test.go | 4 ++-- server_tls_test.go | 6 +++--- 6 files changed, 18 insertions(+), 21 deletions(-) diff --git a/client.go b/client.go index 692ca3c1..98c5590c 100644 --- a/client.go +++ b/client.go @@ -304,7 +304,7 @@ func (c *client) handlePacket(remoteAddr net.Addr, packet []byte) error { rcvTime := time.Now() r := bytes.NewReader(packet) - hdr, err := wire.ParseHeaderSentByServer(r, c.version) + hdr, err := wire.ParseHeaderSentByServer(r) // drop the packet if we can't parse the header if err != nil { return fmt.Errorf("error parsing packet from %s: %s", remoteAddr.String(), err.Error()) diff --git a/internal/wire/header.go b/internal/wire/header.go index 41261333..076e4d43 100644 --- a/internal/wire/header.go +++ b/internal/wire/header.go @@ -39,7 +39,7 @@ type Header struct { } // ParseHeaderSentByServer parses the header for a packet that was sent by the server. -func ParseHeaderSentByServer(b *bytes.Reader, version protocol.VersionNumber) (*Header, error) { +func ParseHeaderSentByServer(b *bytes.Reader) (*Header, error) { typeByte, err := b.ReadByte() if err != nil { return nil, err @@ -49,13 +49,10 @@ func ParseHeaderSentByServer(b *bytes.Reader, version protocol.VersionNumber) (* var isPublicHeader bool if typeByte&0x80 > 0 { // gQUIC always has 0x80 unset. IETF Long Header or Version Negotiation isPublicHeader = false - } else if typeByte&0xcf == 0x9 { // gQUIC Version Negotiation Packet - isPublicHeader = true } else { - // the client knows the version that this packet was sent with - isPublicHeader = !version.UsesTLS() + // gQUIC never uses 6 byte packet numbers, so the third and fourth bit will never be 11 + isPublicHeader = typeByte&0x30 != 0x30 } - return parsePacketHeader(b, protocol.PerspectiveServer, isPublicHeader) } diff --git a/internal/wire/header_test.go b/internal/wire/header_test.go index b897266b..9a000743 100644 --- a/internal/wire/header_test.go +++ b/internal/wire/header_test.go @@ -68,7 +68,7 @@ var _ = Describe("Header", func() { PacketNumber: 0x42, }).writeHeader(buf) Expect(err).ToNot(HaveOccurred()) - hdr, err := ParseHeaderSentByServer(bytes.NewReader(buf.Bytes()), versionIETFHeader) + hdr, err := ParseHeaderSentByServer(bytes.NewReader(buf.Bytes())) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsPublicHeader).To(BeFalse()) }) @@ -94,7 +94,7 @@ var _ = Describe("Header", func() { Expect(hdr.IsPublicHeader).To(BeTrue()) }) - It("parses a gQUIC Public Header, when the version is known", func() { + It("parses a gQUIC Public Header", func() { connID := protocol.ConnectionID{8, 7, 6, 5, 4, 3, 2, 1} buf := &bytes.Buffer{} err := (&Header{ @@ -105,7 +105,7 @@ var _ = Describe("Header", func() { DiversificationNonce: bytes.Repeat([]byte{'f'}, 32), }).writePublicHeader(buf, protocol.PerspectiveServer, versionPublicHeader) Expect(err).ToNot(HaveOccurred()) - hdr, err := ParseHeaderSentByServer(bytes.NewReader(buf.Bytes()), versionPublicHeader) + hdr, err := ParseHeaderSentByServer(bytes.NewReader(buf.Bytes())) Expect(err).ToNot(HaveOccurred()) Expect(hdr.DestConnectionID).To(Equal(connID)) Expect(hdr.SrcConnectionID).To(Equal(connID)) @@ -130,7 +130,7 @@ var _ = Describe("Header", func() { }) It("errors when given no data", func() { - _, err := ParseHeaderSentByServer(bytes.NewReader([]byte{}), protocol.VersionUnknown) + _, err := ParseHeaderSentByServer(bytes.NewReader([]byte{})) Expect(err).To(MatchError(io.EOF)) _, err = ParseHeaderSentByClient(bytes.NewReader([]byte{})) Expect(err).To(MatchError(io.EOF)) @@ -140,7 +140,7 @@ var _ = Describe("Header", func() { connID := protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad, 0xde, 0xca, 0xfb, 0xad} versions := []protocol.VersionNumber{0x13, 0x37} data := ComposeGQUICVersionNegotiation(connID, versions) - hdr, err := ParseHeaderSentByServer(bytes.NewReader(data), protocol.VersionUnknown) + hdr, err := ParseHeaderSentByServer(bytes.NewReader(data)) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsPublicHeader).To(BeTrue()) Expect(hdr.DestConnectionID).To(Equal(connID)) @@ -157,7 +157,7 @@ var _ = Describe("Header", func() { versions := []protocol.VersionNumber{0x13, 0x37} data, err := ComposeVersionNegotiation(destConnID, srcConnID, versions) Expect(err).ToNot(HaveOccurred()) - hdr, err := ParseHeaderSentByServer(bytes.NewReader(data), protocol.VersionUnknown) + hdr, err := ParseHeaderSentByServer(bytes.NewReader(data)) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsPublicHeader).To(BeFalse()) Expect(hdr.IsVersionNegotiation).To(BeTrue()) @@ -199,7 +199,7 @@ var _ = Describe("Header", func() { } err := hdr.Write(buf, protocol.PerspectiveServer, versionIETFHeader) Expect(err).ToNot(HaveOccurred()) - _, err = ParseHeaderSentByServer(bytes.NewReader(buf.Bytes()), versionIETFFrames) + _, err = ParseHeaderSentByServer(bytes.NewReader(buf.Bytes())) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsPublicHeader).To(BeFalse()) }) diff --git a/packet_packer_test.go b/packet_packer_test.go index 6e69990c..d682cc95 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -265,7 +265,7 @@ var _ = Describe("Packet packer", func() { Expect(err).ToNot(HaveOccurred()) // parse the packet r := bytes.NewReader(p.raw) - hdr, err := wire.ParseHeaderSentByServer(r, packer.version) + hdr, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) Expect(hdr.PayloadLen).To(BeEquivalentTo(r.Len())) }) @@ -614,7 +614,7 @@ var _ = Describe("Packet packer", func() { Expect(p.header.IsLongHeader).To(BeTrue()) // parse the packet r := bytes.NewReader(p.raw) - hdr, err := wire.ParseHeaderSentByServer(r, packer.version) + hdr, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) Expect(hdr.PayloadLen).To(BeEquivalentTo(r.Len())) }) diff --git a/server_test.go b/server_test.go index 9461a718..0642bd72 100644 --- a/server_test.go +++ b/server_test.go @@ -571,7 +571,7 @@ var _ = Describe("Server", func() { Eventually(func() int { return conn.dataWritten.Len() }).ShouldNot(BeZero()) Expect(conn.dataWrittenTo).To(Equal(udpAddr)) r := bytes.NewReader(conn.dataWritten.Bytes()) - packet, err := wire.ParseHeaderSentByServer(r, protocol.VersionUnknown) + packet, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) Expect(packet.VersionFlag).To(BeTrue()) Expect(packet.DestConnectionID).To(Equal(connID)) @@ -614,7 +614,7 @@ var _ = Describe("Server", func() { Eventually(func() int { return conn.dataWritten.Len() }).ShouldNot(BeZero()) Expect(conn.dataWrittenTo).To(Equal(udpAddr)) r := bytes.NewReader(conn.dataWritten.Bytes()) - packet, err := wire.ParseHeaderSentByServer(r, protocol.VersionUnknown) + packet, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) Expect(packet.IsVersionNegotiation).To(BeTrue()) Expect(packet.DestConnectionID).To(Equal(connID)) diff --git a/server_tls_test.go b/server_tls_test.go index b1246ab6..a03954b8 100644 --- a/server_tls_test.go +++ b/server_tls_test.go @@ -71,7 +71,7 @@ var _ = Describe("Stateless TLS handling", func() { unpackPacket := func(data []byte) (*wire.Header, []byte) { r := bytes.NewReader(conn.dataWritten.Bytes()) - hdr, err := wire.ParseHeaderSentByServer(r, protocol.VersionTLS) + hdr, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) hdr.Raw = data[:len(data)-r.Len()] aead, err := crypto.NewNullAEAD(protocol.PerspectiveClient, hdr.SrcConnectionID, protocol.VersionTLS) @@ -89,7 +89,7 @@ var _ = Describe("Stateless TLS handling", func() { } server.HandleInitial(nil, hdr, bytes.Repeat([]byte{0}, protocol.MinInitialPacketSize)) Expect(conn.dataWritten.Len()).ToNot(BeZero()) - hdr, err := wire.ParseHeaderSentByServer(bytes.NewReader(conn.dataWritten.Bytes()), protocol.VersionUnknown) + hdr, err := wire.ParseHeaderSentByServer(bytes.NewReader(conn.dataWritten.Bytes())) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsVersionNegotiation).To(BeTrue()) Expect(sessionChan).ToNot(Receive()) @@ -118,7 +118,7 @@ var _ = Describe("Stateless TLS handling", func() { server.HandleInitial(nil, hdr, data) Expect(conn.dataWritten.Len()).ToNot(BeZero()) r := bytes.NewReader(conn.dataWritten.Bytes()) - replyHdr, err := wire.ParseHeaderSentByServer(r, protocol.VersionTLS) + replyHdr, err := wire.ParseHeaderSentByServer(r) Expect(err).ToNot(HaveOccurred()) Expect(replyHdr.Type).To(Equal(protocol.PacketTypeRetry)) Expect(replyHdr.SrcConnectionID).To(Equal(hdr.DestConnectionID))