From c522bcc68331b933f99a90a5d894da95dde272ac Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 12 Jun 2019 21:27:27 +0800 Subject: [PATCH] return a defined error when the AEAD cannot open a packet --- internal/handshake/aead.go | 6 +++++- internal/handshake/aead_test.go | 4 ++-- internal/handshake/crypto_setup.go | 10 ---------- internal/handshake/initial_aead_test.go | 2 +- internal/handshake/interface.go | 13 +++++++++++++ internal/handshake/updatable_aead.go | 14 +++++++++++--- internal/handshake/updatable_aead_test.go | 6 +++--- session.go | 6 ++++-- session_test.go | 23 ++++++++++++++++++++++- 9 files changed, 61 insertions(+), 23 deletions(-) diff --git a/internal/handshake/aead.go b/internal/handshake/aead.go index 673544e2..6dfae660 100644 --- a/internal/handshake/aead.go +++ b/internal/handshake/aead.go @@ -76,7 +76,11 @@ func (o *longHeaderOpener) Open(dst, src []byte, pn protocol.PacketNumber, ad [] binary.BigEndian.PutUint64(o.nonceBuf[len(o.nonceBuf)-8:], uint64(pn)) // The AEAD we're using here will be the qtls.aeadAESGCM13. // It uses the nonce provided here and XOR it with the IV. - return o.aead.Open(dst, o.nonceBuf, src, ad) + dec, err := o.aead.Open(dst, o.nonceBuf, src, ad) + if err != nil { + err = ErrDecryptionFailed + } + return dec, err } func (o *longHeaderOpener) DecryptHeader(sample []byte, firstByte *byte, pnBytes []byte) { diff --git a/internal/handshake/aead_test.go b/internal/handshake/aead_test.go index d211d3a1..7498fb5a 100644 --- a/internal/handshake/aead_test.go +++ b/internal/handshake/aead_test.go @@ -43,14 +43,14 @@ var _ = Describe("AEAD", func() { sealer, opener := getSealerAndOpener() encrypted := sealer.Seal(nil, msg, 0x1337, ad) _, err := opener.Open(nil, encrypted, 0x1337, []byte("wrong ad")) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) }) It("fails to open a message if the packet number is not the same", func() { sealer, opener := getSealerAndOpener() encrypted := sealer.Seal(nil, msg, 0x1337, ad) _, err := opener.Open(nil, encrypted, 0x42, ad) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) }) }) diff --git a/internal/handshake/crypto_setup.go b/internal/handshake/crypto_setup.go index 086e46b6..0f5729c2 100644 --- a/internal/handshake/crypto_setup.go +++ b/internal/handshake/crypto_setup.go @@ -55,16 +55,6 @@ func (m messageType) String() string { } } -var ( - // 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. - ErrOpenerNotYetAvailable = errors.New("CryptoSetup: opener at this encryption level not yet available") - // ErrKeysDropped is returned when an opener or a sealer is requested for an encryption level, - // but the corresponding keys have already been dropped. - ErrKeysDropped = errors.New("CryptoSetup: keys were already dropped") -) - type cryptoSetup struct { tlsConf *qtls.Config conn *qtls.Conn diff --git a/internal/handshake/initial_aead_test.go b/internal/handshake/initial_aead_test.go index 1a3031e8..87b82895 100644 --- a/internal/handshake/initial_aead_test.go +++ b/internal/handshake/initial_aead_test.go @@ -114,7 +114,7 @@ var _ = Describe("Initial AEAD using AES-GCM", func() { clientMessage := clientSealer.Seal(nil, []byte("foobar"), 42, []byte("aad")) _, err = serverOpener.Open(nil, clientMessage, 42, []byte("aad")) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) }) It("encrypts und decrypts the header", func() { diff --git a/internal/handshake/interface.go b/internal/handshake/interface.go index 03f15fdf..07152c1f 100644 --- a/internal/handshake/interface.go +++ b/internal/handshake/interface.go @@ -2,12 +2,25 @@ package handshake import ( "crypto/tls" + "errors" "io" "github.com/lucas-clemente/quic-go/internal/protocol" "github.com/marten-seemann/qtls" ) +var ( + // 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. + ErrOpenerNotYetAvailable = errors.New("CryptoSetup: opener at this encryption level not yet available") + // ErrKeysDropped is returned when an opener or a sealer is requested for an encryption level, + // but the corresponding keys have already been dropped. + ErrKeysDropped = errors.New("CryptoSetup: keys were already dropped") + // ErrDecryptionFailed is returned when the AEAD fails to open the packet. + ErrDecryptionFailed = errors.New("decryption failed") +) + type headerDecryptor interface { DecryptHeader(sample []byte, firstByte *byte, pnBytes []byte) } diff --git a/internal/handshake/updatable_aead.go b/internal/handshake/updatable_aead.go index f62d4a84..e5cf2f71 100644 --- a/internal/handshake/updatable_aead.go +++ b/internal/handshake/updatable_aead.go @@ -96,11 +96,17 @@ func (a *updatableAEAD) Open(dst, src []byte, pn protocol.PacketNumber, kp proto if a.firstRcvdWithCurrentKey == protocol.InvalidPacketNumber || pn < a.firstRcvdWithCurrentKey { // TODO: check that prevRcv actually exists // we updated the key, but the peer hasn't updated yet - return a.prevRcvAEAD.Open(dst, a.nonceBuf, src, ad) + dec, err := a.prevRcvAEAD.Open(dst, a.nonceBuf, src, ad) + if err != nil { + err = ErrDecryptionFailed + } + return dec, err } // try opening the packet with the next key phase dec, err := a.nextRcvAEAD.Open(dst, a.nonceBuf, src, ad) - if err == nil { + if err != nil { + err = ErrDecryptionFailed + } else { // if opening succeeds, roll over to the next key phase a.rollKeys() a.firstRcvdWithCurrentKey = pn @@ -110,7 +116,9 @@ func (a *updatableAEAD) Open(dst, src []byte, pn protocol.PacketNumber, kp proto // The AEAD we're using here will be the qtls.aeadAESGCM13. // It uses the nonce provided here and XOR it with the IV. dec, err := a.rcvAEAD.Open(dst, a.nonceBuf, src, ad) - if err == nil && a.firstRcvdWithCurrentKey == protocol.InvalidPacketNumber { + if err != nil { + err = ErrDecryptionFailed + } else if a.firstRcvdWithCurrentKey == protocol.InvalidPacketNumber { a.firstRcvdWithCurrentKey = pn } return dec, err diff --git a/internal/handshake/updatable_aead_test.go b/internal/handshake/updatable_aead_test.go index b72393b7..5a6cacdc 100644 --- a/internal/handshake/updatable_aead_test.go +++ b/internal/handshake/updatable_aead_test.go @@ -84,13 +84,13 @@ var _ = Describe("Updatable AEAD", func() { It("fails to open a message if the associated data is not the same", func() { encrypted := client.Seal(nil, msg, 0x1337, ad) _, err := server.Open(nil, encrypted, 0x1337, protocol.KeyPhaseZero, []byte("wrong ad")) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) }) It("fails to open a message if the packet number is not the same", func() { encrypted := server.Seal(nil, msg, 0x1337, ad) _, err := client.Open(nil, encrypted, 0x42, protocol.KeyPhaseZero, ad) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) }) Context("key updates", func() { @@ -103,7 +103,7 @@ var _ = Describe("Updatable AEAD", func() { Expect(encrypted0).ToNot(Equal(encrypted1)) // expect opening to fail. The client didn't roll keys yet _, err := client.Open(nil, encrypted1, 0x1337, protocol.KeyPhaseZero, ad) - Expect(err).To(MatchError("cipher: message authentication failed")) + Expect(err).To(MatchError(ErrDecryptionFailed)) client.rollKeys() decrypted, err := client.Open(nil, encrypted1, 0x1337, protocol.KeyPhaseOne, ad) Expect(err).ToNot(HaveOccurred()) diff --git a/session.go b/session.go index 8ae7ace9..d92f70c5 100644 --- a/session.go +++ b/session.go @@ -596,10 +596,12 @@ func (s *session) handleSinglePacket(p *receivedPacket, hdr *wire.Header) bool / // Try again later. wasQueued = true s.tryQueueingUndecryptablePacket(p) - default: + case handshake.ErrDecryptionFailed: // This might be a packet injected by an attacker. // Drop it. - s.logger.Debugf("Dropping packet that could not be unpacked. Unpack error: %s", err) + s.logger.Debugf("Dropping packet that could not be unpacked.") + default: + s.closeLocal(err) } return false } diff --git a/session_test.go b/session_test.go index 9aca5f0c..5399f938 100644 --- a/session_test.go +++ b/session_test.go @@ -553,7 +553,7 @@ var _ = Describe("Session", func() { }) It("drops a packet when unpacking fails", func() { - unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, errors.New("unpack error")) + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, handshake.ErrDecryptionFailed) streamManager.EXPECT().CloseWithError(gomock.Any()) cryptoSetup.EXPECT().Close() packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil) @@ -573,6 +573,27 @@ var _ = Describe("Session", func() { Eventually(sess.Context().Done()).Should(BeClosed()) }) + It("closes the session when unpacking fails for any other reason than a decryption error", func() { + testErr := errors.New("test error") + unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(nil, testErr) + streamManager.EXPECT().CloseWithError(gomock.Any()) + cryptoSetup.EXPECT().Close() + packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil) + done := make(chan struct{}) + go func() { + defer GinkgoRecover() + cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) + Expect(sess.run()).To(MatchError(testErr)) + close(done) + }() + sessionRunner.EXPECT().Retire(gomock.Any()) + sess.handlePacket(getPacket(&wire.ExtendedHeader{ + Header: wire.Header{DestConnectionID: sess.srcConnID}, + PacketNumberLen: protocol.PacketNumberLen1, + }, nil)) + Eventually(sess.Context().Done()).Should(BeClosed()) + }) + It("rejects packets with empty payload", func() { unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any()).Return(&unpackedPacket{ hdr: &wire.ExtendedHeader{},