diff --git a/protocol/server_parameters.go b/protocol/server_parameters.go index 61b62fc1..54f04bf2 100644 --- a/protocol/server_parameters.go +++ b/protocol/server_parameters.go @@ -23,6 +23,10 @@ const InitialCongestionWindow = 32 // session queues for later until it sends a public reset. const MaxUndecryptablePackets = 10 +// PublicResetTimeout is the time to wait before sending a Public Reset when receiving too many undecryptable packets during the handshake +// This timeout allows the Go scheduler to switch to the Go rountine that reads the crypto stream and to escalate the crypto +const PublicResetTimeout = 500 * time.Millisecond + // AckSendDelay is the maximum delay that can be applied to an ACK for a retransmittable packet // This is the value Chromium is using const AckSendDelay = 25 * time.Millisecond diff --git a/session.go b/session.go index c83a87f4..7454ff7b 100644 --- a/session.go +++ b/session.go @@ -76,8 +76,12 @@ type session struct { runClosed chan struct{} closed uint32 // atomic bool - undecryptablePackets []*receivedPacket - aeadChanged chan protocol.EncryptionLevel + // when we receive too many undecryptable packets during the handshake, we send a Public reset + // but only after a time of protocol.PublicResetTimeout has passed + undecryptablePackets []*receivedPacket + receivedTooManyUndecrytablePacketsTime time.Time + + aeadChanged chan protocol.EncryptionLevel nextAckScheduledTime time.Time @@ -250,10 +254,14 @@ runLoop: if err := s.sendPacket(); err != nil { s.close(err) } - if time.Now().Sub(s.lastNetworkActivityTime) >= s.idleTimeout() { + now := time.Now() + if !s.receivedTooManyUndecrytablePacketsTime.IsZero() && s.receivedTooManyUndecrytablePacketsTime.Add(protocol.PublicResetTimeout).Before(now) { + s.close(qerr.Error(qerr.DecryptionFailure, "too many undecryptable packets received")) + } + if now.Sub(s.lastNetworkActivityTime) >= s.idleTimeout() { s.close(qerr.Error(qerr.NetworkIdleTimeout, "No recent network activity.")) } - if !s.cryptoSetup.HandshakeComplete() && time.Now().Sub(s.sessionCreationTime) >= protocol.MaxTimeForCryptoHandshake { + if !s.cryptoSetup.HandshakeComplete() && now.Sub(s.sessionCreationTime) >= protocol.MaxTimeForCryptoHandshake { s.close(qerr.Error(qerr.NetworkIdleTimeout, "Crypto handshake did not complete in time.")) } s.garbageCollectStreams() @@ -276,6 +284,9 @@ func (s *session) maybeResetTimer() { handshakeDeadline := s.sessionCreationTime.Add(protocol.MaxTimeForCryptoHandshake) nextDeadline = utils.MinTime(nextDeadline, handshakeDeadline) } + if !s.receivedTooManyUndecrytablePacketsTime.IsZero() { + nextDeadline = utils.MinTime(nextDeadline, s.receivedTooManyUndecrytablePacketsTime.Add(protocol.PublicResetTimeout)) + } if nextDeadline.Equal(s.currentDeadline) { // No need to reset the timer @@ -774,8 +785,10 @@ func (s *session) tryQueueingUndecryptablePacket(p *receivedPacket) { return } utils.Infof("Queueing packet 0x%x for later decryption", p.publicHeader.PacketNumber) - if len(s.undecryptablePackets)+1 >= protocol.MaxUndecryptablePackets { - s.close(qerr.Error(qerr.DecryptionFailure, "too many undecryptable packets received")) + if len(s.undecryptablePackets)+1 >= protocol.MaxUndecryptablePackets && s.receivedTooManyUndecrytablePacketsTime.IsZero() { + s.receivedTooManyUndecrytablePacketsTime = time.Now() + s.maybeResetTimer() + return } s.undecryptablePackets = append(s.undecryptablePackets, p) } diff --git a/session_test.go b/session_test.go index 674714c6..e4c7aa7c 100644 --- a/session_test.go +++ b/session_test.go @@ -1174,43 +1174,63 @@ var _ = Describe("Session", func() { Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.InvalidCryptoMessageType)) }) - It("sends public reset after too many undecryptable packets", func() { - // Write protocol.MaxUndecryptablePackets and expect a public reset to happen - for i := 0; i < protocol.MaxUndecryptablePackets; i++ { - hdr := &PublicHeader{ - PacketNumber: protocol.PacketNumber(i + 1), + Context("sending a Public Reset when receiving undecryptable packets during the handshake", func() { + It("doesn't immediately send a Public Reset after receiving too many undecryptable packets", func() { + go sess.run() + for i := 0; i < protocol.MaxUndecryptablePackets; i++ { + hdr := &PublicHeader{ + PacketNumber: protocol.PacketNumber(i + 1), + } + sess.handlePacket(&receivedPacket{publicHeader: hdr, data: []byte("foobar")}) } - sess.handlePacket(&receivedPacket{publicHeader: hdr, data: []byte("foobar")}) - } - sess.run() + sess.scheduleSending() + Consistently(func() [][]byte { return mconn.written }).Should(HaveLen(0)) + }) - Expect(mconn.written).To(HaveLen(1)) - Expect(mconn.written[0]).To(ContainSubstring(string([]byte("PRST")))) - Expect(sess.runClosed).To(Receive()) - }) - - It("ignores undecryptable packets after the handshake is complete", func() { - *(*bool)(unsafe.Pointer(reflect.ValueOf(sess.cryptoSetup).Elem().FieldByName("receivedForwardSecurePacket").UnsafeAddr())) = true - for i := 0; i < protocol.MaxUndecryptablePackets; i++ { - hdr := &PublicHeader{ - PacketNumber: protocol.PacketNumber(i + 1), + It("sets a deadline to send a Public Reset after receiving too many undecryptable packets", func() { + go sess.run() + for i := 0; i < protocol.MaxUndecryptablePackets; i++ { + hdr := &PublicHeader{ + PacketNumber: protocol.PacketNumber(i + 1), + } + sess.handlePacket(&receivedPacket{publicHeader: hdr, data: []byte("foobar")}) } - sess.handlePacket(&receivedPacket{publicHeader: hdr, data: []byte("foobar")}) - } - go sess.run() - Consistently(sess.undecryptablePackets).Should(HaveLen(0)) - sess.closeImpl(nil, true) - Eventually(sess.runClosed).Should(Receive()) - }) + Eventually(func() time.Time { return sess.receivedTooManyUndecrytablePacketsTime }).Should(BeTemporally("~", time.Now(), 10*time.Millisecond)) + }) - It("unqueues undecryptable packets for later decryption", func() { - sess.undecryptablePackets = []*receivedPacket{{ - publicHeader: &PublicHeader{PacketNumber: protocol.PacketNumber(42)}, - }} - Expect(sess.receivedPackets).NotTo(Receive()) - sess.tryDecryptingQueuedPackets() - Expect(sess.undecryptablePackets).To(BeEmpty()) - Expect(sess.receivedPackets).To(Receive()) + It("sends a Public Reset after a timeout", func() { + sess.receivedTooManyUndecrytablePacketsTime = time.Now().Add(-protocol.PublicResetTimeout) + go sess.run() + time.Sleep(10 * time.Millisecond) // wait for the run loop to spin up + sess.scheduleSending() // wake up the run loop + Eventually(func() [][]byte { return mconn.written }).Should(HaveLen(1)) + Expect(mconn.written[0]).To(ContainSubstring(string([]byte("PRST")))) + Expect(sess.runClosed).To(Receive()) + }) + + It("ignores undecryptable packets after the handshake is complete", func() { + *(*bool)(unsafe.Pointer(reflect.ValueOf(sess.cryptoSetup).Elem().FieldByName("receivedForwardSecurePacket").UnsafeAddr())) = true + for i := 0; i < protocol.MaxUndecryptablePackets; i++ { + hdr := &PublicHeader{ + PacketNumber: protocol.PacketNumber(i + 1), + } + sess.handlePacket(&receivedPacket{publicHeader: hdr, data: []byte("foobar")}) + } + go sess.run() + Consistently(sess.undecryptablePackets).Should(HaveLen(0)) + sess.closeImpl(nil, true) + Eventually(sess.runClosed).Should(Receive()) + }) + + It("unqueues undecryptable packets for later decryption", func() { + sess.undecryptablePackets = []*receivedPacket{{ + publicHeader: &PublicHeader{PacketNumber: protocol.PacketNumber(42)}, + }} + Expect(sess.receivedPackets).NotTo(Receive()) + sess.tryDecryptingQueuedPackets() + Expect(sess.undecryptablePackets).To(BeEmpty()) + Expect(sess.receivedPackets).To(Receive()) + }) }) It("calls the cryptoChangeCallback when the AEAD changes", func(done Done) {