diff --git a/README.md b/README.md index cb8b6dcf..8ba28b83 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. 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 실행 전에 검증됩니다. 실제로 치환되는 값만 검증하며 사용하지 않는 필드와 `%%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`.* ### Command Execution and Automation Options diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 88f33d4d..eb29babe 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -725,6 +725,13 @@ 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. +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. .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 2612c554..9ded6f4c 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 cfeca02e..92f5a998 100644 --- a/src/ssh/tokio_client/proxy_command.rs +++ b/src/ssh/tokio_client/proxy_command.rs @@ -85,6 +85,15 @@ impl ProxyCommandConfig { self } + /// 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 + /// 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 @@ -108,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(), @@ -409,6 +435,213 @@ 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(); + 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");