From 7b9b711a779edf4c507ee8955a3106b2343465c3 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 28 Jan 2019 11:10:50 +0900 Subject: [PATCH] fix timing side channel when reading headers with invalid reserved bits --- internal/wire/extended_header.go | 25 +++++++++------ internal/wire/header_test.go | 20 +++++++----- packet_unpacker.go | 32 +++++++++++++------ packet_unpacker_test.go | 54 ++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 26 deletions(-) diff --git a/internal/wire/extended_header.go b/internal/wire/extended_header.go index 4c50e050..081ed01a 100644 --- a/internal/wire/extended_header.go +++ b/internal/wire/extended_header.go @@ -10,6 +10,12 @@ import ( "github.com/lucas-clemente/quic-go/internal/utils" ) +// ErrInvalidReservedBits is returned when the reserved bits are incorrect. +// When this error is returned, parsing continues, and an ExtendedHeader is returned. +// This is necessary because we need to decrypt the packet in that case, +// in order to avoid a timing side-channel. +var ErrInvalidReservedBits = errors.New("invalid reserved bits") + // ExtendedHeader is the header of a QUIC packet. type ExtendedHeader struct { Header @@ -39,20 +45,17 @@ func (h *ExtendedHeader) parse(b *bytes.Reader, v protocol.VersionNumber) (*Exte } func (h *ExtendedHeader) parseLongHeader(b *bytes.Reader, v protocol.VersionNumber) (*ExtendedHeader, error) { - if h.typeByte&0xc != 0 { - return nil, errors.New("5th and 6th bit must be 0") - } if err := h.readPacketNumber(b); err != nil { return nil, err } - return h, nil + var err error + if h.typeByte&0xc != 0 { + err = ErrInvalidReservedBits + } + return h, err } func (h *ExtendedHeader) parseShortHeader(b *bytes.Reader, v protocol.VersionNumber) (*ExtendedHeader, error) { - if h.typeByte&0x18 != 0 { - return nil, errors.New("4th and 5th bit must be 0") - } - h.KeyPhase = protocol.KeyPhaseZero if h.typeByte&0x4 > 0 { h.KeyPhase = protocol.KeyPhaseOne @@ -61,7 +64,11 @@ func (h *ExtendedHeader) parseShortHeader(b *bytes.Reader, v protocol.VersionNum if err := h.readPacketNumber(b); err != nil { return nil, err } - return h, nil + var err error + if h.typeByte&0x18 != 0 { + err = ErrInvalidReservedBits + } + return h, err } func (h *ExtendedHeader) readPacketNumber(b *bytes.Reader) error { diff --git a/internal/wire/header_test.go b/internal/wire/header_test.go index 245e3bc7..6141a797 100644 --- a/internal/wire/header_test.go +++ b/internal/wire/header_test.go @@ -291,15 +291,18 @@ var _ = Describe("Header Parsing", func() { }) It("errors if the 5th or 6th bit are set", func() { - data := []byte{0xc0 | 0x2<<4 /* set the 5th bit */ | 0x8} + data := []byte{0xc0 | 0x2<<4 | 0x8 /* set the 5th bit */ | 0x1 /* 2 byte packet number */} data = appendVersion(data, versionIETFFrames) - data = append(data, 0x0) // connection ID lengths - data = append(data, encodeVarInt(0)...) // length + data = append(data, 0x0) // connection ID lengths + data = append(data, encodeVarInt(2)...) // length + data = append(data, []byte{0x12, 0x34}...) // packet number hdr, _, _, err := ParsePacket(data, 0) Expect(err).ToNot(HaveOccurred()) Expect(hdr.Type).To(Equal(protocol.PacketTypeHandshake)) - _, err = hdr.ParseExtended(bytes.NewReader(data), versionIETFFrames) - Expect(err).To(MatchError("5th and 6th bit must be 0")) + extHdr, err := hdr.ParseExtended(bytes.NewReader(data), versionIETFFrames) + Expect(err).To(MatchError(ErrInvalidReservedBits)) + Expect(extHdr).ToNot(BeNil()) + Expect(extHdr.PacketNumber).To(Equal(protocol.PacketNumber(0x1234))) }) It("errors on EOF, when parsing the header", func() { @@ -443,11 +446,14 @@ var _ = Describe("Header Parsing", func() { It("errors if the 4th or 5th bit are set", func() { connID := protocol.ConnectionID{1, 2, 3, 4, 5} data := append([]byte{0x40 | 0x10 /* set the 4th bit */}, connID...) + data = append(data, 0x42) // packet number hdr, _, _, err := ParsePacket(data, 5) Expect(err).ToNot(HaveOccurred()) Expect(hdr.IsLongHeader).To(BeFalse()) - _, err = hdr.ParseExtended(bytes.NewReader(data), versionIETFFrames) - Expect(err).To(MatchError("4th and 5th bit must be 0")) + extHdr, err := hdr.ParseExtended(bytes.NewReader(data), versionIETFFrames) + Expect(err).To(MatchError(ErrInvalidReservedBits)) + Expect(extHdr).ToNot(BeNil()) + Expect(extHdr.PacketNumber).To(Equal(protocol.PacketNumber(0x42))) }) It("reads a Short Header with a 5 byte connection ID", func() { diff --git a/packet_unpacker.go b/packet_unpacker.go index fe302d09..3b905169 100644 --- a/packet_unpacker.go +++ b/packet_unpacker.go @@ -91,28 +91,40 @@ func (u *packetUnpacker) Unpack(hdr *wire.Header, data []byte) (*unpackedPacket, } func (u *packetUnpacker) unpackLongHeaderPacket(opener handshake.LongHeaderOpener, hdr *wire.Header, data []byte) (*wire.ExtendedHeader, []byte, error) { - extHdr, err := u.unpack(opener, hdr, data) - if err != nil { - return nil, nil, err + extHdr, parseErr := u.unpack(opener, hdr, data) + // If the reserved bits are set incorrectly, we still need to continue unpacking. + // This avoids a timing side-channel, which otherwise might allow an attacker + // to gain information about the header encryption. + if parseErr != nil && parseErr != wire.ErrInvalidReservedBits { + return nil, nil, fmt.Errorf("error parsing extended header: %s", parseErr) } extHdrLen := extHdr.GetLength(u.version) decrypted, err := opener.Open(data[extHdrLen:extHdrLen], data[extHdrLen:], extHdr.PacketNumber, data[:extHdrLen]) if err != nil { return nil, nil, err } + if parseErr != nil { + return nil, nil, parseErr + } return extHdr, decrypted, nil } func (u *packetUnpacker) unpackShortHeaderPacket(opener handshake.ShortHeaderOpener, hdr *wire.Header, data []byte) (*wire.ExtendedHeader, []byte, error) { - extHdr, err := u.unpack(opener, hdr, data) - if err != nil { - return nil, nil, err + extHdr, parseErr := u.unpack(opener, hdr, data) + // If the reserved bits are set incorrectly, we still need to continue unpacking. + // This avoids a timing side-channel, which otherwise might allow an attacker + // to gain information about the header encryption. + if parseErr != nil && parseErr != wire.ErrInvalidReservedBits { + return nil, nil, parseErr } extHdrLen := extHdr.GetLength(u.version) decrypted, err := opener.Open(data[extHdrLen:extHdrLen], data[extHdrLen:], extHdr.PacketNumber, extHdr.KeyPhase, data[:extHdrLen]) if err != nil { return nil, nil, err } + if parseErr != nil { + return nil, nil, parseErr + } return extHdr, decrypted, nil } @@ -134,9 +146,9 @@ func (u *packetUnpacker) unpack(hd headerDecryptor, hdr *wire.Header, data []byt data[hdrLen:hdrLen+4], ) // 3. parse the header (and learn the actual length of the packet number) - extHdr, err := hdr.ParseExtended(r, u.version) - if err != nil { - return nil, fmt.Errorf("error parsing extended header: %s", err) + extHdr, parseErr := hdr.ParseExtended(r, u.version) + if parseErr != nil && parseErr != wire.ErrInvalidReservedBits { + return nil, parseErr } // 4. if the packet number is shorter than 4 bytes, replace the remaining bytes with the copy we saved earlier if extHdr.PacketNumberLen != protocol.PacketNumberLen4 { @@ -148,5 +160,5 @@ func (u *packetUnpacker) unpack(hd headerDecryptor, hdr *wire.Header, data []byt u.largestRcvdPacketNumber, extHdr.PacketNumber, ) - return extHdr, nil + return extHdr, parseErr } diff --git a/packet_unpacker_test.go b/packet_unpacker_test.go index fdaba9b1..b9736bc7 100644 --- a/packet_unpacker_test.go +++ b/packet_unpacker_test.go @@ -110,6 +110,60 @@ var _ = Describe("Packet Unpacker", func() { Expect(err).To(MatchError("test err")) }) + It("defends against the timing side-channel when the reserved bits are wrong, for long header packets", func() { + extHdr := &wire.ExtendedHeader{ + Header: wire.Header{ + IsLongHeader: true, + Type: protocol.PacketTypeHandshake, + DestConnectionID: connID, + Version: version, + }, + PacketNumber: 0x1337, + PacketNumberLen: 2, + } + hdr, hdrRaw := getHeader(extHdr) + hdrRaw[0] |= 0xc + opener := mocks.NewMockLongHeaderOpener(mockCtrl) + opener.EXPECT().DecryptHeader(gomock.Any(), gomock.Any(), gomock.Any()) + cs.EXPECT().GetHandshakeOpener().Return(opener, nil) + opener.EXPECT().Open(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte("payload"), nil) + _, err := unpacker.Unpack(hdr, append(hdrRaw, payload...)) + Expect(err).To(MatchError(wire.ErrInvalidReservedBits)) + }) + + It("defends against the timing side-channel when the reserved bits are wrong, for short header packets", func() { + extHdr := &wire.ExtendedHeader{ + Header: wire.Header{DestConnectionID: connID}, + PacketNumber: 0x1337, + PacketNumberLen: 2, + } + hdr, hdrRaw := getHeader(extHdr) + hdrRaw[0] |= 0x18 + opener := mocks.NewMockShortHeaderOpener(mockCtrl) + opener.EXPECT().DecryptHeader(gomock.Any(), gomock.Any(), gomock.Any()) + cs.EXPECT().Get1RTTOpener().Return(opener, nil) + opener.EXPECT().Open(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte("payload"), nil) + _, err := unpacker.Unpack(hdr, append(hdrRaw, payload...)) + Expect(err).To(MatchError(wire.ErrInvalidReservedBits)) + }) + + It("returns the decryption error, when unpacking a packet with wrong reserved bits fails", func() { + extHdr := &wire.ExtendedHeader{ + Header: wire.Header{DestConnectionID: connID}, + PacketNumber: 0x1337, + PacketNumberLen: 2, + } + hdr, hdrRaw := getHeader(extHdr) + hdrRaw[0] |= 0x18 + opener := mocks.NewMockShortHeaderOpener(mockCtrl) + opener.EXPECT().DecryptHeader(gomock.Any(), gomock.Any(), gomock.Any()) + cs.EXPECT().Get1RTTOpener().Return(opener, nil) + testErr := errors.New("decryption error") + opener.EXPECT().Open(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, testErr) + _, err := unpacker.Unpack(hdr, append(hdrRaw, payload...)) + Expect(err).To(MatchError(testErr)) + }) + It("decrypts the header", func() { extHdr := &wire.ExtendedHeader{ Header: wire.Header{