Repository: pion/rtp
Pull request #203 (2023-01-06 19:53:25)

Fix parsing of VP8 packets with degenerate header
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.


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/203?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Base: **87.60**% // Head: **87.43**% // Decreases project coverage by **`-0.16%`** :warning:
> Coverage data is based on head [(`a6a113e`)](https://codecov.io/gh/pion/rtp/pull/203?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) compared to base [(`d13e8b6`)](https://codecov.io/gh/pion/rtp/commit/d13e8b61b96c5985359fd6cd6d88a3115b22cedd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Patch coverage: 72.72% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
- Coverage   87.60%   87.43%   -0.17%     
==========================================
  Files          20       20              
  Lines        1831     1854      +23     
==========================================
+ Hits         1604     1621      +17     
- Misses        197      201       +4     
- Partials       30       32       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `87.43% <72.72%> (-0.17%)` | :arrow_down: |
| wasm | `86.46% <72.72%> (-0.16%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/rtp/pull/203?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [codecs/vp8\_packet.go](https://codecov.io/gh/pion/rtp/pull/203?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL3ZwOF9wYWNrZXQuZ28=) | `91.08% <72.72%> (-2.95%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/pion/rtp/pull/203?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index cd86929..b831dd3 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/vp9_packet.go b/codecs/vp9_packet.go
index 917e630..533a3b1 100644
--- a/codecs/vp9_packet.go
+++ b/codecs/vp9_packet.go
@@ -207,12 +207,15 @@ func (p *VP9Packet) Unmarshal(packet []byte) ([]byte, error) {
 
 // Picture ID:
 //
-//      +-+-+-+-+-+-+-+-+
+//	+-+-+-+-+-+-+-+-+
+//
 // I:   |M| PICTURE ID  |   M:0 => picture id is 7 bits.
-//      +-+-+-+-+-+-+-+-+   M:1 => picture id is 15 bits.
+//
+//	+-+-+-+-+-+-+-+-+   M:1 => picture id is 15 bits.
+//
 // M:   | EXTENDED PID  |
-//      +-+-+-+-+-+-+-+-+
 //
+//	+-+-+-+-+-+-+-+-+
 func (p *VP9Packet) parsePictureID(packet []byte, pos int) (int, error) {
 	if len(packet) <= pos {
 		return pos, errShortPacket
@@ -245,10 +248,11 @@ func (p *VP9Packet) parseLayerInfo(packet []byte, pos int) (int, error) {
 
 // Layer indices (flexible mode):
 //
-//      +-+-+-+-+-+-+-+-+
+//	+-+-+-+-+-+-+-+-+
+//
 // L:   |  T  |U|  S  |D|
-//      +-+-+-+-+-+-+-+-+
 //
+//	+-+-+-+-+-+-+-+-+
 func (p *VP9Packet) parseLayerInfoCommon(packet []byte, pos int) (int, error) {
 	if len(packet) <= pos {
 		return pos, errShortPacket
@@ -269,12 +273,13 @@ func (p *VP9Packet) parseLayerInfoCommon(packet []byte, pos int) (int, error) {
 
 // Layer indices (non-flexible mode):
 //
-//      +-+-+-+-+-+-+-+-+
+//	+-+-+-+-+-+-+-+-+
+//
 // L:   |  T  |U|  S  |D|
-//      +-+-+-+-+-+-+-+-+
-//      |   TL0PICIDX   |
-//      +-+-+-+-+-+-+-+-+
 //
+//	+-+-+-+-+-+-+-+-+
+//	|   TL0PICIDX   |
+//	+-+-+-+-+-+-+-+-+
 func (p *VP9Packet) parseLayerInfoNonFlexibleMode(packet []byte, pos int) (int, error) {
 	if len(packet) <= pos {
 		return pos, errShortPacket
@@ -287,11 +292,12 @@ func (p *VP9Packet) parseLayerInfoNonFlexibleMode(packet []byte, pos int) (int,
 
 // Reference indices:
 //
-//      +-+-+-+-+-+-+-+-+                P=1,F=1: At least one reference index
+//	+-+-+-+-+-+-+-+-+                P=1,F=1: At least one reference index
+//
 // P,F: | P_DIFF      |N|  up to 3 times          has to be specified.
-//      +-+-+-+-+-+-+-+-+                    N=1: An additional P_DIFF follows
-//                                                current P_DIFF.
 //
+//	+-+-+-+-+-+-+-+-+                    N=1: An additional P_DIFF follows
+//	                                          current P_DIFF.
 func (p *VP9Packet) parseRefIndices(packet []byte, pos int) (int, error) {
 	for {
 		if len(packet) <= pos {
@@ -313,24 +319,30 @@ func (p *VP9Packet) parseRefIndices(packet []byte, pos int) (int, error) {
 
 // Scalability structure (SS):
 //
-//      +-+-+-+-+-+-+-+-+
+//	+-+-+-+-+-+-+-+-+
+//
 // V:   | N_S |Y|G|-|-|-|
-//      +-+-+-+-+-+-+-+-+              -|
+//
+//	+-+-+-+-+-+-+-+-+              -|
+//
 // Y:   |     WIDTH     | (OPTIONAL)    .
-//      +               +               .
-//      |               | (OPTIONAL)    .
-//      +-+-+-+-+-+-+-+-+               . N_S + 1 times
-//      |     HEIGHT    | (OPTIONAL)    .
-//      +               +               .
-//      |               | (OPTIONAL)    .
-//      +-+-+-+-+-+-+-+-+              -|
+//   - +               .
+//     |               | (OPTIONAL)    .
+//     +-+-+-+-+-+-+-+-+               . N_S + 1 times
+//     |     HEIGHT    | (OPTIONAL)    .
+//   - +               .
+//     |               | (OPTIONAL)    .
+//     +-+-+-+-+-+-+-+-+              -|
+//
 // G:   |      N_G      | (OPTIONAL)
-//      +-+-+-+-+-+-+-+-+                           -|
+//
+//	+-+-+-+-+-+-+-+-+                           -|
+//
 // N_G: |  T  |U| R |-|-| (OPTIONAL)                 .
-//      +-+-+-+-+-+-+-+-+              -|            . N_G times
-//      |    P_DIFF     | (OPTIONAL)    . R times    .
-//      +-+-+-+-+-+-+-+-+              -|           -|
 //
+//	+-+-+-+-+-+-+-+-+              -|            . N_G times
+//	|    P_DIFF     | (OPTIONAL)    . R times    .
+//	+-+-+-+-+-+-+-+-+              -|           -|
 func (p *VP9Packet) parseSSData(packet []byte, pos int) (int, error) {
 	if len(packet) <= pos {
 		return pos, errShortPacket
@@ -348,6 +360,10 @@ func (p *VP9Packet) parseSSData(packet []byte, pos int) (int, error) {
 		p.Width = make([]uint16, NS)
 		p.Height = make([]uint16, NS)
 		for i := 0; i < int(NS); i++ {
+			if len(packet) <= (pos + 3) {
+				return pos, errShortPacket
+			}
+
 			p.Width[i] = uint16(packet[pos])<<8 | uint16(packet[pos+1])
 			pos += 2
 			p.Height[i] = uint16(packet[pos])<<8 | uint16(packet[pos+1])
@@ -356,17 +372,30 @@ func (p *VP9Packet) parseSSData(packet []byte, pos int) (int, error) {
 	}
 
 	if p.G {
+		if len(packet) <= pos {
+			return pos, errShortPacket
+		}
+
 		p.NG = packet[pos]
 		pos++
 	}
 
 	for i := 0; i < int(p.NG); i++ {
+		if len(packet) <= pos {
+			return pos, errShortPacket
+		}
+
 		p.PGTID = append(p.PGTID, packet[pos]>>5)
 		p.PGU = append(p.PGU, packet[pos]&0x10 != 0)
 		R := (packet[pos] >> 2) & 0x3
 		pos++
 
 		p.PGPDiff = append(p.PGPDiff, []uint8{})
+
+		if len(packet) <= (pos + int(R) - 1) {
+			return pos, errShortPacket
+		}
+
 		for j := 0; j < int(R); j++ {
 			p.PGPDiff[i] = append(p.PGPDiff[i], packet[pos])
 			pos++


Test patch:
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) {
diff --git a/codecs/vp9_packet_test.go b/codecs/vp9_packet_test.go
index 65861c6..f2c5e2d 100644
--- a/codecs/vp9_packet_test.go
+++ b/codecs/vp9_packet_test.go
@@ -167,6 +167,22 @@ func TestVP9Packet_Unmarshal(t *testing.T) {
 				Payload: []byte{},
 			},
 		},
+		"ScalabilityMissingWidth": {
+			b:   []byte("200"),
+			err: errShortPacket,
+		},
+		"ScalabilityMissingNG": {
+			b:   []byte("b00200000000"),
+			err: errShortPacket,
+		},
+		"ScalabilityMissingTemporalLayerIDs": {
+			b:   []byte("20B0"),
+			err: errShortPacket,
+		},
+		"ScalabilityMissingReferenceIndices": {
+			b:   []byte("20B007"),
+			err: errShortPacket,
+		},
 	}
 	for name, c := range cases {
 		c := c