diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index a02d5cd78..b55cf8b3f 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -1043,14 +1043,18 @@ impl ReceiveContext { let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); if existing_session.is_some() { + // receive_windows only has race conditions with the counter of the remote party. It is theoretically possible that the local host receives counters under new_key_id while the receive_window is still in the process of resetting, but this is very unlikely. If it does happen, two things could happen: + // 1) The received counter is less than what is currently stored in the window, so a valid packet is rejected + // 2) The received counter is greater than what is currently stored in the window, so a valid packet is accepted *but* its counter is deleted from the window so it can be replayed + // 1 is completely acceptable behavior; 2 is unacceptable, but extremely extremely unlikely. Since it is utterly impractical for an adversary to trigger 2 intentionally, and preventing 2 is expensive, we do not currently plan to prevent it. + // if receive_window is ever reimplemented, double check it maintains the above properties. + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + let _ = state.remote_session_id.replace(alice_session_id); - //if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter - //for this reason do not access send_counters without holding the read lock + // if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + // for this reason do not access send_counters without holding the read lock state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); - //receive_windows only has race conditions with the send counter of the remote party. It is theoretically possible that the local host receives counters under new_key_id while the receive_window is still in the process of resetting, but this is both very unlikely and can only ever cause dropped packets under the current implementation of receive_window. - //if receive_window is ever reimplemented, double check it maintains the above property. - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } drop(state); @@ -1184,9 +1188,9 @@ impl ReceiveContext { if !is_new_session { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } let _ = state.offer.take();