diff --git a/frames/ack_frame.go b/frames/ack_frame.go index 9c22a953..d88f00c2 100644 --- a/frames/ack_frame.go +++ b/frames/ack_frame.go @@ -14,6 +14,8 @@ type NackRange struct { LastPacketNumber protocol.PacketNumber } +var errInvalidNackRanges = errors.New("AckFrame: ACK frame contains invalid NACK ranges") + // An AckFrame in QUIC type AckFrame struct { Entropy byte @@ -164,6 +166,9 @@ func ParseAckFrame(r *bytes.Reader) (*AckFrame, error) { } } + // Invalid NACK Handling: + // NACKs contain a lot of offsets that require substractions of PacketNumbers. If an ACK contains invalid data, it is possible to underflow the uint64 used to store the PacketNumber + // ToDo: handle uint64 overflows if hasNACK { var numRanges uint8 numRanges, err = r.ReadByte() @@ -185,12 +190,18 @@ func ParseAckFrame(r *bytes.Reader) (*AckFrame, error) { nackRange := NackRange{} if i == 0 { + if uint64(frame.LargestObserved) < missingPacketSequenceNumberDelta+uint64(rangeLength) { + return nil, errInvalidNackRanges + } nackRange.FirstPacketNumber = frame.LargestObserved - protocol.PacketNumber(missingPacketSequenceNumberDelta+uint64(rangeLength)) } else { if missingPacketSequenceNumberDelta == 0 { return nil, errors.New("ACK frame: Continues NACK ranges not yet implemented") } lastNackRange := frame.NackRanges[len(frame.NackRanges)-1] + if uint64(lastNackRange.FirstPacketNumber) <= missingPacketSequenceNumberDelta+uint64(rangeLength) { + return nil, errInvalidNackRanges + } nackRange.FirstPacketNumber = lastNackRange.FirstPacketNumber - protocol.PacketNumber(missingPacketSequenceNumberDelta+uint64(rangeLength)) - 1 } nackRange.LastPacketNumber = protocol.PacketNumber(uint64(nackRange.FirstPacketNumber) + uint64(rangeLength)) @@ -198,5 +209,37 @@ func ParseAckFrame(r *bytes.Reader) (*AckFrame, error) { } } + if !frame.validateNackRanges() { + return nil, errInvalidNackRanges + } + return frame, nil } + +func (f *AckFrame) validateNackRanges() bool { + // check the validity of every single NACK range + for _, nackRange := range f.NackRanges { + if nackRange.FirstPacketNumber > nackRange.LastPacketNumber { + return false + } + if nackRange.LastPacketNumber >= f.LargestObserved { + return false + } + } + + // check the consistency for ACK with multiple NACK ranges + for i, nackRange := range f.NackRanges { + if i == 0 { + continue + } + lastNackRange := f.NackRanges[i-1] + if lastNackRange.FirstPacketNumber <= nackRange.FirstPacketNumber { + return false + } + if lastNackRange.FirstPacketNumber <= nackRange.LastPacketNumber { + return false + } + } + + return true +} diff --git a/frames/ack_frame_test.go b/frames/ack_frame_test.go index 0550be6a..b519b1b6 100644 --- a/frames/ack_frame_test.go +++ b/frames/ack_frame_test.go @@ -76,6 +76,14 @@ var _ = Describe("AckFrame", func() { Expect(frame.NackRanges[2].LastPacketNumber).To(Equal(protocol.PacketNumber(2))) Expect(b.Len()).To(Equal(0)) }) + + It("rejects a packet with an invalid NACK range", func() { + // LargestObserved: 8, NackRange: (8-7-3) to (8-7) + b := bytes.NewReader([]byte{0x60, 0x8, 0x7, 0x72, 0x1, 0x1, 0x0, 0xc0, 0x15, 0x0, 0x0, 0x1, 0x7, 0x3}) + _, err := ParseAckFrame(b) + Expect(err).To(HaveOccurred()) + Expect(err).To(Equal(errInvalidNackRanges)) + }) }) Context("GetHighestInOrderPacket", func() { @@ -107,6 +115,76 @@ var _ = Describe("AckFrame", func() { }) }) + Context("NACK range validator", func() { + It("rejects NACKs with FirstPacketNumber greater than LastPacketNumber", func() { + nackRange := NackRange{FirstPacketNumber: 7, LastPacketNumber: 6} + ack := AckFrame{ + LargestObserved: 10, + NackRanges: []NackRange{nackRange}, + } + Expect(ack.validateNackRanges()).To(BeFalse()) + }) + + It("rejects NACKs with FirstPacketNumber greater than LargestObserved", func() { + nackRange := NackRange{FirstPacketNumber: 6, LastPacketNumber: 6} + ack := AckFrame{ + LargestObserved: 5, + NackRanges: []NackRange{nackRange}, + } + Expect(ack.validateNackRanges()).To(BeFalse()) + }) + + It("rejects NACKs with NackRanges in the wrong order", func() { + nackRanges := []NackRange{ + NackRange{FirstPacketNumber: 2, LastPacketNumber: 2}, + NackRange{FirstPacketNumber: 6, LastPacketNumber: 6}, + } + ack := AckFrame{ + LargestObserved: 7, + NackRanges: nackRanges, + } + Expect(ack.validateNackRanges()).To(BeFalse()) + }) + + It("rejects NACKs with overlapping NackRanges", func() { + nackRanges := []NackRange{ + NackRange{FirstPacketNumber: 5, LastPacketNumber: 6}, + NackRange{FirstPacketNumber: 2, LastPacketNumber: 5}, + } + ack := AckFrame{ + LargestObserved: 7, + NackRanges: nackRanges, + } + Expect(ack.validateNackRanges()).To(BeFalse()) + }) + + It("accepts an ACK without NACK Ranges", func() { + ack := AckFrame{LargestObserved: 7} + Expect(ack.validateNackRanges()).To(BeTrue()) + }) + + It("accepts an ACK with one NACK Ranges", func() { + nackRange := NackRange{FirstPacketNumber: 6, LastPacketNumber: 8} + ack := AckFrame{ + LargestObserved: 10, + NackRanges: []NackRange{nackRange}, + } + Expect(ack.validateNackRanges()).To(BeTrue()) + }) + + It("accepts an ACK with multiple NACK Ranges", func() { + nackRanges := []NackRange{ + NackRange{FirstPacketNumber: 6, LastPacketNumber: 7}, + NackRange{FirstPacketNumber: 2, LastPacketNumber: 4}, + } + ack := AckFrame{ + LargestObserved: 10, + NackRanges: nackRanges, + } + Expect(ack.validateNackRanges()).To(BeTrue()) + }) + }) + Context("when writing", func() { It("writes simple frames without NACK ranges", func() { b := &bytes.Buffer{}