From 236bd76e733aa899acbbe177582fdd8f7994008c Mon Sep 17 00:00:00 2001 From: Sean DuBois Date: Thu, 9 Nov 2023 12:45:22 -0500 Subject: [PATCH 1/3] 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 --- rfc8888.go | 23 ++++++++++++----------- rfc8888_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/rfc8888.go b/rfc8888.go index 4a4df9a..9a3e337 100644 --- a/rfc8888.go +++ b/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 { diff --git a/rfc8888_test.go b/rfc8888_test.go index 24a8ab8..d0ada9c 100644 --- a/rfc8888_test.go +++ b/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) +} From 8753c485bac350ab3f2e430f744f1aed998655bb Mon Sep 17 00:00:00 2001 From: Sean Date: Thu, 9 Nov 2023 17:47:18 +0000 Subject: [PATCH 2/3] Update AUTHORS.txt --- AUTHORS.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/AUTHORS.txt b/AUTHORS.txt index d87ad60..8378798 100644 --- a/AUTHORS.txt +++ b/AUTHORS.txt @@ -11,6 +11,7 @@ cnderrauber Gabor Pongracz Hugo Arregui Hugo Arregui +Juho Nurminen Juliusz Chroboczek Kevin Wang lllf @@ -21,6 +22,8 @@ Sean DuBois Sean DuBois Simone Gotti Steffen Vogel +tanghao +tanghao Woodrow Douglass # List of contributors not appearing in Git history From 6486bc088ed1d3b1ef92ab4966a54f33ab956e3c Mon Sep 17 00:00:00 2001 From: Sean Date: Thu, 9 Nov 2023 17:47:46 +0000 Subject: [PATCH 3/3] Update AUTHORS.txt --- AUTHORS.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS.txt b/AUTHORS.txt index 8378798..5f55cf2 100644 --- a/AUTHORS.txt +++ b/AUTHORS.txt @@ -18,6 +18,7 @@ lllf Luke Curley Mathis Engelbart Max Hawkins +Sean Sean DuBois Sean DuBois Simone Gotti