Fix RTP padding length validation

[sirzooro]

Added validation of RTP padding length in received packets. Also check for zero padding length when marshaling.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.93%. Comparing base (bc5124c) to head (cdd3390).

Files	Patch %	Lines
packet.go	0.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   84.10%   83.93%   -0.18%     
==========================================
  Files          24       24              
  Lines        1950     1954       +4     
==========================================
  Hits         1640     1640              
- Misses        253      255       +2     
- Partials       57       59       +2     

Flag	Coverage Δ
go	83.93% <0.00%> (-0.18%)	⬇️
wasm	83.31% <0.00%> (-0.18%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[boks1971]

LGTM

[aler9]

Hello, although the specification says "If the padding bit is set, the packet contains one or more additional padding octets", I don't think it's a good idea to add a strict check on the topic since there are lots of devices that are not behaving accordingly. The only result would be decreasing the compatibility level of this library.

[sirzooro]

Hello, although the specification says "If the padding bit is set, the packet contains one or more additional padding octets", I don't think it's a good idea to add a strict check on the topic since there are lots of devices that are not behaving accordingly. The only result would be decreasing the compatibility level of this library.

I saw this. The problem is that padding length is specified in last byte of the packet, so you never can be fully sure if that last byte is valid padding length or some junk value. I will check how to lessen these checks a bit.

[sirzooro]

@aler9 I have removed second check. Its first part checked for zero length padding, what is allowed now, Second part was redundant, the same check was done below after closing curly bracket. I left one check to make sure that there is at least one byte after header. Is it OK now?

[aler9]

the remaining check, which is

rtp/packet.go

Lines 218 to 220 in cdd3390

	if end <= n {
	return errTooSmall
	}

is almost a duplicate of a check that is present 4 lines below:

rtp/packet.go

Lines 224 to 227 in cdd3390

	if end < n {
	return errTooSmall
	}

The only difference is that it doesn't allow a zero-sized padding, again decreasing compatibility.

At the same time, the check on marshal:

rtp/packet.go

Lines 481 to 483 in cdd3390

	if p.Header.Padding && p.PaddingSize == 0 {
	return 0, errInvalidRTPPadding
	}

Prevents from marshaling invalid packets, which is something that not everyone wants to do, since a user might want to route incoming invalid packets to other peers by unmarshaling and marshaling them.

Therefore, this PR adds mandatory compliance checks which are not particularly useful and furthermore it decreases performance, since the additional lines in Unmarshal() are run during each packet unmarshaling, which is a mandatory operation that might happen hundreds of times per second for each video stream.

If you want to add compliance checks on RTP packets, you are free to do it directly inside your software rather than in the base library.

By the way, this is just my opinion which is in no way a final judgement.

[sirzooro]

the remaining check, which is

rtp/packet.go

Lines 218 to 220 in cdd3390

	if end <= n {
	return errTooSmall
	}

is almost a duplicate of a check that is present 4 lines below:

rtp/packet.go

Lines 224 to 227 in cdd3390

	if end < n {
	return errTooSmall
	}

The only difference is that it doesn't allow a zero-sized padding, again decreasing compatibility.

This remaining check verifies that field with padding length is not part of RTP header. According to RTP RFC "The last octet of the padding contains a count of how many padding octets should be ignored, including itself". In other words. it is not possible to create zero-length padding by setting padding length to zero - you have to set padding bit in RTP header to 0.

At the same time, the check on marshal:

rtp/packet.go

Lines 481 to 483 in cdd3390

	if p.Header.Padding && p.PaddingSize == 0 {
	return 0, errInvalidRTPPadding
	}

Prevents from marshaling invalid packets, which is something that not everyone wants to do, since a user might want to route incoming invalid packets to other peers by unmarshaling and marshaling them.

Attempt to do this causes crash in MarshalTo, I added this check to prevent it.

Therefore, this PR adds mandatory compliance checks which are not particularly useful and furthermore it decreases performance, since the additional lines in Unmarshal() are run during each packet unmarshaling, which is a mandatory operation that might happen hundreds of times per second for each video stream.

If you want to add compliance checks on RTP packets, you are free to do it directly inside your software rather than in the base library.

By the way, this is just my opinion which is in no way a final judgement.