Skip to content

Commit

Permalink
Merge pull request #2862 from lucas-clemente/simplify-amplification-l…
Browse files Browse the repository at this point in the history 
…imit

allow an amplification factor of 3.x
  • Loading branch information
@marten-seemann
marten-seemann authored Nov 10, 2020
2 parents 272229a + 0c2f562 commit 59cafab
Show file tree
Hide file tree
Showing 9 changed files with 51 additions and 138 deletions.
1 change: 0 additions & 1 deletion internal/ackhandler/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ type SentPacketHandler interface {

// The SendMode determines if and what kind of packets can be sent.
SendMode() SendMode
AmplificationWindow() protocol.ByteCount
// TimeUntilSend is the time when the next packet should be sent.
// It is used for pacing packets.
TimeUntilSend() time.Time
Expand Down
11 changes: 4 additions & 7 deletions internal/ackhandler/sent_packet_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ func (h *sentPacketHandler) SendMode() SendMode {
numTrackedPackets += h.handshakePackets.history.Len()
}

if h.AmplificationWindow() == 0 {
if h.isAmplificationLimited() {
h.logger.Debugf("Amplification window limited. Received %d bytes, already sent out %d bytes", h.bytesReceived, h.bytesSent)
return SendNone
}
Expand Down Expand Up @@ -733,14 +733,11 @@ func (h *sentPacketHandler) HasPacingBudget() bool {
return h.congestion.HasPacingBudget()
}

func (h *sentPacketHandler) AmplificationWindow() protocol.ByteCount {
func (h *sentPacketHandler) isAmplificationLimited() bool {
if h.peerAddressValidated {
return protocol.MaxByteCount
}
if h.bytesSent >= amplificationFactor*h.bytesReceived {
return 0
return false
}
return amplificationFactor*h.bytesReceived - h.bytesSent
return h.bytesSent >= amplificationFactor*h.bytesReceived
}

func (h *sentPacketHandler) QueueProbePacket(encLevel protocol.EncryptionLevel) bool {
Expand Down
26 changes: 11 additions & 15 deletions internal/ackhandler/sent_packet_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,31 +512,27 @@ var _ = Describe("SentPacketHandler", func() {
handler.SendMode()
})

It("returns SendNone if limited by the 3x limit", func() {
handler.ReceivedBytes(100)
cong.EXPECT().OnPacketSent(gomock.Any(), protocol.ByteCount(300), gomock.Any(), protocol.ByteCount(300), true)
It("limits the window to 3x the bytes received, to avoid amplification attacks", func() {
handler.ReceivedPacket(protocol.EncryptionInitial) // receiving an Initial packet doesn't validate the client's address
handler.ReceivedBytes(200)
cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), true).Times(2)
handler.SentPacket(&Packet{
Length: 300,
PacketNumber: 1,
Length: 599,
EncryptionLevel: protocol.EncryptionInitial,
Frames: []Frame{{Frame: &wire.PingFrame{}}},
SendTime: time.Now(),
})
cong.EXPECT().CanSend(protocol.ByteCount(300)).Return(true).AnyTimes()
Expect(handler.AmplificationWindow()).To(BeZero())
Expect(handler.SendMode()).To(Equal(SendNone))
})

It("limits the window to 3x the bytes received, to avoid amplification attacks", func() {
handler.ReceivedPacket(protocol.EncryptionInitial) // receiving an Initial packet doesn't validate the client's address
cong.EXPECT().OnPacketSent(gomock.Any(), protocol.ByteCount(50), gomock.Any(), protocol.ByteCount(50), true)
cong.EXPECT().CanSend(protocol.ByteCount(599)).Return(true)
Expect(handler.SendMode()).To(Equal(SendAny))
handler.SentPacket(&Packet{
Length: 50,
PacketNumber: 2,
Length: 1,
EncryptionLevel: protocol.EncryptionInitial,
Frames: []Frame{{Frame: &wire.PingFrame{}}},
SendTime: time.Now(),
})
handler.ReceivedBytes(100)
Expect(handler.AmplificationWindow()).To(Equal(protocol.ByteCount(3*100 - 50)))
Expect(handler.SendMode()).To(Equal(SendNone))
})

It("allows sending of ACKs when congestion limited", func() {
Expand Down
14 changes: 0 additions & 14 deletions internal/mocks/ackhandler/sent_packet_handler.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions mock_packer_test.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 5 additions & 9 deletions packet_packer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
)

type packer interface {
PackCoalescedPacket(protocol.ByteCount) (*coalescedPacket, error)
PackCoalescedPacket() (*coalescedPacket, error)
PackPacket() (*packedPacket, error)
MaybePackProbePacket(protocol.EncryptionLevel) (*packedPacket, error)
MaybePackAckPacket(handshakeConfirmed bool) (*packedPacket, error)
Expand Down Expand Up @@ -331,9 +331,9 @@ func (p *packetPacker) maybePadPacket(firstPacket *packetContents, buffer *packe
// PackCoalescedPacket packs a new packet.
// It packs an Initial / Handshake if there is data to send in these packet number spaces.
// It should only be called before the handshake is confirmed.
func (p *packetPacker) PackCoalescedPacket(maxPacketSize protocol.ByteCount) (*coalescedPacket, error) {
func (p *packetPacker) PackCoalescedPacket() (*coalescedPacket, error) {
buffer := getPacketBuffer()
packet, err := p.packCoalescedPacket(buffer, maxPacketSize)
packet, err := p.packCoalescedPacket(buffer)
if err != nil {
return nil, err
}
Expand All @@ -347,15 +347,11 @@ func (p *packetPacker) PackCoalescedPacket(maxPacketSize protocol.ByteCount) (*c
return packet, nil
}

func (p *packetPacker) packCoalescedPacket(buffer *packetBuffer, maxPacketSize protocol.ByteCount) (*coalescedPacket, error) {
maxPacketSize = utils.MinByteCount(maxPacketSize, p.maxPacketSize)
func (p *packetPacker) packCoalescedPacket(buffer *packetBuffer) (*coalescedPacket, error) {
maxPacketSize := p.maxPacketSize
if p.perspective == protocol.PerspectiveClient {
maxPacketSize = protocol.MinInitialPacketSize
}
if maxPacketSize < protocol.MinCoalescedPacketSize {
return nil, nil
}

packet := &coalescedPacket{
buffer: buffer,
packets: make([]*packetContents, 0, 3),
Expand Down
90 changes: 16 additions & 74 deletions packet_packer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ var _ = Describe("Packet packer", func() {
expectAppendControlFrames()
f := &wire.StreamFrame{Data: []byte{0xde, 0xca, 0xfb, 0xad}}
expectAppendStreamFrames(ackhandler.Frame{Frame: f})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p).ToNot(BeNil())
Expect(p.packets).To(HaveLen(1))
Expand Down Expand Up @@ -286,7 +286,7 @@ var _ = Describe("Packet packer", func() {
framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).DoAndReturn(func(frames []ackhandler.Frame, _ protocol.ByteCount) ([]ackhandler.Frame, protocol.ByteCount) {
return frames, 0
})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(p).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expand Down Expand Up @@ -553,7 +553,7 @@ var _ = Describe("Packet packer", func() {
packer.retransmissionQueue.AddHandshake(&wire.PingFrame{})
handshakeStream.EXPECT().HasData()
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false)
packet, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
packet, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(packet).ToNot(BeNil())
Expect(packet.packets).To(HaveLen(1))
Expand Down Expand Up @@ -812,7 +812,7 @@ var _ = Describe("Packet packer", func() {
sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped)
sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil)
sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable)
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p).ToNot(BeNil())
checkLength(p.buffer.Data)
Expand All @@ -834,7 +834,7 @@ var _ = Describe("Packet packer", func() {
Expect(f.Length(packer.version)).To(Equal(size))
return f
})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].frames).To(HaveLen(1))
Expand All @@ -861,7 +861,7 @@ var _ = Describe("Packet packer", func() {
handshakeStream.EXPECT().PopCryptoFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) *wire.CryptoFrame {
return &wire.CryptoFrame{Offset: 0x1337, Data: []byte("handshake")}
})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.buffer.Len()).To(BeEquivalentTo(packer.maxPacketSize))
Expect(p.packets).To(HaveLen(2))
Expand Down Expand Up @@ -899,7 +899,7 @@ var _ = Describe("Packet packer", func() {
})
expectAppendControlFrames()
expectAppendStreamFrames(ackhandler.Frame{Frame: &wire.StreamFrame{Data: []byte("foobar")}})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.buffer.Len()).To(BeNumerically(">=", protocol.MinInitialPacketSize))
Expect(p.buffer.Len()).To(BeEquivalentTo(maxPacketSize))
Expand Down Expand Up @@ -936,7 +936,7 @@ var _ = Describe("Packet packer", func() {
})
expectAppendControlFrames()
expectAppendStreamFrames(ackhandler.Frame{Frame: &wire.StreamFrame{Data: []byte("foobar")}})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(2))
Expect(p.packets[0].EncryptionLevel()).To(Equal(protocol.EncryptionHandshake))
Expand Down Expand Up @@ -969,72 +969,14 @@ var _ = Describe("Packet packer", func() {
Expect(f.Length(packer.version)).To(Equal(s))
return f
})
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].EncryptionLevel()).To(Equal(protocol.EncryptionHandshake))
Expect(len(p.buffer.Data)).To(BeEquivalentTo(maxPacketSize - protocol.MinCoalescedPacketSize))
checkLength(p.buffer.Data)
})

It("doesn't pack a coalesced packet if there's not enough space", func() {
p, err := packer.PackCoalescedPacket(protocol.MinCoalescedPacketSize - 1)
Expect(err).ToNot(HaveOccurred())
Expect(p).To(BeNil())
})

It("packs a small packet", func() {
const size = protocol.MinCoalescedPacketSize + 10
pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2)
pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24))
sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped)
sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil)
// don't EXPECT any calls to Get1RTTSealer
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false)
handshakeStream.EXPECT().HasData().Return(true).Times(2)
handshakeStream.EXPECT().PopCryptoFrame(gomock.Any()).DoAndReturn(func(s protocol.ByteCount) *wire.CryptoFrame {
f := &wire.CryptoFrame{Offset: 0x1337}
f.Data = bytes.Repeat([]byte{'f'}, int(s-f.Length(packer.version)-1))
Expect(f.Length(packer.version)).To(Equal(s))
return f
})
p, err := packer.PackCoalescedPacket(size)
Expect(err).ToNot(HaveOccurred())
Expect(p).ToNot(BeNil())
Expect(len(p.buffer.Data)).To(Equal(size))
})

It("packs a small packet, that includes a 1-RTT packet", func() {
const size = 2 * protocol.MinCoalescedPacketSize
pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2)
pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24))
pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2)
pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x24))
sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped)
sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil)
oneRTTSealer := getSealer()
sealingManager.EXPECT().Get1RTTSealer().Return(oneRTTSealer, nil)
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false)
framer.EXPECT().HasData().Return(true)
handshakeStream.EXPECT().HasData().Return(true).Times(2)
handshakeStream.EXPECT().PopCryptoFrame(gomock.Any()).Return(&wire.CryptoFrame{
Offset: 0x1337,
Data: []byte("foobar"),
})
expectAppendControlFrames()
var appDataSize protocol.ByteCount
framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).DoAndReturn(func(frames []ackhandler.Frame, maxSize protocol.ByteCount) ([]ackhandler.Frame, protocol.ByteCount) {
appDataSize = maxSize
f := &wire.StreamFrame{Data: []byte("foobar")}
return append(frames, ackhandler.Frame{Frame: f}), f.Length(packer.version)
})
p, err := packer.PackCoalescedPacket(size)
Expect(err).ToNot(HaveOccurred())
Expect(p).ToNot(BeNil())
Expect(p.packets).To(HaveLen(2))
Expect(appDataSize).To(Equal(size - p.packets[0].length - p.packets[1].header.GetLength(packer.version) - protocol.ByteCount(oneRTTSealer.Overhead())))
})

It("pads if payload length + packet number length is smaller than 4, for Long Header packets", func() {
pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen1)
pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42))
Expand All @@ -1045,7 +987,7 @@ var _ = Describe("Packet packer", func() {
packer.retransmissionQueue.AddHandshake(&wire.PingFrame{})
handshakeStream.EXPECT().HasData()
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake, false)
packet, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
packet, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(packet).ToNot(BeNil())
Expect(packet.packets).To(HaveLen(1))
Expand Down Expand Up @@ -1084,7 +1026,7 @@ var _ = Describe("Packet packer", func() {
sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable)
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial, false)
initialStream.EXPECT().HasData()
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].EncryptionLevel()).To(Equal(protocol.EncryptionInitial))
Expand All @@ -1101,7 +1043,7 @@ var _ = Describe("Packet packer", func() {
sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable)
pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
pnManager.EXPECT().PopPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x42))
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].ack).To(Equal(ack))
Expand All @@ -1113,7 +1055,7 @@ var _ = Describe("Packet packer", func() {
sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable)
initialStream.EXPECT().HasData()
ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial, true)
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p).To(BeNil())
})
Expand All @@ -1129,7 +1071,7 @@ var _ = Describe("Packet packer", func() {
sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable)
pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42))
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].ack).To(Equal(ack))
Expand All @@ -1152,7 +1094,7 @@ var _ = Describe("Packet packer", func() {
initialStream.EXPECT().HasData().Return(true).Times(2)
initialStream.EXPECT().PopCryptoFrame(gomock.Any()).Return(f)
packer.perspective = protocol.PerspectiveClient
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.buffer.Len()).To(BeNumerically(">=", protocol.MinInitialPacketSize))
Expect(p.buffer.Len()).To(BeEquivalentTo(maxPacketSize))
Expand All @@ -1178,7 +1120,7 @@ var _ = Describe("Packet packer", func() {
initialStream.EXPECT().PopCryptoFrame(gomock.Any()).Return(f)
packer.version = protocol.VersionTLS
packer.perspective = protocol.PerspectiveClient
p, err := packer.PackCoalescedPacket(protocol.MaxByteCount)
p, err := packer.PackCoalescedPacket()
Expect(err).ToNot(HaveOccurred())
Expect(p.packets).To(HaveLen(1))
Expect(p.packets[0].ack).To(Equal(ack))
Expand Down
2 changes: 1 addition & 1 deletion session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1553,7 +1553,7 @@ func (s *session) sendPacket() (bool, error) {

if !s.handshakeConfirmed {
now := time.Now()
packet, err := s.packer.PackCoalescedPacket(s.sentPacketHandler.AmplificationWindow())
packet, err := s.packer.PackCoalescedPacket()
if err != nil || packet == nil {
return false, err
}
Expand Down
Loading

0 comments on commit 59cafab

Please sign in to comment.