Skip to content

Commit

Permalink
Fix unmarshal of packets with padding
Browse files Browse the repository at this point in the history
WebRTC clients use padding only packets as probe
packets for bandwidth estimation. As padding was
not removed from the payload, downstream code
trying to parse padding as valid media payload
results in erroneous parsing

Testing:
--------
- Add unit tests for different padding scenarios
- Check that ion-sfu does not try to parse padding only
packets as valid video payload.
  • Loading branch information
boks1971 authored and Sean-Der committed Jun 14, 2023
1 parent 6033c9a commit 8265264
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 1 deletion.
2 changes: 2 additions & 0 deletions AUTHORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ aler9 <46489434+aler9@users.noreply.github.com>
Antoine Baché <antoine.bache@epitech.eu>
Antoine Baché <antoine@tenten.app>
Atsushi Watanabe <atsushi.w@ieee.org>
baiyufei <baiyufei@outlook.com>
Bao Nguyen <bao@n4n.dev>
boks1971 <raja.gobi@tutanota.com>
debiandebiandebian <debiandebiandebiandebian@gmail.com>
ffmiyo <leffmiyo@gmail.com>
Guilherme <gqgs@protonmail.com>
Expand Down
11 changes: 10 additions & 1 deletion packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,17 @@ func (p *Packet) Unmarshal(rawPacket []byte) error {
return err
}

p.Payload = rawPacket[p.PayloadOffset:]
end := len(rawPacket)
if p.Header.Padding {
end -= int(rawPacket[end-1])
}
if end < p.PayloadOffset {
return errTooSmall
}

p.Payload = rawPacket[p.PayloadOffset:end]
p.Raw = rawPacket

return nil
}

Expand Down
103 changes: 103 additions & 0 deletions packet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ func TestBasic(t *testing.T) {
}
parsedPacket := &Packet{
Header: Header{
Padding: false,
Marker: true,
Extension: true,
ExtensionProfile: 1,
Expand Down Expand Up @@ -73,6 +74,108 @@ func TestBasic(t *testing.T) {
}
})
}

// packet with padding
rawPkt = []byte{
0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x98, 0x36, 0xbe, 0x88, 0x04,
}
parsedPacket = &Packet{
Header: Header{
Padding: true,
Marker: true,
Extension: true,
ExtensionProfile: 1,
Extensions: []Extension{
{0, []byte{
0xFF, 0xFF, 0xFF, 0xFF,
}},
},
Version: 2,
PayloadType: 96,
SequenceNumber: 27023,
Timestamp: 3653407706,
SSRC: 476325762,
CSRC: []uint32{},
PayloadOffset: 20,
},
Payload: rawPkt[20:21],
Raw: rawPkt,
}
if err := p.Unmarshal(rawPkt); err != nil {
t.Error(err)
} else if !reflect.DeepEqual(p, parsedPacket) {
t.Errorf("TestBasic padding unmarshal: got %#v, want %#v", p, parsedPacket)
}

// packet with only padding
rawPkt = []byte{
0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x98, 0x36, 0xbe, 0x88, 0x05,
}
parsedPacket = &Packet{
Header: Header{
Padding: true,
Marker: true,
Extension: true,
ExtensionProfile: 1,
Extensions: []Extension{
{0, []byte{
0xFF, 0xFF, 0xFF, 0xFF,
}},
},
Version: 2,
PayloadType: 96,
SequenceNumber: 27023,
Timestamp: 3653407706,
SSRC: 476325762,
CSRC: []uint32{},
PayloadOffset: 20,
},
Payload: []byte{},
Raw: rawPkt,
}
if err := p.Unmarshal(rawPkt); err != nil {
t.Error(err)
} else if !reflect.DeepEqual(p, parsedPacket) {
t.Errorf("TestBasic padding only unmarshal: got %#v, want %#v", p, parsedPacket)
}
if len(p.Payload) != 0 {
t.Errorf("Unmarshal of padding only packet has payload of non-zero length: %d", len(p.Payload))
}

// packet with excessive padding
rawPkt = []byte{
0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x98, 0x36, 0xbe, 0x88, 0x06,
}
parsedPacket = &Packet{
Header: Header{
Padding: true,
Marker: true,
Extension: true,
ExtensionProfile: 1,
Extensions: []Extension{
{0, []byte{
0xFF, 0xFF, 0xFF, 0xFF,
}},
},
Version: 2,
PayloadType: 96,
SequenceNumber: 27023,
Timestamp: 3653407706,
SSRC: 476325762,
CSRC: []uint32{},
},
Payload: []byte{},
}
err := p.Unmarshal(rawPkt)
if err == nil {
t.Fatal("Unmarshal did not error on packet with excessive padding")
}
if !errors.Is(err, errTooSmall) {
t.Errorf("Expected error: %v, got: %v", errTooSmall, err)
}
}

func TestExtension(t *testing.T) {
Expand Down

0 comments on commit 8265264

Please sign in to comment.