From d182aaf4e137672279f4745c5d0d67bbfeebd5f0 Mon Sep 17 00:00:00 2001
From: Juliusz Chroboczek <jch@irif.fr>
Date: Fri, 6 Jan 2023 20:50:53 +0100
Subject: [PATCH] Fix parsing of VP8 packets with degenerate header

All of the fields in the VP8 header except the first byte are
optional.  We used to reject VP8 packets smaller than 4 bytes,
which is incorrect.

There are two cases where such packets may appear on the wire.
GStreamer's WebRTC implementation generates VP8 streams with
no picture id, and one-byte headers.  It will occasionally
generate packets that are below 4 bytes, and which we used
to reject.

The second use case is more theoretical.  According to RFC 7741
Section 4.4, a packetizer may ignore VP8 partition boundaries.
If it splits a packet outside of a partition boundary, it may
generate a packet with S=0 and a one-byte header.
---
 codecs/vp8_packet.go      | 48 +++++++++++++++++++++++++++------------
 codecs/vp8_packet_test.go | 41 ++++++++++++++++++++++-----------
 2 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index 2eb7bb8..f4608d0 100644
--- a/codecs/vp8_packet.go
+++ b/codecs/vp8_packet.go
@@ -127,12 +127,11 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 
 	payloadLen := len(payload)
 
-	if payloadLen < 4 {
-		return nil, errShortPacket
-	}
-
 	payloadIndex := 0
 
+	if payloadIndex >= payloadLen {
+		return nil, errShortPacket
+	}
 	p.X = (payload[payloadIndex] & 0x80) >> 7
 	p.N = (payload[payloadIndex] & 0x20) >> 5
 	p.S = (payload[payloadIndex] & 0x10) >> 4
@@ -141,14 +140,25 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 	payloadIndex++
 
 	if p.X == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		p.I = (payload[payloadIndex] & 0x80) >> 7
 		p.L = (payload[payloadIndex] & 0x40) >> 6
 		p.T = (payload[payloadIndex] & 0x20) >> 5
 		p.K = (payload[payloadIndex] & 0x10) >> 4
 		payloadIndex++
+	} else {
+		p.I = 0
+		p.L = 0
+		p.T = 0
+		p.K = 0
 	}
 
 	if p.I == 1 { // PID present?
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		if payload[payloadIndex]&0x80 > 0 { // M == 1, PID is 16bit
 			p.PictureID = (uint16(payload[payloadIndex]&0x7F) << 8) | uint16(payload[payloadIndex+1])
 			payloadIndex += 2
@@ -156,35 +166,43 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 			p.PictureID = uint16(payload[payloadIndex])
 			payloadIndex++
 		}
-	}
-
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
+	} else {
+		p.PictureID = 0
 	}
 
 	if p.L == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		p.TL0PICIDX = payload[payloadIndex]
 		payloadIndex++
-	}
-
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
+	} else {
+		p.TL0PICIDX = 0
 	}
 
 	if p.T == 1 || p.K == 1 {
+		if payloadIndex >= payloadLen {
+			return nil, errShortPacket
+		}
 		if p.T == 1 {
 			p.TID = payload[payloadIndex] >> 6
 			p.Y = (payload[payloadIndex] >> 5) & 0x1
+		} else {
+			p.TID = 0
+			p.Y = 0
 		}
 		if p.K == 1 {
 			p.KEYIDX = payload[payloadIndex] & 0x1F
+		} else {
+			p.KEYIDX = 0
 		}
 		payloadIndex++
+	} else {
+		p.TID = 0
+		p.Y = 0
+		p.KEYIDX = 0
 	}
 
-	if payloadIndex >= payloadLen {
-		return nil, errShortPacket
-	}
 	p.Payload = payload[payloadIndex:]
 	return p.Payload, nil
 }
diff --git a/codecs/vp8_packet_test.go b/codecs/vp8_packet_test.go
index b173cfb..e7a9610 100644
--- a/codecs/vp8_packet_test.go
+++ b/codecs/vp8_packet_test.go
@@ -27,15 +27,6 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 		t.Fatal("Error should be:", errShortPacket)
 	}
 
-	// Payload smaller than header size
-	raw, err = pck.Unmarshal([]byte{0x00, 0x11, 0x22})
-	if raw != nil {
-		t.Fatal("Result should be nil in case of error")
-	}
-	if !errors.Is(err, errShortPacket) {
-		t.Fatal("Error should be:", errShortPacket)
-	}
-
 	// Normal payload
 	raw, err = pck.Unmarshal([]byte{0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x90})
 	if raw == nil {
@@ -65,11 +56,11 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 
 	// Header size, X and I, PID 16bits
 	raw, err = pck.Unmarshal([]byte{0x80, 0x80, 0x81, 0x00})
-	if raw != nil {
-		t.Fatal("Result should be nil in case of error")
+	if raw == nil {
+		t.Fatal("Result shouldn't be nil in case of success")
 	}
-	if !errors.Is(err, errShortPacket) {
-		t.Fatal("Error should be:", errShortPacket)
+	if err != nil {
+		t.Fatal("Error should be nil in case of success")
 	}
 
 	// Header size, X and L
@@ -107,6 +98,30 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 	if !errors.Is(err, errShortPacket) {
 		t.Fatal("Error should be:", errShortPacket)
 	}
+
+	// According to RFC 7741 Section 4.4, the packetizer need not pay
+	// attention to partition boundaries.  In that case, it may
+	// produce packets with minimal headers.
+
+	// The next two have been witnessed in nature.
+	_, err = pck.Unmarshal([]byte{0x00})
+	if err != nil {
+		t.Errorf("Empty packet with trivial header: %v", err)
+	}
+	_, err = pck.Unmarshal([]byte{0x00, 0x2a, 0x94})
+	if err != nil {
+		t.Errorf("Non-empty packet with trivial header: %v", err)
+	}
+
+	// The following two were invented.
+	_, err = pck.Unmarshal([]byte{0x80, 0x00})
+	if err != nil {
+		t.Errorf("Empty packet with trivial extension: %v", err)
+	}
+	_, err = pck.Unmarshal([]byte{0x80, 0x80, 42})
+	if err != nil {
+		t.Errorf("Header with PictureID: %v", err)
+	}
 }
 
 func TestVP8Payloader_Payload(t *testing.T) {