From 09bab7fdc1b19bfaf9ed4c9b5385c606e23e8799 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 23:08:30 +0900 Subject: [PATCH 1/2] fix(security): constrain ProxyCommand endpoint token expansion Validate all endpoint tokens at the common expansion boundary before launching the local shell, preventing inventory, environment and library input from adding shell syntax or arguments. Preserve trusted command quoting and pipelines, and document the restricted token alphabet and IPv6 compatibility. Add no-side-effect regressions for host, username and alias tokens, resolved YAML/environment inputs, and a successful IPv6 pipeline. The nine proxy tests pass locally. Refs #326 --- README.md | 4 + docs/man/bssh.1 | 6 + src/ssh/tokio_client/error.rs | 5 + src/ssh/tokio_client/proxy_command.rs | 207 ++++++++++++++++++++++++++ 4 files changed, 222 insertions(+) diff --git a/README.md b/README.md index cb8b6dcfa..8aa8d6a1b 100644 --- a/README.md +++ b/README.md @@ -903,6 +903,10 @@ These options control SSH proxy connection behavior: **ProxyCommand** uses the user's shell so OpenSSH-style quoting, redirection, and pipelines work. `ProxyCommand none` explicitly selects a direct connection. `ProxyCommand` and `ProxyJump` follow OpenSSH's first-obtained ssh_config rule, while command-line `-J` takes precedence over both. +Endpoint data for `%h`, `%n`, `%r`, and `%k` is checked before starting any ProxyCommand, including data from CLI arguments, YAML inventory, environment expansion, and library callers. All four values must be nonempty and contain only ASCII letters, digits, `.`, `_`, `:`, `@`, or `-`, with no leading `-`. Whitespace, shell substitutions, wildcards, quotes, and non-ASCII characters are rejected even when the template quotes the token. Use ASCII/punycode names and unbracketed IPv6 literals; scoped IPv6 (`%zone`) and raw bracketed literals are unsupported at this boundary. Normal aliases and authored quoting, redirection, and pipelines continue to work. ProxyCommand templates (including nested shells or `eval`) remain trusted executable code; endpoint data must never be used as code deliberately by a template. + +`%h`, `%n`, `%r`, `%k`의 값은 CLI, YAML, 환경 변수 및 라이브러리 입력 경로 모두에서 ProxyCommand 실행 전에 검증됩니다. 비어 있지 않은 ASCII 영문자·숫자와 `.`, `_`, `:`, `@`, `-`만 허용하며 첫 문자는 `-`일 수 없습니다. 공백, 셸 치환, 와일드카드 및 비ASCII 문자는 따옴표 안에서도 거부됩니다. IPv6는 대괄호와 영역 식별자 없이 사용하고 국제화 호스트명은 punycode를 사용하세요. 별칭과 명령에 직접 작성한 인용·리다이렉션·파이프라인은 계속 지원됩니다. 명령 템플릿 자체는 신뢰할 수 있는 코드여야 합니다. + *Note: `ProxyUseFdpass yes` is rejected with an actionable error before the command starts. Use a streaming ProxyCommand without `-F`, or set `ProxyUseFdpass no`.* ### Command Execution and Automation Options diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 88f33d4de..93a21e333 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -725,6 +725,12 @@ The command is run by the user's shell, preserving OpenSSH-style quoting, redirection, and pipelines. .br Supported tokens are %%, %h, %k, %n, %p, and %r. +Endpoint values for %h, %n, %r, and %k are validated before spawning, +regardless of whether they came from CLI, inventory, environment, or library APIs. +They must be nonempty ASCII letters/digits or ., _, :, @, - and cannot start +with -. Whitespace, shell substitutions, glob patterns, and quotes are rejected +even in quoted tokens. Use punycode names and unbracketed IPv6 without a zone +identifier. The command template itself remains trusted executable code. .br .I ProxyCommand none explicitly selects a direct connection. When ProxyCommand and ProxyJump both diff --git a/src/ssh/tokio_client/error.rs b/src/ssh/tokio_client/error.rs index 2612c554e..9ded6f4ce 100644 --- a/src/ssh/tokio_client/error.rs +++ b/src/ssh/tokio_client/error.rs @@ -157,6 +157,10 @@ pub enum Error { AuthenticationPromptTimeout { prompt: &'static str, seconds: u64 }, #[error("ProxyCommand '{command}' contains unsupported token '{token}'")] InvalidProxyCommandToken { command: String, token: String }, + #[error( + "ProxyCommand token {token} contains unsafe endpoint data; use nonempty ASCII letters, digits, '.', '_', ':', '@', or '-', without a leading '-'" + )] + UnsafeProxyCommandValue { token: &'static str }, #[error("Failed to start ProxyCommand '{command}': {source}")] ProxyCommandSpawn { command: String, @@ -315,6 +319,7 @@ impl Error { | Self::ConnectionTimeout { .. } | Self::ProtocolNegotiation { .. } | Self::InvalidProxyCommandToken { .. } + | Self::UnsafeProxyCommandValue { .. } | Self::ProxyCommandSpawn { .. } | Self::ProxyCommandFailed { .. } | Self::ProxyUseFdpassUnsupported { .. } diff --git a/src/ssh/tokio_client/proxy_command.rs b/src/ssh/tokio_client/proxy_command.rs index cfeca02e1..1b19c36a8 100644 --- a/src/ssh/tokio_client/proxy_command.rs +++ b/src/ssh/tokio_client/proxy_command.rs @@ -85,11 +85,36 @@ impl ProxyCommandConfig { self } + /// Expand endpoint tokens only when their data is safe in a shell word. + /// + /// Token values must be nonempty ASCII letters/digits or `.`, `_`, `:`, + /// `@`, `-`, and must not begin with `-`. This intentionally rejects shell + /// expansions, whitespace, glob patterns, and option-like values, even + /// inside quotes. Use unbracketed IPv6 and ASCII (punycode) hostnames. + /// The authored command remains trusted executable code, including any + /// explicit reinterpretation of its arguments by `eval` or nested shells. pub fn expand(&self, host: &str, port: u16, remote_user: &str) -> Result { let key_alias = self .host_key_alias .as_deref() .unwrap_or(&self.original_host); + // Enforce this at the last common boundary, not in individual input + // parsers: inventory, environment, and public API data all reach here. + for (token, value) in [ + ("%h", host), + ("%n", self.original_host.as_str()), + ("%r", remote_user), + ("%k", key_alias), + ] { + if value.is_empty() + || value.starts_with('-') + || !value.bytes().all(|byte| { + byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b':' | b'@' | b'-') + }) + { + return Err(Error::UnsafeProxyCommandValue { token }); + } + } let port = port.to_string(); let mut expanded = String::with_capacity(self.command.len()); let mut chars = self.command.chars(); @@ -409,6 +434,188 @@ mod tests { assert!(error.to_string().contains("%x")); } + #[tokio::test] + async fn unsafe_endpoint_tokens_are_rejected_before_any_shell_side_effect() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("proxy-started"); + let substitution = format!("$(touch {})", marker.display()); + let backticks = format!("`touch {}`", marker.display()); + let values = [ + "", + "two words", + "host\toption", + "host\ncommand", + "host\rcommand", + "host\0suffix", + "-oProxyCommand=evil", + "a;b", + "a|b", + "a&b", + "a>b", + "a host = value, + "%n" => config.original_host = value.to_string(), + "%r" => user = value, + "%k" => config.host_key_alias = Some(value.to_string()), + _ => unreachable!(), + } + assert!( + matches!( + spawn_proxy_command(&config, host, 22, user), + Err(Error::UnsafeProxyCommandValue { token: rejected }) if rejected == token + ), + "accepted {token} value {value:?}" + ); + assert!(!marker.exists(), "proxy shell was launched for {token}"); + } + } + } + } + + #[tokio::test] + async fn detailed_inventory_values_cannot_execute_proxy_shell_code() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("inventory-injection"); + for field in ["host", "user"] { + let yaml = format!( + "clusters:\n test:\n nodes:\n - host: target\n user: alice\n {field}: '$(touch {})'\n", + marker.display() + ); + // Build a single occurrence of each key to exercise normal YAML + // deserialization rather than duplicate-key handling. + let yaml = if field == "host" { + yaml.replace( + " - host: target\n user: alice", + " - user: alice", + ) + } else { + yaml.replace(" user: alice\n", "") + }; + let inventory: crate::config::Config = serde_yaml::from_str(&yaml).unwrap(); + let nodes = inventory.resolve_nodes("test").unwrap(); + let node = &nodes[0]; + let proxy = ProxyCommandConfig::new("cat %h %r", "inventory-alias"); + assert!(matches!( + spawn_proxy_command(&proxy, &node.host, node.port, &node.username), + Err(Error::UnsafeProxyCommandValue { .. }) + )); + assert!(!marker.exists()); + } + } + + #[tokio::test] + async fn environment_inventory_tokens_are_checked_after_expansion() { + const PAYLOAD_ENV: &str = "BSSH_TEST_PROXY_TOKEN_PAYLOAD_326"; + if let Ok(payload) = std::env::var(PAYLOAD_ENV) { + for field in ["host", "user"] { + let yaml = if field == "host" { + format!( + "clusters:\n test:\n nodes:\n - host: '${{{PAYLOAD_ENV}}}'\n user: alice\n" + ) + } else { + format!( + "clusters:\n test:\n nodes:\n - host: target\n user: '${{{PAYLOAD_ENV}}}'\n" + ) + }; + let inventory: crate::config::Config = serde_yaml::from_str(&yaml).unwrap(); + let nodes = inventory.resolve_nodes("test").unwrap(); + let node = &nodes[0]; + assert_eq!( + if field == "host" { + &node.host + } else { + &node.username + }, + &payload + ); + let proxy = ProxyCommandConfig::new("cat %h %r", "inventory-alias"); + assert!(matches!( + spawn_proxy_command(&proxy, &node.host, node.port, &node.username), + Err(Error::UnsafeProxyCommandValue { .. }) + )); + } + return; + } + // Give only a subprocess the environment override: parallel tests + // must not observe process-global environment mutation. + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("environment-injection"); + let mut child = tokio::process::Command::new(std::env::current_exe().unwrap()); + child + .args(["--exact", "ssh::tokio_client::proxy_command::tests::environment_inventory_tokens_are_checked_after_expansion", "--nocapture"]) + .env(PAYLOAD_ENV, format!("$(touch {})", marker.display())) + .kill_on_drop(true); + let output = tokio::time::timeout(Duration::from_secs(10), child.output()) + .await + .unwrap() + .unwrap(); + assert!( + output.status.success(), + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("1 passed")); + assert!(!marker.exists()); + } + + #[tokio::test] + async fn ipv6_aliases_and_quoted_pipeline_tokens_remain_literal_arguments() { + let spec = crate::node::parse_node_spec("alice@[2001:db8::1]:2222").unwrap(); + let proxy = ProxyCommandConfig::new( + "printf '%%s\\n' %h '%n' \"%k\" %p %r | tr A-Z a-z", + "Prod_alias-1", + ) + .with_host_key_alias(Some("Key_alias-1".to_string())); + let mut session = + spawn_proxy_command(&proxy, spec.host, spec.port.unwrap(), spec.user.unwrap()).unwrap(); + let mut output = String::new(); + tokio::time::timeout( + Duration::from_secs(5), + session.stream.read_to_string(&mut output), + ) + .await + .unwrap() + .unwrap(); + assert_eq!( + output, + "2001:db8::1\nprod_alias-1\nkey_alias-1\n2222\nalice\n" + ); + } + #[tokio::test] async fn shell_pipeline_is_preserved() { let config = ProxyCommandConfig::new("printf PROXY | tr A-Z a-z", "target"); From 8006f4faaca614a5f81f9325ebd13fd16ce03edf Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Oct 2026 23:16:31 +0900 Subject: [PATCH 2/2] fix: validate only expanded ProxyCommand endpoint tokens Keep the pre-spawn shell data boundary while allowing unused endpoint fields to retain their original values. Escaped literal tokens do not trigger validation. Add regression coverage and update compatibility documentation. Refs #326 --- README.md | 4 +- docs/man/bssh.1 | 3 +- src/ssh/tokio_client/proxy_command.rs | 68 ++++++++++++++++++--------- 3 files changed, 51 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 8aa8d6a1b..8ba28b837 100644 --- a/README.md +++ b/README.md @@ -903,9 +903,9 @@ These options control SSH proxy connection behavior: **ProxyCommand** uses the user's shell so OpenSSH-style quoting, redirection, and pipelines work. `ProxyCommand none` explicitly selects a direct connection. `ProxyCommand` and `ProxyJump` follow OpenSSH's first-obtained ssh_config rule, while command-line `-J` takes precedence over both. -Endpoint data for `%h`, `%n`, `%r`, and `%k` is checked before starting any ProxyCommand, including data from CLI arguments, YAML inventory, environment expansion, and library callers. All four values must be nonempty and contain only ASCII letters, digits, `.`, `_`, `:`, `@`, or `-`, with no leading `-`. Whitespace, shell substitutions, wildcards, quotes, and non-ASCII characters are rejected even when the template quotes the token. Use ASCII/punycode names and unbracketed IPv6 literals; scoped IPv6 (`%zone`) and raw bracketed literals are unsupported at this boundary. Normal aliases and authored quoting, redirection, and pipelines continue to work. ProxyCommand templates (including nested shells or `eval`) remain trusted executable code; endpoint data must never be used as code deliberately by a template. +Endpoint data for `%h`, `%n`, `%r`, and `%k` is checked before starting any ProxyCommand, including data from CLI arguments, YAML inventory, environment expansion, and library callers. Only values actually substituted into the command are checked; unused fields and escaped literal tokens such as `%%h` are unrestricted. Substituted values must be nonempty and contain only ASCII letters, digits, `.`, `_`, `:`, `@`, or `-`, with no leading `-`. Whitespace, shell substitutions, wildcards, quotes, and non-ASCII characters are rejected even when the template quotes the token. Use ASCII/punycode names and unbracketed IPv6 literals; scoped IPv6 (`%zone`) and raw bracketed literals are unsupported at this boundary. Normal aliases and authored quoting, redirection, and pipelines continue to work. ProxyCommand templates (including nested shells or `eval`) remain trusted executable code; endpoint data must never be used as code deliberately by a template. -`%h`, `%n`, `%r`, `%k`의 값은 CLI, YAML, 환경 변수 및 라이브러리 입력 경로 모두에서 ProxyCommand 실행 전에 검증됩니다. 비어 있지 않은 ASCII 영문자·숫자와 `.`, `_`, `:`, `@`, `-`만 허용하며 첫 문자는 `-`일 수 없습니다. 공백, 셸 치환, 와일드카드 및 비ASCII 문자는 따옴표 안에서도 거부됩니다. IPv6는 대괄호와 영역 식별자 없이 사용하고 국제화 호스트명은 punycode를 사용하세요. 별칭과 명령에 직접 작성한 인용·리다이렉션·파이프라인은 계속 지원됩니다. 명령 템플릿 자체는 신뢰할 수 있는 코드여야 합니다. +`%h`, `%n`, `%r`, `%k`의 값은 CLI, YAML, 환경 변수 및 라이브러리 입력 경로 모두에서 ProxyCommand 실행 전에 검증됩니다. 실제로 치환되는 값만 검증하며 사용하지 않는 필드와 `%%h` 같은 리터럴 토큰은 제한하지 않습니다. 비어 있지 않은 ASCII 영문자·숫자와 `.`, `_`, `:`, `@`, `-`만 허용하며 첫 문자는 `-`일 수 없습니다. 공백, 셸 치환, 와일드카드 및 비ASCII 문자는 따옴표 안에서도 거부됩니다. IPv6는 대괄호와 영역 식별자 없이 사용하고 국제화 호스트명은 punycode를 사용하세요. 별칭과 명령에 직접 작성한 인용·리다이렉션·파이프라인은 계속 지원됩니다. 명령 템플릿 자체는 신뢰할 수 있는 코드여야 합니다. *Note: `ProxyUseFdpass yes` is rejected with an actionable error before the command starts. Use a streaming ProxyCommand without `-F`, or set `ProxyUseFdpass no`.* diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 93a21e333..eb29babe3 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -727,7 +727,8 @@ redirection, and pipelines. Supported tokens are %%, %h, %k, %n, %p, and %r. Endpoint values for %h, %n, %r, and %k are validated before spawning, regardless of whether they came from CLI, inventory, environment, or library APIs. -They must be nonempty ASCII letters/digits or ., _, :, @, - and cannot start +Only substituted values are checked; unused fields and escaped literal tokens +(such as %%h) are unrestricted. Substituted values must be nonempty ASCII letters/digits or ., _, :, @, - and cannot start with -. Whitespace, shell substitutions, glob patterns, and quotes are rejected even in quoted tokens. Use punycode names and unbracketed IPv6 without a zone identifier. The command template itself remains trusted executable code. diff --git a/src/ssh/tokio_client/proxy_command.rs b/src/ssh/tokio_client/proxy_command.rs index 1b19c36a8..92f5a998d 100644 --- a/src/ssh/tokio_client/proxy_command.rs +++ b/src/ssh/tokio_client/proxy_command.rs @@ -87,6 +87,7 @@ impl ProxyCommandConfig { /// Expand endpoint tokens only when their data is safe in a shell word. /// + /// Only expanded token values are checked; unused fields are unrestricted. /// Token values must be nonempty ASCII letters/digits or `.`, `_`, `:`, /// `@`, `-`, and must not begin with `-`. This intentionally rejects shell /// expansions, whitespace, glob patterns, and option-like values, even @@ -98,23 +99,6 @@ impl ProxyCommandConfig { .host_key_alias .as_deref() .unwrap_or(&self.original_host); - // Enforce this at the last common boundary, not in individual input - // parsers: inventory, environment, and public API data all reach here. - for (token, value) in [ - ("%h", host), - ("%n", self.original_host.as_str()), - ("%r", remote_user), - ("%k", key_alias), - ] { - if value.is_empty() - || value.starts_with('-') - || !value.bytes().all(|byte| { - byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b':' | b'@' | b'-') - }) - { - return Err(Error::UnsafeProxyCommandValue { token }); - } - } let port = port.to_string(); let mut expanded = String::with_capacity(self.command.len()); let mut chars = self.command.chars(); @@ -133,11 +117,28 @@ impl ProxyCommandConfig { }; match token { '%' => expanded.push('%'), - 'h' => expanded.push_str(host), - 'k' => expanded.push_str(key_alias), - 'n' => expanded.push_str(&self.original_host), 'p' => expanded.push_str(&port), - 'r' => expanded.push_str(remote_user), + 'h' | 'k' | 'n' | 'r' => { + let (token, value) = match token { + 'h' => ("%h", host), + 'k' => ("%k", key_alias), + 'n' => ("%n", self.original_host.as_str()), + _ => ("%r", remote_user), + }; + // Check only data entering the shell program, at the + // common boundary shared by every input path. Literal + // %% escapes and unused endpoint fields need no check. + if value.is_empty() + || value.starts_with('-') + || !value.bytes().all(|byte| { + byte.is_ascii_alphanumeric() + || matches!(byte, b'.' | b'_' | b':' | b'@' | b'-') + }) + { + return Err(Error::UnsafeProxyCommandValue { token }); + } + expanded.push_str(value); + } other => { return Err(Error::InvalidProxyCommandToken { command: self.command.clone(), @@ -434,6 +435,31 @@ mod tests { assert!(error.to_string().contains("%x")); } + #[test] + fn unused_endpoint_fields_and_escaped_tokens_are_not_validated() { + let unsafe_value = "$(touch unused)"; + let config = ProxyCommandConfig::new("nc %h %p", unsafe_value) + .with_host_key_alias(Some(unsafe_value.to_string())); + assert_eq!( + config.expand("target", 22, unsafe_value).unwrap(), + "nc target 22" + ); + + for command in ["fixed-proxy", "echo %%h %%n %%r %%k"] { + let config = ProxyCommandConfig::new(command, unsafe_value) + .with_host_key_alias(Some(unsafe_value.to_string())); + assert_eq!( + config.expand(unsafe_value, 22, unsafe_value).unwrap(), + command.replace("%%", "%") + ); + } + let config = ProxyCommandConfig::new("nc %h %p; echo %r", "alias"); + assert!(matches!( + config.expand("target", 22, unsafe_value), + Err(Error::UnsafeProxyCommandValue { token: "%r" }) + )); + } + #[tokio::test] async fn unsafe_endpoint_tokens_are_rejected_before_any_shell_side_effect() { let dir = tempfile::tempdir().unwrap();