Repository: pion/rtp
Pull request #63 (2020-05-18 07:03:45)

Fix packet size validation on unmarshal
Avoid panic on parsing invalid packet.
(New test uses Go 1.13 error wrapping. It requires Go 1.13 or later.)

found during investigating #62

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=h1) Report
> Merging [#63](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=desc) into [master](https://codecov.io/gh/pion/rtp/commit/ee3e7ab3d3222745a80bc10bdf6569da843d7bc3&el=desc) will **increase** coverage by `0.14%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/rtp/pull/63/graphs/tree.svg?width=650&height=150&src=pr&token=4KrbiBsspj)](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   88.85%   88.99%   +0.14%     
==========================================
  Files          13       13              
  Lines         628      636       +8     
==========================================
+ Hits          558      566       +8     
  Misses         54       54              
  Partials       16       16              
```

| Flag | Coverage Δ | |
|---|---|---|
| #go | `88.99% <100.00%> (+0.14%)` | :arrow_up: |
| #wasm | `88.99% <100.00%> (+0.14%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packet.go](https://codecov.io/gh/pion/rtp/pull/63/diff?src=pr&el=tree#diff-cGFja2V0Lmdv) | `83.33% <100.00%> (+0.64%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=footer). Last update [ee3e7ab...99ec079](https://codecov.io/gh/pion/rtp/pull/63?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/error.go b/error.go
new file mode 100644
index 0000000..57f47db
--- /dev/null
+++ b/error.go
@@ -0,0 +1,8 @@
+package rtp
+
+import (
+	"errors"
+)
+
+var errHeaderSizeInsufficient = errors.New("RTP header size insufficient")
+var errHeaderSizeInsufficientForExtension = errors.New("RTP header size insufficient for extension")
diff --git a/go.mod b/go.mod
index 1046d76..afd606b 100644
--- a/go.mod
+++ b/go.mod
@@ -1,3 +1,3 @@
 module github.com/pion/rtp
 
-go 1.12
+go 1.13
diff --git a/packet.go b/packet.go
index 6986623..ff6bd2a 100644
--- a/packet.go
+++ b/packet.go
@@ -105,6 +105,11 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 	h.Extension = (rawPacket[0] >> extensionShift & extensionMask) > 0
 	h.CSRC = make([]uint32, rawPacket[0]&ccMask)
 
+	currOffset := csrcOffset + (len(h.CSRC) * csrcLength)
+	if len(rawPacket) < currOffset {
+		return fmt.Errorf("size %d < %d: %w", len(rawPacket), currOffset, errHeaderSizeInsufficient)
+	}
+
 	h.Marker = (rawPacket[1] >> markerShift & markerMask) > 0
 	h.PayloadType = rawPacket[1] & ptMask
 
@@ -112,19 +117,17 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 	h.Timestamp = binary.BigEndian.Uint32(rawPacket[timestampOffset : timestampOffset+timestampLength])
 	h.SSRC = binary.BigEndian.Uint32(rawPacket[ssrcOffset : ssrcOffset+ssrcLength])
 
-	currOffset := csrcOffset + (len(h.CSRC) * csrcLength)
-	if len(rawPacket) < currOffset {
-		return fmt.Errorf("RTP header size insufficient; %d < %d", len(rawPacket), currOffset)
-	}
-
 	for i := range h.CSRC {
 		offset := csrcOffset + (i * csrcLength)
 		h.CSRC[i] = binary.BigEndian.Uint32(rawPacket[offset:])
 	}
 
 	if h.Extension {
-		if len(rawPacket) < currOffset+4 {
-			return fmt.Errorf("RTP header size insufficient for extension; %d < %d", len(rawPacket), currOffset)
+		if expected := currOffset + 4; len(rawPacket) < expected {
+			return fmt.Errorf("size %d < %d: %w",
+				len(rawPacket), expected,
+				errHeaderSizeInsufficientForExtension,
+			)
 		}
 
 		h.ExtensionProfile = binary.BigEndian.Uint16(rawPacket[currOffset:])
@@ -132,6 +135,13 @@ func (h *Header) Unmarshal(rawPacket []byte) error {
 		extensionLength := int(binary.BigEndian.Uint16(rawPacket[currOffset:])) * 4
 		currOffset += 2
 
+		if expected := currOffset + extensionLength; len(rawPacket) < expected {
+			return fmt.Errorf("size %d < %d: %w",
+				len(rawPacket), expected,
+				errHeaderSizeInsufficientForExtension,
+			)
+		}
+
 		switch h.ExtensionProfile {
 		// RFC 8285 RTP One Byte Header Extension
 		case extensionProfileOneByte:
@@ -301,7 +311,10 @@ func (h *Header) MarshalTo(buf []byte) (n int, err error) {
 		binary.BigEndian.PutUint16(buf[extHeaderPos+2:extHeaderPos+4], uint16(roundedExtSize/4))
 
 		// add padding to reach 4 bytes boundaries
-		n += roundedExtSize - extSize
+		for i := 0; i < roundedExtSize-extSize; i++ {
+			buf[n] = 0
+			n++
+		}
 	}
 
 	h.PayloadOffset = n


Test patch:
diff --git a/packet_test.go b/packet_test.go
index 89eba2b..a5f502f 100644
--- a/packet_test.go
+++ b/packet_test.go
@@ -3,6 +3,7 @@ package rtp
 import (
 	"bytes"
 	"encoding/hex"
+	"errors"
 	"reflect"
 	"testing"
 )
@@ -247,6 +248,33 @@ func TestRFC8285OneByteMultipleExtensionsWithPadding(t *testing.T) {
 	if !bytes.Equal(ext3, ext3Expect) {
 		t.Errorf("Extension has incorrect data. Got: %v+, Expected: %v+", ext3, ext3Expect)
 	}
+
+	rawPktReMarshal := []byte{
+		0x90, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
+		0x27, 0x82, 0xBE, 0xDE, 0x00, 0x03, 0x10, 0xAA, 0x21, 0xBB,
+		0xBB, 0x33, 0xCC, 0xCC, 0xCC, 0xCC, 0x00, 0x00, // padding is moved to the end by re-marshaling
+		// Payload
+		0x98, 0x36, 0xbe, 0x88, 0x9e,
+	}
+	dstBuf := map[string][]byte{
+		"CleanBuffer": make([]byte, 1000),
+		"DirtyBuffer": make([]byte, 1000),
+	}
+	for i := range dstBuf["DirtyBuffer"] {
+		dstBuf["DirtyBuffer"][i] = 0xFF
+	}
+	for name, buf := range dstBuf {
+		buf := buf
+		t.Run(name, func(t *testing.T) {
+			n, err := p.MarshalTo(buf)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !bytes.Equal(buf[:n], rawPktReMarshal) {
+				t.Errorf("Marshal failed raw \nMarshaled:\n%s\nrawPkt:\n%s", hex.Dump(buf[:n]), hex.Dump(rawPktReMarshal))
+			}
+		})
+	}
 }
 
 func TestRFC8285OneByteMultipleExtensions(t *testing.T) {
@@ -920,6 +948,69 @@ func TestRFC3550SetExtensionShouldRaiseErrorWhenSettingNonzeroID(t *testing.T) {
 		t.Error("SetExtension did not error on invalid id")
 	}
 }
+
+func TestUnmarshal_ErrorHandling(t *testing.T) {
+	cases := map[string]struct {
+		input []byte
+		err   error
+	}{
+		"ShortHeader": {
+			input: []byte{
+				0x80, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, // SSRC (one byte missing)
+			},
+			err: errHeaderSizeInsufficient,
+		},
+		"MissingCSRC": {
+			input: []byte{
+				0x81, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+			},
+			err: errHeaderSizeInsufficient,
+		},
+		"MissingExtension": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+		"MissingExtensionData": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+				0xBE, 0xDE, 0x00, 0x03, // specified to have 3 extensions, but actually not
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+		"MissingExtensionDataPayload": {
+			input: []byte{
+				0x90, 0xe0, 0x69, 0x8f,
+				0xd9, 0xc2, 0x93, 0xda, // timestamp
+				0x1c, 0x64, 0x27, 0x82, // SSRC
+				0xBE, 0xDE, 0x00, 0x01, // have 1 extension
+				0x12, 0x00, // length of the payload is expected to be 3, but actually have only 1
+			},
+			err: errHeaderSizeInsufficientForExtension,
+		},
+	}
+
+	for name, testCase := range cases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			h := &Header{}
+			err := h.Unmarshal(testCase.input)
+			if !errors.Is(err, testCase.err) {
+				t.Errorf("Expected error: %v, got: %v", testCase.err, err)
+			}
+		})
+	}
+}
+
 func TestRoundtrip(t *testing.T) {
 	rawPkt := []byte{
 		0x00, 0x10, 0x23, 0x45, 0x12, 0x34, 0x45, 0x67, 0xCC, 0xDD, 0xEE, 0xFF,