Do not decrypt already received packets #554
Conversation
|
@xnorpx this is the fix I believe. |
algesten
force-pushed
the
fix/srtp-replay-attack
branch
from
ab8ac68
to
25c8e29
Compare
|
maybe worth a simple end2end tests |
Problem is that there is no observable difference between the packet being decrypted vs the packet being dropped due to being a dupe. |
| return false; | ||
| } | ||
|
|
||
| !self.packet(seq).received || seq > active.end |
There was a problem hiding this comment.
how long is this window for active.end? I.e. if we had a 2 second burst loss would this seq number be outside the active window and we would drop it?
There was a problem hiding this comment.
It's not a fixed time, but a fixed number of packets. We call it MAX_MISORDER, if packets are arriving in sequence, the window shrinks. If they come out of order, we grow up to MAX_MISORDER.
There was a problem hiding this comment.
ok, so if we have:
SeqNo: 1,2,3,4 200, 201, 202, 203
the 200 packet will fit in the windows still and be inserted
There was a problem hiding this comment.
#[test]
fn nack_jump_larger_than_misorder() {
let mut reg = NackRegister::new(None);
for i in &[1, 2, 3, 4, 200, 201, 202, 203] {
reg.update((*i).into());
}
assert_eq!(Some(103.into()..203.into()), reg.active)
}MAX_MISORDER is 100. So the active window goes from 103-203. I.e. we accept any packet as far back as 103. If you at this state run a nack report, you get 103-199 reported as missing. Sequence numbers 5-102 are silently forgotten.
This is to protect str0m against SRTP replay attacks where already received packets are being repeated. Before this PR, this would force str0m to spend CPU decrypting it over and over again. With this PR, str0m checks the NACK register whether the packet is one we expect before doing the decryption.