validate NACK ranges when receiving an AckFrame

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
+}

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{}