From 63dcf7676c7218195928fec4543392bbf5555c84 Mon Sep 17 00:00:00 2001 From: Crimson Date: Thu, 6 Aug 2026 22:52:17 +0500 Subject: [PATCH] fix(config): tighten CIDR, interface and list validation [P1] parse_cidr_v4 split on '/' and read the first two components without checking for a third, so "10.8.1.0/24/garbage" parsed as a valid /24 with the tail silently dropped. The value reaches nftables rules and tc filters, so accepting malformed input meant applying rules derived from a config the operator did not write. Now a second separator is an error, and the negative cases are pinned by a test: missing prefix, empty prefix, double separator, negative and out-of-range prefixes, a malformed octet, an empty string, and a missing address. Interface names were only checked for being non-empty, and only for ifb_name. They are passed straight to ip and tc, so a name that cannot exist produces a confusing runtime failure instead of a config error. Validation now covers amnezia.interface_candidates and traffic.ifb_name: non-empty, within the Linux IFNAMSIZ-1 limit of 15 characters, not '.' or '..', and free of whitespace, control characters, '/' and ':'. interface_patterns get the same rules plus one specific to the matcher: only a single trailing '*' is supported, so a pattern like "*wg" or "a*g*" would have silently never matched anything. A bare "*" is rejected outright - matching every interface is not a fallback, it is a way to attach shaping to the wrong device. container_names are matched exactly against docker ps output, so a name outside Docker's own [a-zA-Z0-9][a-zA-Z0-9_.-]* grammar can never be found. That is now a config error rather than a container that mysteriously never appears. All four ordered lists - container_names, interface_candidates, interface_patterns, wg_binaries - are checked for blank and duplicate entries through one shared helper. A blank entry becomes an unmatchable candidate; a duplicate makes an ordered list lie about its priority. Neither is normalized away silently. Seven tests added, covering accepted and rejected values for each rule. cargo test --workspace: 155 + 4 passed. clippy -D warnings clean. --- traffic_core/src/config.rs | 236 ++++++++++++++++++++++++++++++++++--- 1 file changed, 221 insertions(+), 15 deletions(-) diff --git a/traffic_core/src/config.rs b/traffic_core/src/config.rs index d06f8e5..4a42413 100644 --- a/traffic_core/src/config.rs +++ b/traffic_core/src/config.rs @@ -167,23 +167,30 @@ impl Config { } pub fn validate_config(&self) -> anyhow::Result<()> { - if self.amnezia.container_names.is_empty() { - anyhow::bail!("amnezia.container_names must not be empty"); + check_non_empty_unique(&self.amnezia.container_names, "amnezia.container_names")?; + for name in &self.amnezia.container_names { + validate_container_name(name)?; } - if self.amnezia.interface_candidates.is_empty() { - anyhow::bail!("amnezia.interface_candidates must not be empty"); + + check_non_empty_unique( + &self.amnezia.interface_candidates, + "amnezia.interface_candidates", + )?; + for name in &self.amnezia.interface_candidates { + validate_interface_name(name, "amnezia.interface_candidates")?; } - if self.amnezia.interface_patterns.is_empty() { - anyhow::bail!("amnezia.interface_patterns must not be empty"); - } - if self.amnezia.wg_binaries.is_empty() { - anyhow::bail!("amnezia.wg_binaries must not be empty"); + + check_non_empty_unique( + &self.amnezia.interface_patterns, + "amnezia.interface_patterns", + )?; + for pattern in &self.amnezia.interface_patterns { + validate_interface_pattern(pattern)?; } + + check_non_empty_unique(&self.amnezia.wg_binaries, "amnezia.wg_binaries")?; for binary in &self.amnezia.wg_binaries { // Values are executed inside the container: keep them bare command names. - if binary.is_empty() { - anyhow::bail!("amnezia.wg_binaries must not contain an empty value"); - } if binary.chars().any(char::is_whitespace) { anyhow::bail!("amnezia.wg_binaries must not contain whitespace: {binary}"); } @@ -196,9 +203,7 @@ impl Config { if self.traffic.class_start_id < 10 { anyhow::bail!("traffic.class_start_id must be >= 10"); } - if self.traffic.ifb_name.trim().is_empty() { - anyhow::bail!("traffic.ifb_name must not be empty"); - } + validate_interface_name(&self.traffic.ifb_name, "traffic.ifb_name")?; if self.traffic.peer_limit.trim().is_empty() { anyhow::bail!("traffic.peer_limit must not be empty"); } @@ -505,6 +510,87 @@ fn tool_exists(tool: &str) -> bool { false } +/// Linux caps interface names at `IFNAMSIZ - 1`. +const MAX_INTERFACE_NAME_LEN: usize = 15; + +/// Rejects list entries that are blank or repeated. +/// +/// A blank entry silently becomes an unmatchable candidate, and a duplicate makes the +/// ordered lists lie about their priority, so both are configuration errors rather than +/// something to normalize away. +fn check_non_empty_unique(values: &[String], field: &str) -> anyhow::Result<()> { + if values.is_empty() { + anyhow::bail!("{field} must not be empty"); + } + let mut seen = std::collections::BTreeSet::new(); + for value in values { + if value.trim().is_empty() { + anyhow::bail!("{field} must not contain an empty value"); + } + if !seen.insert(value.as_str()) { + anyhow::bail!("{field} contains a duplicate value: {value}"); + } + } + Ok(()) +} + +/// Validates a name that will be passed to `ip`/`tc` as an interface. +fn validate_interface_name(name: &str, field: &str) -> anyhow::Result<()> { + if name.is_empty() { + anyhow::bail!("{field} must not be empty"); + } + if name.len() > MAX_INTERFACE_NAME_LEN { + anyhow::bail!( + "{field} exceeds the Linux interface name limit of {MAX_INTERFACE_NAME_LEN} characters: {name}" + ); + } + if name == "." || name == ".." { + anyhow::bail!("{field} must not be '.' or '..': {name}"); + } + if let Some(bad) = name + .chars() + .find(|c| c.is_whitespace() || c.is_control() || *c == '/' || *c == ':') + { + anyhow::bail!("{field} contains an invalid character {bad:?}: {name}"); + } + Ok(()) +} + +/// Same rules as an interface name, but a single trailing `*` is allowed. +/// +/// Only the suffix form is supported by the matcher, so a pattern with `*` anywhere else +/// would silently never match. +fn validate_interface_pattern(pattern: &str) -> anyhow::Result<()> { + let field = "amnezia.interface_patterns"; + let stem = pattern.strip_suffix('*').unwrap_or(pattern); + if stem.contains('*') { + anyhow::bail!("{field} supports only a single trailing '*': {pattern}"); + } + if stem.is_empty() { + anyhow::bail!("{field} must not match every interface: {pattern}"); + } + validate_interface_name(stem, field) +} + +/// Docker container names are `[a-zA-Z0-9][a-zA-Z0-9_.-]*`; matching is exact, so a name +/// that cannot exist would silently never be found. +fn validate_container_name(name: &str) -> anyhow::Result<()> { + let field = "amnezia.container_names"; + let mut chars = name.chars(); + let first = chars + .next() + .ok_or_else(|| anyhow::anyhow!("{field} must not contain an empty value"))?; + if !first.is_ascii_alphanumeric() { + anyhow::bail!("{field} must start with an alphanumeric character: {name}"); + } + if let Some(bad) = + chars.find(|c| !(c.is_ascii_alphanumeric() || *c == '_' || *c == '.' || *c == '-')) + { + anyhow::bail!("{field} contains an invalid character {bad:?}: {name}"); + } + Ok(()) +} + pub fn parse_cidr_v4(cidr: &str) -> anyhow::Result<(Ipv4Addr, u8)> { let mut parts = cidr.split('/'); let ip = parts @@ -515,6 +601,11 @@ pub fn parse_cidr_v4(cidr: &str) -> anyhow::Result<(Ipv4Addr, u8)> { .next() .ok_or_else(|| anyhow::anyhow!("invalid cidr: {cidr}"))? .parse::()?; + // Without this, "10.8.1.0/24/garbage" parsed as a valid /24 and the trailing junk was + // silently dropped. + if parts.next().is_some() { + anyhow::bail!("invalid cidr: {cidr} has more than one '/' separator"); + } if prefix > 32 { anyhow::bail!("invalid prefix in cidr: {cidr}"); } @@ -584,6 +675,121 @@ mod tests { assert!(err.is_some()); } + #[test] + fn parse_cidr_accepts_well_formed_values() { + assert_eq!( + parse_cidr_v4("10.8.1.0/24").expect("cidr"), + ("10.8.1.0".parse().expect("ip"), 24) + ); + assert_eq!( + parse_cidr_v4("0.0.0.0/0").expect("cidr"), + ("0.0.0.0".parse().expect("ip"), 0) + ); + } + + /// "10.8.1.0/24/garbage" used to parse as a valid /24 with the tail silently dropped. + #[test] + fn parse_cidr_rejects_malformed_values() { + for bad in [ + "10.8.1.0", + "10.8.1.0/", + "10.8.1.0/24/extra", + "10.8.1.0//24", + "10.8.1.0/-1", + "10.8.1.0/33", + "10.8.1.999/24", + "", + "/24", + ] { + assert!(parse_cidr_v4(bad).is_err(), "must reject cidr: {bad:?}"); + } + } + + #[test] + fn interface_name_rules() { + for good in ["awg0", "wg0", "ifb0", "amnezia0", "abcdefghijklmno"] { + assert!( + validate_interface_name(good, "f").is_ok(), + "must accept: {good}" + ); + } + for bad in [ + "", + "abcdefghijklmnop", + "eth 0", + "eth/0", + "eth:0", + "eth\u{0}0", + ".", + "..", + ] { + assert!( + validate_interface_name(bad, "f").is_err(), + "must reject: {bad:?}" + ); + } + } + + #[test] + fn interface_pattern_rules() { + for good in ["awg*", "wg*", "tun*", "awg0"] { + assert!( + validate_interface_pattern(good).is_ok(), + "must accept: {good}" + ); + } + // A bare '*' would match every interface, and '*' is only supported as a suffix. + for bad in ["*", "a*g*", "*wg", "aw g*"] { + assert!( + validate_interface_pattern(bad).is_err(), + "must reject: {bad:?}" + ); + } + } + + #[test] + fn container_name_rules() { + for good in ["amnezia-awg2", "amnezia_awg", "awg.1", "a"] { + assert!(validate_container_name(good).is_ok(), "must accept: {good}"); + } + for bad in ["", "-awg", ".awg", "amnezia awg", "amnezia/awg"] { + assert!( + validate_container_name(bad).is_err(), + "must reject: {bad:?}" + ); + } + } + + #[test] + fn lists_reject_blank_and_duplicate_entries() { + let mut cfg = base_config(); + cfg.amnezia.container_names = vec!["a".into(), "".into()]; + assert!(cfg.validate_config().is_err()); + + let mut cfg = base_config(); + cfg.amnezia.container_names = vec!["a".into(), "a".into()]; + assert!(cfg.validate_config().is_err()); + + let mut cfg = base_config(); + cfg.amnezia.interface_candidates = vec!["awg0".into(), "awg0".into()]; + assert!(cfg.validate_config().is_err()); + + let mut cfg = base_config(); + cfg.amnezia.wg_binaries = vec!["awg".into(), "awg".into()]; + assert!(cfg.validate_config().is_err()); + } + + #[test] + fn ifb_name_is_validated_as_an_interface() { + let mut cfg = base_config(); + cfg.traffic.ifb_name = "ifb 0".into(); + assert!(cfg.validate_config().is_err()); + + let mut cfg = base_config(); + cfg.traffic.ifb_name = "abcdefghijklmnop".into(); + assert!(cfg.validate_config().is_err()); + } + #[test] fn ip_membership() { let ok = ip_in_cidr("10.8.1.10".parse().expect("ip"), "10.8.1.0/24").expect("cidr");