Repository navigation
Conversation
rfuchs
left a comment
rfuchs
left a comment
There was a problem hiding this comment.
This has me scratching my head a bit
| mp->rtp->ssrc = htonl(mp->ssrc_out->h.ssrc); | ||
| mp->rtp->seq_num = htons(ntohs(mp->rtp->seq_num) + mp->ssrc_out->seq_diff); | ||
| } | ||
| else if (h->kernelize && mp->rtp && mp->ssrc_out && mp->ssrc_out->seq_diff) { |
There was a problem hiding this comment.
Confused by this case distinction: shouldn't a copy always be made if the packet data is modified?
There was a problem hiding this comment.
Yes, it should. The copy is there because the same packet goes through the handler once per sink, so the header has to be put back after queueing, and for that the queued packet can’t share the buffer.
The fixed egress branch above has the same problem, I checked: with a second sink on the stream (a subscriber), that sink receives the first sink’s fixed egress SSRC instead of the source SSRC, and after the source changes its SSRC it also gets the first sink’s sequence offset. I didn’t touch it here since it’s a separate thing. I can fold both into one branch that queues a private copy and edits SSRC/seq on the copy, leaving the packet untouched. Want that in this PR?
There was a problem hiding this comment.
Yes, it would make more sense to fix it for all use cases and not just one
There was a problem hiding this comment.
Done the other way round: no copy in the handlers at all. The sink loop now gives every sink its own copy of the received packet unless it's the last one, the same as it already did for RTCP, and the handlers keep rewriting in place. Verified with a second subscriber on the stream (tests added) and under ASAN.
| sh->attrs.sender_numbering = shared_key && !sink_substitutes_ssrc(media_in, sh, media_out) | ||
| && !ML_ISSET(media_out->monologue, INJECT_DTMF); | ||
| } |
There was a problem hiding this comment.
Should this rather set must_recrypt = true if either seq or SSRC substitution is active?
There was a problem hiding this comment.
For SSRC substitution yes. I tried it: with SDES keys passed through and no recrypt flag, a fixed egress SSRC currently makes every packet fail authentication at the receiver (SSRC rewritten, tag still the sender’s). Adding the SSRC substitution cases to must_recrypt fixes that, I can include it here.
For the seq offset alone it doesn’t work. With shared keys the outgoing key and SSRC are the sender’s, so re-encrypting a renumbered packet reuses a (key, SSRC, index) the sender already used for a different packet on the other wire, i.e. keystream reuse (RFC 3711 9.1). An earlier version of this change did that and a capture showed the same index encrypted twice with different payloads. rtpengine also keeps no index state for a stream it never encrypted before. That’s why the offset is dropped there and the receiver sees one forward gap instead.
There was a problem hiding this comment.
Ok, this is a bit hard to follow without seeing the on-wire RTP, but in general it feels like this should work without having to special-case anything. With the seq being renumbered, decryption should happen with the original seq, then the seq offset is applied, and then the packet should be re-encrypted with the changed seq. And this should work the same even if the seq offset is zero. If there's a problem with this, I'd rather fix the underlying issue instead of special-casing it for this particular usage.
There was a problem hiding this comment.
I tried exactly that first. On a stream with a key of its own it works; the problem is SDES with the key passed through, where the packet leaves under the sender's master key and the sender's SSRC. On the wire: B sends 10010..10059 while C is injected, and A gets those only inside the mix, under an SSRC of rtpengine's own. After inject stop the offset is -50, so B's 10060 would be decrypted at index 10060 and re-encrypted at index 10010, under B's key and B's SSRC. B already encrypted its own packet 10010 at that index on its leg to rtpengine, with different audio, so the two packets share a keystream, and XOR-ing captures from both legs gives the XOR of the two payloads (RFC 3711 section 9.1: "Any fixed keystream output, generated from the same key and index MUST only be used to encrypt once").
With the offset at zero it does work, and that's what the change does now, as one rule rather than a case for a feature: the offset is applied unless the packet leaves under the sender's own master key and SSRC. Then it stays zero, and whether the packet is re-encrypted or forwarded as it is, A gets B's bytes. A sink with an SSRC of rtpengine's own (echo, transcoding, a fixed egress SSRC) or with another key (DTLS, an SDES key rtpengine generated) renumbers. Only the master key and salt of the packet's own direction count, so an MKI, UNENCRYPTED_SRTCP or a re-invite that changes the other direction's key make no difference. SSRC substitution now always sets must_recrypt, as you suggested, with the same predicate the kernel module uses for ssrc_subst; that also fixes the fixed egress SSRC case I mentioned.
The cost is one forward jump at A, as long as the injection. RFC 3711's index estimate (appendix A) follows a jump of up to 2^15 packets, about eleven minutes at 20 ms, the same as on master's userspace path today. The underlying fix would be for rtpengine to stop sending under the sender's key and SSRC once it has inserted packets of its own, with a fresh SSRC or a key of its own, which the receiver or the SDP would see. I'd keep that for a separate change, if you think it's worth doing.
Two cases still renumber under the sender's SSRC and a key the sender has used. One is DTMF injection, which inserts packets into the sender's stream and does the same on master today. The other is a sender that answers a re-invite with a new key of the same suite during an injection: rtpengine keeps sending to the other side under the old key, and after the injection numbers the sender's packets back into indices it used under it. Carrying the new key on in the answer, as the first answer does, would close that. Tests cover the numbering with and without recrypt, across 65535, with UNENCRYPTED_SRTCP and after re-invites that change one side's key, and I checked the same through the kernel module.
Sorry for the terse description.I didn’t want to write a wall of text After inject stop a WebRTC client goes silent for about the injection length minus 20 seconds. Decoding for the mix lowers seq_diff per packet, so afterwards it is -N. The first packets after the stop go through userspace, which never applied the offset, then the kernel applies -N: the sequence jumps forward and back by N, and Chrome’s replay window is 1024 packets, so the rest is dropped as a replay. Now userspace applies the offset too, on a copy because the next sink needs the header as received. Exception: SDES with the keys passed through. Renumbering breaks the sender’s tag, and re-encrypting under the sender’s key and SSRC reuses indexes the sender already used (tried it, the capture showed it). There the offset is dropped and the receiver sees one forward gap. That’s sender_numbering, same in userspace and kernelize. One more thing, this overlaps with 2195 It stops the seq_diff decrement for packets decoded into an audio player, so after an injection the receiver gets a forward gap of N packets instead of the jump. That works for the replay window, but a gap over 32768 packets (an injection longer than ~11 minutes at 50 pps) breaks the receiver’s ROC estimate and the leg goes silent. So I’d keep the offset and apply it consistently, as here. With a fixed egress SSRC #2195 re-bases the numbering anyway, so its decrement change could be limited to that case. The rest of #2195 doesn’t overlap. |
3c7a736 to
88c7ab7
Compare
The kernel module adds the egress sequence offset to every packet it forwards; userspace passthrough did not, unless a fixed egress SSRC was set. Decoding for an injection mix lowers the offset by one per packet. After `inject stop` the first packets left userspace with their original numbers and the kernel then applied the offset, so the receiver saw the sequence jump forward and back. SRTP derives the packet index from the sequence number, and a receiver discards packets behind its replay window: a WebRTC client, whose window is 1024 packets, went silent for 20 seconds after a 40-second injection. Userspace passthrough now applies the offset in place, as the kernel module does. A stream forwarded under the sender's own SRTP master key and SSRC keeps the sender's numbering instead, whether or not it is re-encrypted on the way: a renumbered packet would repeat, under that key, an index the sender has already used for another packet, so the offset is dropped for that stream, in userspace and in the kernel alike, and the receiver sees a gap the length of the injection. Only the master key and salt the stream arrives and leaves under decide it: an MKI, a session parameter such as unencrypted SRTCP, or a re-invite that replaces the key of the other direction leave that keystream as it is. Echo, transcoding and a fixed egress SSRC send under an SSRC of rtpengine's own and keep the offset, as does a stream that leaves under another key. DTMF injection keeps it as well, since it inserts packets into the sender's stream; with a key passed through unchanged it still repeats indices, as before. The same happens when a sender answers a re-invite with a new key during an injection: rtpengine goes on sending under the former key and, after the injection, numbers the sender's later packets back into indices it used under that key. A sink that substitutes the SSRC rewrites the header the sender authenticated, so it re-encrypts the packet. A fixed egress SSRC on a stream forwarded under the sender's key used to reach the receiver with the sender's authentication tag, and the receiver dropped every packet. The handlers rewrite the packet in place while every sink of the stream is handed the same received buffer, so a fixed egress SSRC, the offset and an injected DTMF event used to reach the next sink too, and after an event the next sink read a header on the handler's stack. The sink loop now starts every sink from the packet as received and gives each one its own copy unless it is the last, as it already did for RTCP.
88c7ab7 to
380e3ec
Compare
2 participants
The kernel module adds the egress sequence offset to every packet it forwards;
userspace passthrough did not, unless a fixed egress SSRC was set. Decoding for
an injection mix lowers the offset by one per packet. After
inject stopthefirst packets left userspace with their original numbers and the kernel then
applied the offset, so the receiver saw the sequence jump forward and back.
SRTP derives the packet index from the sequence number, and a receiver discards
packets behind its replay window: a WebRTC client, whose window is 1024 packets,
went silent for 20 seconds after a 40-second injection.
Userspace passthrough now applies the offset, on a copy of the whole packet
since other sinks share it, for handlers the kernel may forward. A stream
forwarded under the sender's own SRTP key and SSRC keeps the sender's numbering
instead, whether or not it is re-encrypted on the way: a renumbered packet would
repeat an index the sender has used, so the offset is dropped for that stream,
in userspace and in the kernel alike, and the receiver sees a gap. A fixed
egress SSRC, echo, transcoding and DTMF injection number the stream themselves
and keep the offset.
Verification:
t/auto-daemon-tests-inject.plgains nine sequence scenarios (RTP, SRTP, thesender's key with and without
recrypt, a 65535 crossing, repeatedinjections, a fixed egress SSRC across a source change); the first eight fail
on master and pass with the change.
make checkwithRTPENGINE_EXTENDED_TESTS=1: the same results as master.DTMF injection, under SDES keys shared end to end, three runs each against
master and this change: the sequence stays continuous and the kernel keeps
forwarding while the daemon is stopped with SIGSTOP.