SRTP does not include padding bytes

[bixycler]

What did you do?

Establish a peer connection to send a track then call webrtc.TrackLocalStaticRTP.WriteRTP() to write RTP packets with non-empty payload and payload padding (PaddingSize > 1), e.g. with packets like this:

rtp.Packet{
	Header: rtp.Header{
		Version: 2,
		Padding: true,
	},
	PaddingSize: 4,
	Payload: []byte{0x01, 0x02, 0x03},
}

What did you expect?

The sent ciphertext (SRTP) should have corresponding plaintext containing trailing padding bytes, e.g. with plaintext like this:

[160 0 0 0 0 0 0 0 0 0 0 0 1 2 3 0 0 0 4]
// padding bytes ----------------^-^-^ ^----- PaddingSize

What happened?

The sent ciphertext (SRTP) has corresponding plaintext with Header.Padding == true but no padding bytes, e.g. with plaintext like this:

[160 0 0 0 0 0 0 0 0 0 0 0 1 2 3]

which makes the receiver peer wrongly parse the received packet, e.g. as

[160 0 0 0 0 0 0 0 0 0 0 0 1 2 3]
// padding bytes ----------^-^ ^----- PaddingSize

Causes of the problem:

In webrtc.TrackLocalStaticRTP.writeRTP(p *rtp.Packet), the packet is not passed down as a whole but only its header and payload (missing PaddingSize) are passed down via b.writeStream.WriteRTP(&p.Header, p.Payload), to TrackLocalWriter, then to interceptor, then to srtp.SessionSRTP.writeRTP(header *rtp.Header, payload []byte), and finally to srtp.Context.encryptRTP(dst []byte, header *rtp.Header, payload []byte).

Currently, there's only one way to generate padding bytes, that is to marshal RTP packet via rtp.Packet.Marshal()(See pion/rtp#155). But even with marshalled packet, the plaintext will always be unmarshalled and its padding info is thrown away: both webrtc.TrackLocalStaticRTP.Write(b []byte) and srtp.SessionSRTP.write(b []byte) just p.Unmarshal(b) then call the "writeRTP()" counterparts.

Suggested solutions

Either call rtp.Packet.Marshal() or add the padding appending codes to update the payload somewhere before passing it to srtp.

Discussion

1. Usually, padding packets contain only padding bytes. These padding-only packets hide this issue with empty payload. But in the general case of non-empty payload + padding, current system will send wrong SRTPs which will make the receiver peer crash.
2. The public function srtp.Context.EncryptRTP(dst []byte, plaintext []byte, header *rtp.Header) does require a marshalled plaintext, but the private function srtp.Context.encryptRTP(dst []byte, header *rtp.Header, payload []byte) has a shortcut to the payload which can be unmarshalled. So, these interfaces should be re-considered.

[alexpokotilo]

I don't use webrtc.TrackLocalStaticRTP, but for webrtc.TrackLocalStaticSample I ended up adding padding-only packets and packets with payload and padding into rtp.Payloader here https://github.com/alexpokotilo/rtp/tree/packetizer-padding-support
support for webrtc.TrackLocalStaticSample added via https://github.com/alexpokotilo/webrtc/tree/static-sample-padding-support
I don't see these changes widely needed so I don't PR them

[alexpokotilo]

@bixycler I think the simplest way in your case is to add padding to payload manually and set Padding flag in Header.
PaddingSize indeed doesn't work. Moreover I don't understand how it should work in case Packet unmarshalled before interceptor.
But I honestly don't think this is a problem for webrtc.TrackLocalStaticRTP as you always can append padding to rtp payload before you can webrtc.TrackLocalStaticRTP.Write* functions and just set Header's padding field.
I agree that PaddingSize cannot be used to pass packets with padding. But in your case you could just add padding to rtp manually.

I did the same for webrtc.TrackLocalStaticSample. It was a little harder as webrtc.TrackLocalStaticSample works with samples. But it works in anyway

[sirzooro]

PaddingSize field logically belongs to the header, even that it is added to RTP packet as a last byte. I propose to add PaddingSize field to rtp.Header and mark one in rtp.Packet as a deprecated. @Sean-Der what do you think about this?