Fix overflow in RFC8888

Use int instead of uint16. We assert that values on the wire can't be
greater then math.MaxUint16, but when iterating we would overflow.

This changes everything to int and adds tests

Co-Authored-By: Juho Nurminen <juhonurm@gmail.com>

Author: Sean-Der

rfc8888.go

@@ -93,8 +93,8 @@ func (b CCFeedbackReport) DestinationSSRC() []uint32 {
 }
 
 // Len returns the length of the report in bytes
-func (b *CCFeedbackReport) Len() uint16 {
-	n := uint16(0)
+func (b *CCFeedbackReport) Len() int {
+	n := 0
 	for _, block := range b.ReportBlocks {
 		n += block.len()
 	}
@@ -107,7 +107,7 @@ func (b *CCFeedbackReport) Header() Header {
 		Padding: false,
 		Count:   FormatCCFB,
 		Type:    TypeTransportSpecificFeedback,
-		Length:  b.Len()/4 - 1,
+		Length:  uint16(b.Len()/4 - 1),
 	}
 }
 
@@ -122,7 +122,7 @@ func (b CCFeedbackReport) Marshal() ([]byte, error) {
 	buf := make([]byte, length)
 	copy(buf[:headerLength], headerBuf)
 	binary.BigEndian.PutUint32(buf[headerLength:], b.SenderSSRC)
-	offset := uint16(reportBlockOffset)
+	offset := reportBlockOffset
 	for _, block := range b.ReportBlocks {
 		b, err := block.marshal()
 		if err != nil {
@@ -164,10 +164,10 @@ func (b *CCFeedbackReport) Unmarshal(rawPacket []byte) error {
 
 	b.SenderSSRC = binary.BigEndian.Uint32(rawPacket[headerLength:])
 
-	reportTimestampOffset := uint16(len(rawPacket) - reportTimestampLength)
+	reportTimestampOffset := len(rawPacket) - reportTimestampLength
 	b.ReportTimestamp = binary.BigEndian.Uint32(rawPacket[reportTimestampOffset:])
 
-	offset := uint16(reportBlockOffset)
+	offset := reportBlockOffset
 	b.ReportBlocks = []CCFeedbackReportBlock{}
 	for offset < reportTimestampOffset {
 		var block CCFeedbackReportBlock
@@ -199,12 +199,12 @@ type CCFeedbackReportBlock struct {
 }
 
 // len returns the length of the report block in bytes
-func (b *CCFeedbackReportBlock) len() uint16 {
+func (b *CCFeedbackReportBlock) len() int {
 	n := len(b.MetricBlocks)
 	if n%2 != 0 {
 		n++
 	}
-	return reportsOffset + 2*uint16(n)
+	return reportsOffset + 2*n
 }
 
 func (b CCFeedbackReportBlock) String() string {
@@ -263,13 +263,14 @@ func (b *CCFeedbackReportBlock) unmarshal(rawPacket []byte) error {
 	}
 
 	endSequence := b.BeginSequence + numReportsField
-	numReports := endSequence - b.BeginSequence + 1
+	numReports := int(endSequence - b.BeginSequence + 1)
 
-	if len(rawPacket) < reportsOffset+int(numReports)*2 {
+	if len(rawPacket) < reportsOffset+numReports*2 {
 		return errIncorrectNumReports
 	}
+
 	b.MetricBlocks = make([]CCFeedbackMetricBlock, numReports)
-	for i := uint16(0); i < numReports; i++ {
+	for i := int(0); i < numReports; i++ {
 		var mb CCFeedbackMetricBlock
 		offset := reportsOffset + 2*i
 		if err := mb.unmarshal(rawPacket[offset : offset+2]); err != nil {

rfc8888_test.go

@@ -4,6 +4,7 @@
 package rtcp
 
 import (
+	"bytes"
 	"fmt"
 	"testing"
 
@@ -281,6 +282,16 @@ func TestCCFeedbackReportBlockUnmarshalMarshal(t *testing.T) {
 		assert.Error(t, err)
 		assert.ErrorIs(t, err, errIncorrectNumReports)
 	})
+
+	t.Run("overflowNumReports", func(t *testing.T) {
+		var block CCFeedbackReportBlock
+		data := append([]byte{
+			0, 0, 0, 0, // SSRC
+			0, 0, 0x7F, 0xFB, // begin_seq, num_reports
+		}, bytes.Repeat([]byte{0, 0}, 0x7FFF)...)
+		err := block.unmarshal(data)
+		assert.NoError(t, err)
+	})
 }
 
 func TestCCFeedbackReportUnmarshalMarshal(t *testing.T) {
@@ -396,3 +407,19 @@ func TestCCFeedbackReportUnmarshalMarshal(t *testing.T) {
 		})
 	}
 }
+
+func TestCCFeedbackOverflow(t *testing.T) {
+	p := &CCFeedbackReport{}
+	err := p.Unmarshal(append([]byte{
+		// Header
+		0b10000000, // V = 2
+		205,        // h.Type = TypeTransportSpecificFeedback
+		0, 0,       // h.Length (unused)
+		// SSRC
+		0, 0, 0, 0,
+		// CCFeedbackReportBlock
+		0, 0, 0, 0, 0, 0,
+		0x7F, 0xFB, // numReportsField
+	}, bytes.Repeat([]byte{0, 0}, 0x7FFF)...))
+	assert.ErrorIs(t, err, errReportBlockLength)
+}