From 955621899e2baef0e38a722f0925bcb5614c521b Mon Sep 17 00:00:00 2001 From: rustdesk Date: Mon, 21 Sep 2026 12:29:11 +0800 Subject: [PATCH] tcp: keep the advertisement check armed until an echo arrives The check was spent on the first frame that decrypted, whatever it held. A server tags the messages it chooses to, and a frame carrying no echo reads as `echoed == 0`, which passes; on the way out it also disarmed the check for good, so an attacker who rewrote the advertised version only had to let one untagged frame through to be rid of the only thing that would have caught it. A zero now says nothing either way and leaves the check standing. Only a non-zero echo settles the question, matching or refusing the stream. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab --- src/tcp.rs | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/tcp.rs b/src/tcp.rs index 6673327ac..f53302b37 100644 --- a/src/tcp.rs +++ b/src/tcp.rs @@ -359,15 +359,21 @@ impl Encrypt { self.4 = Some(seen); } - /// The server repeats what it advertised inside its first encrypted message, where it can - /// be neither forged nor stripped. A different value in the clear means the handshake was + /// The server repeats what it advertised inside an encrypted message, where it can be + /// neither forged nor stripped. A different value in the clear means the handshake was /// altered on the way, and the stream is refused rather than run at the version it was - /// pushed down to. No echo is a server from before versions, whose advertisement was none. - fn check_kx_advertised_echo(plain: &[u8], seen: u32) -> Result<(), Error> { + /// pushed down to. Returns whether the echo settled the question: a frame that carries + /// none says nothing either way, and must leave the check armed for the frames after it, + /// since a server tags what it chooses to and an attacker would otherwise only have to + /// let one untagged frame through to be rid of the check. + fn check_kx_advertised_echo(plain: &[u8], seen: u32) -> Result { let echoed = crate::rendezvous_proto::RendezvousMessage::parse_from_bytes(plain) .map(|m| m.kx_advertised) .unwrap_or(0); - if echoed != 0 && echoed != seen { + if echoed == 0 { + return Ok(false); + } + if echoed != seen { return Err(Error::new( ErrorKind::Other, format!( @@ -376,7 +382,7 @@ impl Encrypt { ), )); } - Ok(()) + Ok(true) } /// Version 1: the initiator, the side that sent the sealed key, sends under one subkey and @@ -423,8 +429,10 @@ impl Encrypt { let nonce = FramedStream::get_nonce(self.2); match secretbox::open(bytes, &nonce, self.3.as_ref().unwrap_or(&self.0)) { Ok(res) => { - if let Some(seen) = self.4.take() { - Self::check_kx_advertised_echo(&res, seen)?; + if let Some(seen) = self.4 { + if Self::check_kx_advertised_echo(&res, seen)? { + self.4 = None; + } } bytes.clear(); bytes.put_slice(&res); @@ -585,6 +593,20 @@ mod tests { assert!(first_frame_after(KX_VERSION_LATEST, KX_VERSION_LATEST + 1).is_err()); } + #[test] + fn test_kx_advertised_check_survives_an_untagged_frame() { + let key = secretbox::gen_key(); + let mut server = Encrypt::new(key.clone()); + let mut client = Encrypt::new(key); + client.check_kx_advertised(KX_VERSION_LATEST); + // A message the server does not tag says nothing about the advertisement. + let mut untagged = BytesMut::from(&server.enc(&rendezvous_frame(0))[..]); + client.dec(&mut untagged).unwrap(); + // So the frame that does carry an echo is still held to it. + let mut tagged = BytesMut::from(&server.enc(&rendezvous_frame(KX_VERSION_LATEST + 1))[..]); + assert!(client.dec(&mut tagged).is_err()); + } + #[test] fn test_kx_advertised_check_is_one_shot() { let key = secretbox::gen_key();