From 2f1db1c23d823e073b6d15e14ddbbfa68883d785 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 18 Oct 2017 13:54:28 +0700 Subject: [PATCH 1/2] fix data length check in STREAM frame parser We should check if the rest of the STREAM frame contains enough bytes to read the full data length, not if this overflows the MaxPacketSize (which is the maximum packet size we use for sending, and has nothing to do with receiving packets). --- internal/wire/stream_frame.go | 8 ++++++-- internal/wire/stream_frame_test.go | 5 +++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/wire/stream_frame.go b/internal/wire/stream_frame.go index 1da780c3d..75be88802 100644 --- a/internal/wire/stream_frame.go +++ b/internal/wire/stream_frame.go @@ -61,8 +61,11 @@ func ParseStreamFrame(r *bytes.Reader, version protocol.VersionNumber) (*StreamF } } - if dataLen > uint16(protocol.MaxPacketSize) { - return nil, qerr.Error(qerr.InvalidStreamData, "data len too large") + // shortcut to prevent the unneccessary allocation of dataLen bytes + // if the dataLen is larger than the remaining length of the packet + // reading the packet contents would result in EOF when attempting to READ + if int(dataLen) > r.Len() { + return nil, io.EOF } if !frame.DataLenPresent { @@ -72,6 +75,7 @@ func ParseStreamFrame(r *bytes.Reader, version protocol.VersionNumber) (*StreamF if dataLen != 0 { frame.Data = make([]byte, dataLen) if _, err := io.ReadFull(r, frame.Data); err != nil { + // this should never happen, since we already checked the dataLen earlier return nil, err } } diff --git a/internal/wire/stream_frame_test.go b/internal/wire/stream_frame_test.go index 46b293874..d01d15d9b 100644 --- a/internal/wire/stream_frame_test.go +++ b/internal/wire/stream_frame_test.go @@ -2,6 +2,7 @@ package wire import ( "bytes" + "io" "github.com/lucas-clemente/quic-go/internal/protocol" "github.com/lucas-clemente/quic-go/qerr" @@ -168,9 +169,9 @@ var _ = Describe("StreamFrame", func() { }) It("rejects frames to too large dataLen", func() { - b := bytes.NewReader([]byte{0xa0, 0x1, 0xff, 0xf}) + b := bytes.NewReader([]byte{0xa0, 0x1, 0xff, 0xff}) _, err := ParseStreamFrame(b, protocol.VersionWhatever) - Expect(err).To(MatchError(qerr.Error(qerr.InvalidStreamData, "data len too large"))) + Expect(err).To(MatchError(io.EOF)) }) It("rejects frames that overflow the offset", func() { From 5504c47ca50b3fa89efa0f6ae6b546b71199c690 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 18 Oct 2017 13:55:57 +0700 Subject: [PATCH 2/2] reduce the maximum packet size of sent packets to 1200 bytes This is the value the IETF draft mandates for implementations that don't do PMTUD. --- integrationtests/tools/proxy/proxy.go | 4 ++-- internal/protocol/server_parameters.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/integrationtests/tools/proxy/proxy.go b/integrationtests/tools/proxy/proxy.go index a9d0d4cf2..5b1dc21d0 100644 --- a/integrationtests/tools/proxy/proxy.go +++ b/integrationtests/tools/proxy/proxy.go @@ -164,7 +164,7 @@ func (p *QuicProxy) newConnection(cliAddr *net.UDPAddr) (*connection, error) { // runProxy listens on the proxy address and handles incoming packets. func (p *QuicProxy) runProxy() error { for { - buffer := make([]byte, protocol.MaxPacketSize) + buffer := make([]byte, protocol.MaxReceivePacketSize) n, cliaddr, err := p.conn.ReadFromUDP(buffer) if err != nil { return err @@ -211,7 +211,7 @@ func (p *QuicProxy) runProxy() error { // runConnection handles packets from server to a single client func (p *QuicProxy) runConnection(conn *connection) error { for { - buffer := make([]byte, protocol.MaxPacketSize) + buffer := make([]byte, protocol.MaxReceivePacketSize) n, err := conn.ServerConn.Read(buffer) if err != nil { return err diff --git a/internal/protocol/server_parameters.go b/internal/protocol/server_parameters.go index 69ee039f3..8472fe8da 100644 --- a/internal/protocol/server_parameters.go +++ b/internal/protocol/server_parameters.go @@ -2,9 +2,9 @@ package protocol import "time" -// MaxPacketSize is the maximum packet size, including the public header, that we use for sending packets -// This is the value used by Chromium for a QUIC packet sent using IPv6 (for IPv4 it would be 1370) -const MaxPacketSize ByteCount = 1350 +// MaxPacketSize is the maximum packet size that we use for sending packets. +// It includes the QUIC packet header, but excludes the UDP and IP header. +const MaxPacketSize ByteCount = 1200 // NonForwardSecurePacketSizeReduction is the number of bytes a non forward-secure packet has to be smaller than a forward-secure packet // This makes sure that those packets can always be retransmitted without splitting the contained StreamFrames