SRTP does not include padding bytes

bixycler commented 1 year ago

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.

Discussion

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.
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 commented 7 months ago

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 commented 7 months ago

@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 commented 1 month ago

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?

pion / webrtc