Merge pull request #865 from telemt/flow

Secure + VersionD Outbound Paddings Fix

Cargo.lock

@@ -2899,7 +2899,7 @@ checksum = "7b2093cf4c8eb1e67749a6762251bc9cd836b6fc171623bd0a9d324d37af2417"
 
 [[package]]
 name = "telemt"
-version = "3.4.21"
+version = "3.4.22"
 dependencies = [
  "aes",
  "anyhow",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "telemt"
-version = "3.4.21"
+version = "3.4.22"
 edition = "2024"
 
 [features]

src/protocol/constants.rs

@@ -246,7 +246,7 @@ pub fn secure_payload_len_from_wire_len(wire_len: usize) -> Option<usize> {
 }
 
 /// Generate padding length for Secure Intermediate protocol.
-/// Telegram Desktop uses a 4-bit random padding length for VersionD packets.
+/// Outbound padding is 1..=3 so a receiver can strip it by 4-byte alignment.
 pub fn secure_padding_len(data_len: usize, rng: &SecureRandom) -> usize {
     debug_assert!(
         is_valid_secure_payload_len(data_len),
@@ -425,15 +425,21 @@ mod tests {
     }
 
     #[test]
-    fn secure_padding_matches_tdesktop_range() {
+    fn secure_padding_never_produces_aligned_total() {
         let rng = SecureRandom::new();
         for data_len in (0..1000).step_by(4) {
             for _ in 0..100 {
                 let padding = secure_padding_len(data_len, &rng);
                 assert!(
-                    padding <= 15,
+                    (1..=3).contains(&padding),
                     "padding out of range: data_len={data_len}, padding={padding}"
                 );
+                assert_ne!(
+                    (data_len + padding) % 4,
+                    0,
+                    "invariant violated: data_len={data_len}, padding={padding}, total={}",
+                    data_len + padding
+                );
             }
         }
     }

src/protocol/framing.rs

@@ -8,8 +8,8 @@ pub(crate) const INTERMEDIATE_QUICKACK_FLAG: u32 = 0x8000_0000;
 /// Payload length mask used by Intermediate and Secure Intermediate headers.
 pub(crate) const INTERMEDIATE_WIRE_LEN_MASK: u32 = 0x7fff_ffff;
 
-/// Maximum random tail length used by Telegram Desktop VersionD packets.
-pub(crate) const SECURE_VERSION_D_PADDING_MAX: usize = 15;
+/// Maximum outbound Secure tail length that keeps wire lengths non-aligned.
+pub(crate) const SECURE_VERSION_D_PADDING_MAX: usize = 3;
 
 /// Parsed Intermediate/Secure Intermediate length header.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -51,9 +51,9 @@ pub(crate) fn secure_version_d_body_len_from_wire_len(wire_len: usize) -> Option
     Some(wire_len - (wire_len % 4))
 }
 
-/// Generate Telegram Desktop-compatible VersionD random tail length.
+/// Generate outbound Secure tail length without ambiguous full-word padding.
 pub(crate) fn secure_version_d_padding_len(rng: &SecureRandom) -> usize {
-    rng.range(SECURE_VERSION_D_PADDING_MAX + 1)
+    rng.range(SECURE_VERSION_D_PADDING_MAX) + 1
 }
 
 #[cfg(test)]

src/stream/frame_codec.rs

@@ -312,7 +312,7 @@ fn encode_secure(frame: &Frame, dst: &mut BytesMut, rng: &SecureRandom) -> io::R
         ));
     }
 
-    // Telegram Desktop VersionD uses a 4-bit random padding length.
+    // Outbound Secure padding avoids full-word tails that readers cannot strip.
     let padding_len = secure_padding_len(data.len(), rng);
 
     let total_len = data.len() + padding_len;
@@ -521,13 +521,7 @@ mod tests {
     use tokio_util::codec::{FramedRead, FramedWrite};
 
     fn assert_secure_decoded_payload(decoded: &[u8], original: &[u8]) {
-        assert!(decoded.starts_with(original));
-        assert!(
-            (original.len()..=original.len() + 12).contains(&decoded.len()),
-            "Secure decoded payload may retain up to 12 bytes of full-word padding, got {}",
-            decoded.len()
-        );
-        assert_eq!(decoded.len() % 4, 0);
+        assert_eq!(decoded, original);
     }
 
     #[tokio::test]
@@ -653,7 +647,7 @@ mod tests {
     }
 
     #[test]
-    fn secure_codec_uses_tdesktop_padding_range_and_jitters_wire_length() {
+    fn secure_codec_uses_non_aligned_padding_and_jitters_wire_length() {
         let codec = SecureCodec::new(Arc::new(SecureRandom::new()));
         let payload = Bytes::from_static(&[1, 2, 3, 4, 5, 6, 7, 8]);
         let mut wire_lens = HashSet::new();
@@ -666,9 +660,10 @@ mod tests {
             let wire_len = u32::from_le_bytes([out[0], out[1], out[2], out[3]]) as usize;
             assert_eq!(out.len(), 4 + wire_len);
             assert!(
-                (payload.len()..=payload.len() + 15).contains(&wire_len),
-                "Secure wire length must be payload+0..15, got {wire_len}"
+                (payload.len() + 1..=payload.len() + 3).contains(&wire_len),
+                "Secure wire length must be payload+1..3, got {wire_len}"
             );
+            assert_ne!(wire_len % 4, 0);
             wire_lens.insert(wire_len);
         }
 

src/stream/frame_stream.rs

@@ -367,7 +367,7 @@ impl<W: AsyncWrite + Unpin> SecureIntermediateFrameWriter<W> {
             ));
         }
 
-        // Telegram Desktop VersionD uses a 4-bit random padding length.
+        // Outbound Secure padding avoids full-word tails that readers cannot strip.
         let padding_len = secure_padding_len(data.len(), &self.rng);
         let padding = self.rng.bytes(padding_len);
 
@@ -633,13 +633,7 @@ mod tests {
     use tokio::time::{Duration, timeout};
 
     fn assert_secure_decoded_payload(decoded: &[u8], original: &[u8]) {
-        assert!(decoded.starts_with(original));
-        assert!(
-            (original.len()..=original.len() + 12).contains(&decoded.len()),
-            "Secure decoded payload may retain up to 12 bytes of full-word padding, got {}",
-            decoded.len()
-        );
-        assert_eq!(decoded.len() % 4, 0);
+        assert_eq!(decoded, original);
     }
 
     #[tokio::test]

src/synlimit_control/iptables.rs

@@ -226,8 +226,9 @@ fn iptables_reject_args() -> Vec<String> {
     ]
 }
 
-pub(super) async fn clear_rules_for_binary(binary: &str) -> Result<(), String> {
+pub(super) async fn clear_rules_for_binary(binary: &str) -> Result<bool, String> {
     let mut errors = Vec::new();
+    let mut removed = false;
     for _ in 0..8 {
         match run_command(
             binary,
@@ -236,7 +237,9 @@ pub(super) async fn clear_rules_for_binary(binary: &str) -> Result<(), String> {
         )
         .await
         {
-            Ok(()) => {}
+            Ok(()) => {
+                removed = true;
+            }
             Err(error) if is_missing_command_or_iptables_rule(&error) => break,
             Err(error) => {
                 errors.push(format!("{binary} delete INPUT jump failed: {error}"));
@@ -244,19 +247,27 @@ pub(super) async fn clear_rules_for_binary(binary: &str) -> Result<(), String> {
             }
         }
     }
-    if let Err(error) = run_command(binary, &["-t", "filter", "-F", IPTABLES_CHAIN], None).await
-        && !is_missing_command_or_iptables_rule(&error)
-    {
-        errors.push(format!("{binary} flush chain failed: {error}"));
+    match run_command(binary, &["-t", "filter", "-F", IPTABLES_CHAIN], None).await {
+        Ok(()) => {
+            removed = true;
+        }
+        Err(error) if is_missing_command_or_iptables_rule(&error) => {}
+        Err(error) => {
+            errors.push(format!("{binary} flush chain failed: {error}"));
+        }
     }
-    if let Err(error) = run_command(binary, &["-t", "filter", "-X", IPTABLES_CHAIN], None).await
-        && !is_missing_command_or_iptables_rule(&error)
-    {
-        errors.push(format!("{binary} delete chain failed: {error}"));
+    match run_command(binary, &["-t", "filter", "-X", IPTABLES_CHAIN], None).await {
+        Ok(()) => {
+            removed = true;
+        }
+        Err(error) if is_missing_command_or_iptables_rule(&error) => {}
+        Err(error) => {
+            errors.push(format!("{binary} delete chain failed: {error}"));
+        }
     }
 
     if errors.is_empty() {
-        Ok(())
+        Ok(removed)
     } else {
         Err(errors.join(", "))
     }
@@ -266,6 +277,7 @@ fn is_missing_command_or_iptables_rule(error: &str) -> bool {
     error.contains("is not available")
         || error.contains("No chain/target/match by that name")
         || error.contains("does not exist")
+        || error.contains("Couldn't load target")
 }
 
 #[cfg(test)]

src/synlimit_control/mod.rs

@@ -39,8 +39,14 @@ async fn wait_for_config_channel_close_and_reconcile(
 }
 
 pub(crate) async fn reconcile_synlimit_rules(cfg: &ProxyConfig) {
-    if let Err(error) = clear_synlimit_rules_all_backends().await {
-        warn!(error = %error, "Failed to clear existing SYN limiter rules before reconcile");
+    match clear_synlimit_rules_all_backends().await {
+        Ok(true) => {
+            warn!("Removed stale SYN limiter rules left by a previous run before reconcile");
+        }
+        Ok(false) => {}
+        Err(error) => {
+            warn!(error = %error, "Failed to clear stale SYN limiter rules before reconcile");
+        }
     }
 
     let targets = synlimit_targets(cfg);
@@ -66,24 +72,40 @@ pub(crate) async fn reconcile_synlimit_rules(cfg: &ProxyConfig) {
     }
 }
 
-pub(crate) async fn clear_synlimit_rules_all_backends() -> Result<(), String> {
+pub(crate) async fn clear_synlimit_rules_all_backends() -> Result<bool, String> {
     if !has_cap_net_admin() {
-        return Ok(());
+        return Ok(false);
     }
 
     let mut errors = Vec::new();
-    if let Err(error) = nftables::clear_rules_all_families().await {
-        errors.push(error);
+    let mut removed = false;
+    match nftables::clear_rules_all_families().await {
+        Ok(value) => {
+            removed |= value;
+        }
+        Err(error) => {
+            errors.push(error);
+        }
     }
-    if let Err(error) = iptables::clear_rules_for_binary("iptables").await {
-        errors.push(error);
+    match iptables::clear_rules_for_binary("iptables").await {
+        Ok(value) => {
+            removed |= value;
+        }
+        Err(error) => {
+            errors.push(error);
+        }
     }
-    if let Err(error) = iptables::clear_rules_for_binary("ip6tables").await {
-        errors.push(error);
+    match iptables::clear_rules_for_binary("ip6tables").await {
+        Ok(value) => {
+            removed |= value;
+        }
+        Err(error) => {
+            errors.push(error);
+        }
     }
 
     if errors.is_empty() {
-        Ok(())
+        Ok(removed)
     } else {
         Err(errors.join("; "))
     }

src/synlimit_control/nftables.rs

@@ -186,26 +186,32 @@ fn push_nft_v6_rules(script: &mut String, target: &SynLimitRule, idx: usize) {
     ));
 }
 
-pub(super) async fn clear_rules_all_families() -> Result<(), String> {
+pub(super) async fn clear_rules_all_families() -> Result<bool, String> {
     let mut errors = Vec::new();
+    let mut removed = false;
     for family in [NftFamily::Inet, NftFamily::Ip, NftFamily::Ip6] {
-        if let Err(error) = run_command(
+        match run_command(
             "nft",
             &["delete", "table", family.as_str(), NFT_TABLE],
             None,
         )
         .await
-            && !is_missing_command_or_nft_table(&error)
         {
-            errors.push(format!(
-                "nft delete table {} {NFT_TABLE} failed: {error}",
-                family.as_str()
-            ));
+            Ok(()) => {
+                removed = true;
+            }
+            Err(error) if is_missing_command_or_nft_table(&error) => {}
+            Err(error) => {
+                errors.push(format!(
+                    "nft delete table {} {NFT_TABLE} failed: {error}",
+                    family.as_str()
+                ));
+            }
         }
     }
 
     if errors.is_empty() {
-        Ok(())
+        Ok(removed)
     } else {
         Err(errors.join(", "))
     }