diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index 421c99ca..f775787a 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -47,6 +47,11 @@ func (m messageType) String() string { } } +// ErrOpenerNotYetAvailable is returned when an opener is requested for an encryption level, +// but the corresponding opener has not yet been initialized +// This can happen when packets arrive out of order. +var ErrOpenerNotYetAvailable = errors.New("CryptoSetup: opener at this encryption level not yet available") + type cryptoSetup struct { tlsConf *qtls.Config @@ -520,12 +525,12 @@ func (h *cryptoSetup) GetOpener(level protocol.EncryptionLevel) (Opener, error) return h.initialOpener, nil case protocol.EncryptionHandshake: if h.handshakeOpener == nil { - return nil, errors.New("CryptoSetup: no opener with encryption level Handshake") + return nil, ErrOpenerNotYetAvailable } return h.handshakeOpener, nil case protocol.Encryption1RTT: if h.opener == nil { - return nil, errors.New("CryptoSetup: no opener with encryption level 1-RTT") + return nil, ErrOpenerNotYetAvailable } return h.opener, nil default: diff --git a/packet_unpacker.go b/packet_unpacker.go index 58d1009f..5cde3fa7 100644 --- a/packet_unpacker.go +++ b/packet_unpacker.go @@ -61,7 +61,7 @@ func (u *packetUnpacker) Unpack(hdr *wire.Header, data []byte) (*unpackedPacket, } opener, err := u.cs.GetOpener(encLevel) if err != nil { - return nil, qerr.Error(qerr.DecryptionFailure, err.Error()) + return nil, err } hdrLen := int(hdr.ParsedLen()) // The packet number can be up to 4 bytes long, but we won't know the length until we decrypt it. diff --git a/packet_unpacker_test.go b/packet_unpacker_test.go index 8d7e73ee..8aa4e446 100644 --- a/packet_unpacker_test.go +++ b/packet_unpacker_test.go @@ -5,6 +5,7 @@ import ( "errors" "github.com/golang/mock/gomock" + "github.com/lucas-clemente/quic-go/internal/handshake" "github.com/lucas-clemente/quic-go/internal/mocks" "github.com/lucas-clemente/quic-go/internal/protocol" "github.com/lucas-clemente/quic-go/internal/qerr" @@ -124,9 +125,9 @@ var _ = Describe("Packet Unpacker", func() { PacketNumberLen: 2, } hdr, hdrRaw := getHeader(extHdr) - cs.EXPECT().GetOpener(protocol.Encryption1RTT).Return(nil, errors.New("test err")) + cs.EXPECT().GetOpener(protocol.Encryption1RTT).Return(nil, handshake.ErrOpenerNotYetAvailable) _, err := unpacker.Unpack(hdr, hdrRaw) - Expect(err).To(MatchError(qerr.Error(qerr.DecryptionFailure, "test err"))) + Expect(err).To(MatchError(handshake.ErrOpenerNotYetAvailable)) }) It("returns the error when unpacking fails", func() { diff --git a/session.go b/session.go index 295578cb..fdae1b9d 100644 --- a/session.go +++ b/session.go @@ -358,13 +358,10 @@ runLoop: // We do all the interesting stuff after the switch statement, so // nothing to see here. case p := <-s.receivedPackets: - err := s.handlePacketImpl(p) - if err != nil { - if qErr, ok := err.(*qerr.QuicError); ok && qErr.ErrorCode == qerr.DecryptionFailure { - s.tryQueueingUndecryptablePacket(p) - continue - } - s.closeLocal(err) + // Only reset the timers if this packet was actually processed. + // This avoids modifying any state when handling undecryptable packets, + // which could be injected by an attacker. + if wasProcessed := s.handlePacketImpl(p); !wasProcessed { continue } // This is a bit unclean, but works properly, since the packet always @@ -473,18 +470,24 @@ func (s *session) handleHandshakeComplete() { } } -func (s *session) handlePacketImpl(p *receivedPacket) error { +func (s *session) handlePacketImpl(p *receivedPacket) bool /* was the packet successfully processed */ { // The server can change the source connection ID with the first Handshake packet. // After this, all packets with a different source connection have to be ignored. if s.receivedFirstPacket && p.hdr.IsLongHeader && !p.hdr.SrcConnectionID.Equal(s.destConnID) { s.logger.Debugf("Dropping packet with unexpected source connection ID: %s (expected %s)", p.hdr.SrcConnectionID, s.destConnID) - return nil + return false } packet, err := s.unpacker.Unpack(p.hdr, p.data) // if the decryption failed, this might be a packet sent by an attacker if err != nil { - return err + if err == handshake.ErrOpenerNotYetAvailable { + s.tryQueueingUndecryptablePacket(p) + return false + } + // TODO: don't close the connection when receiving 0-RTT packets + s.closeLocal(err) + return false } if s.logger.Debug() { @@ -492,15 +495,23 @@ func (s *session) handlePacketImpl(p *receivedPacket) error { packet.hdr.Log(s.logger) } + if err := s.handleUnpackedPacket(packet, p.rcvTime); err != nil { + s.closeLocal(err) + return false + } + return true +} + +func (s *session) handleUnpackedPacket(packet *unpackedPacket, rcvTime time.Time) error { // The server can change the source connection ID with the first Handshake packet. - if s.perspective == protocol.PerspectiveClient && !s.receivedFirstPacket && p.hdr.IsLongHeader && !p.hdr.SrcConnectionID.Equal(s.destConnID) { - s.logger.Debugf("Received first packet. Switching destination connection ID to: %s", p.hdr.SrcConnectionID) - s.destConnID = p.hdr.SrcConnectionID + if s.perspective == protocol.PerspectiveClient && !s.receivedFirstPacket && packet.hdr.IsLongHeader && !packet.hdr.SrcConnectionID.Equal(s.destConnID) { + s.logger.Debugf("Received first packet. Switching destination connection ID to: %s", packet.hdr.SrcConnectionID) + s.destConnID = packet.hdr.SrcConnectionID s.packer.ChangeDestConnectionID(s.destConnID) } s.receivedFirstPacket = true - s.lastNetworkActivityTime = p.rcvTime + s.lastNetworkActivityTime = rcvTime s.keepAlivePingSent = false // The client completes the handshake first (after sending the CFIN). @@ -514,9 +525,9 @@ func (s *session) handlePacketImpl(p *receivedPacket) error { // If this is a Retry packet, there's no need to send an ACK. // The session will be closed and recreated as soon as the crypto setup processed the HRR. - if p.hdr.Type != protocol.PacketTypeRetry { + if packet.hdr.Type != protocol.PacketTypeRetry { isRetransmittable := ackhandler.HasRetransmittableFrames(packet.frames) - if err := s.receivedPacketHandler.ReceivedPacket(packet.packetNumber, p.rcvTime, isRetransmittable); err != nil { + if err := s.receivedPacketHandler.ReceivedPacket(packet.packetNumber, rcvTime, isRetransmittable); err != nil { return err } } diff --git a/session_test.go b/session_test.go index 9e074cf2..6e71e98b 100644 --- a/session_test.go +++ b/session_test.go @@ -468,7 +468,7 @@ var _ = Describe("Session", func() { PacketNumberLen: protocol.PacketNumberLen1, } rcvTime := time.Now().Add(-10 * time.Second) - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{packetNumber: 0x1337}, nil) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{packetNumber: 0x1337, hdr: hdr}, nil) rph := mockackhandler.NewMockReceivedPacketHandler(mockCtrl) rph.EXPECT().ReceivedPacket(protocol.PacketNumber(0x1337), rcvTime, false) sess.receivedPacketHandler = rph @@ -476,7 +476,7 @@ var _ = Describe("Session", func() { rcvTime: rcvTime, hdr: &hdr.Header, data: getData(hdr), - })).To(Succeed()) + })).To(BeTrue()) }) It("closes when handling a packet fails", func() { @@ -499,28 +499,29 @@ var _ = Describe("Session", func() { }) It("handles duplicate packets", func() { - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{}, nil).Times(2) hdr := &wire.ExtendedHeader{ PacketNumber: 5, PacketNumberLen: protocol.PacketNumberLen1, } - Expect(sess.handlePacketImpl(&receivedPacket{hdr: &hdr.Header, data: getData(hdr)})).To(Succeed()) - Expect(sess.handlePacketImpl(&receivedPacket{hdr: &hdr.Header, data: getData(hdr)})).To(Succeed()) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{hdr: hdr}, nil).Times(2) + Expect(sess.handlePacketImpl(&receivedPacket{hdr: &hdr.Header, data: getData(hdr)})).To(BeTrue()) + Expect(sess.handlePacketImpl(&receivedPacket{hdr: &hdr.Header, data: getData(hdr)})).To(BeTrue()) }) It("ignores packets with a different source connection ID", func() { + hdr := &wire.Header{ + IsLongHeader: true, + DestConnectionID: sess.destConnID, + SrcConnectionID: sess.srcConnID, + Length: 1, + } // Send one packet, which might change the connection ID. // only EXPECT one call to the unpacker - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{}, nil) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{hdr: &wire.ExtendedHeader{Header: *hdr}}, nil) Expect(sess.handlePacketImpl(&receivedPacket{ - hdr: &wire.Header{ - IsLongHeader: true, - DestConnectionID: sess.destConnID, - SrcConnectionID: sess.srcConnID, - Length: 1, - }, + hdr: hdr, data: getData(&wire.ExtendedHeader{PacketNumberLen: protocol.PacketNumberLen1}), - })).To(Succeed()) + })).To(BeTrue()) // The next packet has to be ignored, since the source connection ID doesn't match. Expect(sess.handlePacketImpl(&receivedPacket{ hdr: &wire.Header{ @@ -530,12 +531,12 @@ var _ = Describe("Session", func() { Length: 1, }, data: getData(&wire.ExtendedHeader{PacketNumberLen: protocol.PacketNumberLen1}), - })).To(Succeed()) + })).To(BeFalse()) }) Context("updating the remote address", func() { It("doesn't support connection migration", func() { - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{}, nil) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{hdr: &wire.ExtendedHeader{}}, nil) origAddr := sess.conn.(*mockConnection).remoteAddr remoteIP := &net.IPAddr{IP: net.IPv4(192, 168, 0, 100)} Expect(origAddr).ToNot(Equal(remoteIP)) @@ -544,8 +545,7 @@ var _ = Describe("Session", func() { hdr: &wire.Header{}, data: getData(&wire.ExtendedHeader{PacketNumberLen: protocol.PacketNumberLen1}), } - err := sess.handlePacketImpl(&p) - Expect(err).ToNot(HaveOccurred()) + Expect(sess.handlePacketImpl(&p)).To(BeTrue()) Expect(sess.conn.(*mockConnection).remoteAddr).To(Equal(origAddr)) }) }) @@ -1306,7 +1306,9 @@ var _ = Describe("Client Session", func() { It("changes the connection ID when receiving the first packet from the server", func() { unpacker := NewMockUnpacker(mockCtrl) - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{}, nil) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).DoAndReturn(func(hdr *wire.Header, data []byte) (*unpackedPacket, error) { + return &unpackedPacket{hdr: &wire.ExtendedHeader{Header: *hdr}}, nil + }) sess.unpacker = unpacker go func() { defer GinkgoRecover() @@ -1324,7 +1326,7 @@ var _ = Describe("Client Session", func() { Length: 1, }, data: []byte{0}, - })).To(Succeed()) + })).To(BeTrue()) // make sure the go routine returns packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil) sessionRunner.EXPECT().retireConnectionID(gomock.Any())