From 669ebe277f2b446424493b9afda5f95d4707a9d0 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 30 Aug 2026 18:43:49 +0900 Subject: [PATCH 1/3] fix(ssh): finalize config keyword support invariant Collapse accepted SSH config keyword support into Runtime or Unimplemented, audit the exact 91-spelling registry, and preserve shared diagnostic provenance and deduplication across config files and -o overlays. Allow repeated OpenSSH -t flags while retaining boolean runtime consumers and last-option-wins precedence against -T, restoring the key-options regression case. Validated with focused registry, parser, resolver, diagnostics, CLI, authentication, session, transport, forwarding, host-key, source/QoS, rekey, and OpenSSH named regression tests, plus format, check, and Clippy gates. Refs #281 --- src/cli/bssh.rs | 30 +++++++- src/ssh/ssh_config/mod.rs | 26 +++++-- src/ssh/ssh_config/parser/core.rs | 71 ++++++++++++------ src/ssh/ssh_config/parser/mod.rs | 6 +- src/ssh/ssh_config/parser/options/mod.rs | 21 ++---- src/ssh/ssh_config/parser/options/support.rs | 79 ++++++++++++++++---- tests/ssh_compat_output_test.rs | 21 ++++++ 7 files changed, 191 insertions(+), 63 deletions(-) diff --git a/src/cli/bssh.rs b/src/cli/bssh.rs index 04be73bb..f3fbd272 100644 --- a/src/cli/bssh.rs +++ b/src/cli/bssh.rs @@ -299,13 +299,18 @@ pub struct Cli { )] pub quiet: bool, - #[arg(short = 't', long = "tty", help = "Force pseudo-terminal allocation")] + #[arg( + short = 't', + long = "tty", + overrides_with_all = ["force_tty", "no_tty"], + help = "Force pseudo-terminal allocation" + )] pub force_tty: bool, #[arg( short = 'T', long = "no-tty", - conflicts_with = "force_tty", + overrides_with_all = ["force_tty", "no_tty"], help = "Disable pseudo-terminal allocation" )] pub no_tty: bool, @@ -842,6 +847,27 @@ mod tests { ); } + #[test] + fn openssh_tty_flags_accept_repetition_and_last_flag_wins() { + for (args, expected) in [ + (vec!["bssh", "-t", "target"], (true, false, false)), + (vec!["bssh", "-tt", "target"], (true, false, false)), + (vec!["bssh", "-ttq", "target"], (true, false, true)), + (vec!["bssh", "-T", "target"], (false, true, false)), + (vec!["bssh", "-tT", "target"], (false, true, false)), + (vec!["bssh", "-Tt", "target"], (true, false, false)), + ] { + let cli = Cli::try_parse_from(&args) + .unwrap_or_else(|error| panic!("failed to parse {args:?}: {error}")); + + assert_eq!( + (cli.force_tty, cli.no_tty, cli.quiet), + expected, + "unexpected tty mode for {args:?}" + ); + } + } + #[test] fn ssh_option_lookup_is_case_insensitive_and_first_value_wins() { let cli = Cli::try_parse_from([ diff --git a/src/ssh/ssh_config/mod.rs b/src/ssh/ssh_config/mod.rs index 1d619c56..7e8eb250 100644 --- a/src/ssh/ssh_config/mod.rs +++ b/src/ssh/ssh_config/mod.rs @@ -18,7 +18,10 @@ //! configurations, and provide a clean API for SSH connection setup. use anyhow::{Context, Result}; -use std::path::{Path, PathBuf}; +use std::{ + collections::HashSet, + path::{Path, PathBuf}, +}; // Internal modules mod env_cache; @@ -50,6 +53,7 @@ pub use types::SshHostConfig; #[derive(Debug, Clone, Default)] pub struct SshConfig { pub hosts: Vec, + reported_diagnostics: HashSet, } impl SshConfig { @@ -104,8 +108,12 @@ impl SshConfig { /// Parse SSH configuration from a string (without Include support) pub fn parse(content: &str) -> Result { - let hosts = parser::parse(content)?; - Ok(Self { hosts }) + let mut reported_diagnostics = HashSet::new(); + let hosts = parser::parse_with_diagnostics(content, &mut reported_diagnostics)?; + Ok(Self { + hosts, + reported_diagnostics, + }) } /// Prepend command-line `-o` options as a structured `Host *` block. @@ -114,7 +122,7 @@ impl SshConfig { /// accepted command-line options have CLI precedence without bypassing /// the parser's validation or first-obtained merge rules. pub fn apply_cli_options(&mut self, options: &[String]) -> Result<()> { - if let Some(overlay) = parser::parse_cli_options(options)? { + if let Some(overlay) = parser::parse_cli_options(options, &mut self.reported_diagnostics)? { self.hosts.insert(0, overlay); } Ok(()) @@ -122,8 +130,14 @@ impl SshConfig { /// Parse SSH configuration from a file with Include support pub async fn parse_from_file_with_content(path: &Path, content: &str) -> Result { - let hosts = parser::parse_from_file(path, content).await?; - Ok(Self { hosts }) + let mut reported_diagnostics = HashSet::new(); + let hosts = + parser::parse_from_file_with_diagnostics(path, content, &mut reported_diagnostics) + .await?; + Ok(Self { + hosts, + reported_diagnostics, + }) } /// Find configuration for a specific hostname diff --git a/src/ssh/ssh_config/parser/core.rs b/src/ssh/ssh_config/parser/core.rs index 7246e4d3..101815cb 100644 --- a/src/ssh/ssh_config/parser/core.rs +++ b/src/ssh/ssh_config/parser/core.rs @@ -28,28 +28,45 @@ use std::path::Path; use super::options; /// Parse SSH configuration content with Include and Match support +#[cfg(test)] pub fn parse(content: &str) -> Result> { + let mut reported_diagnostics = HashSet::new(); + parse_with_diagnostics(content, &mut reported_diagnostics) +} + +pub(crate) fn parse_with_diagnostics( + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { // For synchronous parsing without file path, we can't resolve includes // This maintains backward compatibility for tests and simple usage - parse_without_includes(content) + parse_without_includes(content, reported_diagnostics) } -/// Parse SSH configuration from a file with full Include support -pub async fn parse_from_file(path: &Path, content: &str) -> Result> { +/// Parse SSH configuration from a file with full Include support. +pub(crate) async fn parse_from_file_with_diagnostics( + path: &Path, + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { // Pass 1: Resolve all Include directives let included_files = resolve_includes(path, content) .await .with_context(|| format!("Failed to resolve includes for {}", path.display()))?; - parse_included_files(&included_files) + parse_included_files(&included_files, reported_diagnostics) } /// Parse SSH configuration content without Include resolution -pub(super) fn parse_without_includes(content: &str) -> Result> { +pub(super) fn parse_without_includes( + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { parse_lines( content .lines() .enumerate() .map(|(index, line)| (None, index + 1, line)), + reported_diagnostics, ) } @@ -58,7 +75,10 @@ pub(super) fn parse_without_includes(content: &str) -> Result /// Keeping the overlay as a structured block lets the ordinary resolver apply /// OpenSSH's first-obtained rule: CLI options are visited before file blocks, /// while repeated `-o` scalars retain the first CLI value. -pub(crate) fn parse_cli_options(options: &[String]) -> Result> { +pub(crate) fn parse_cli_options( + options: &[String], + reported_diagnostics: &mut HashSet, +) -> Result> { const MAX_LINE_LENGTH: usize = 8192; const MAX_VALUE_LENGTH: usize = 4096; @@ -71,8 +91,6 @@ pub(crate) fn parse_cli_options(options: &[String]) -> Result Result Result Result> { - parse_lines(files.iter().flat_map(|file| { - file.content.lines().enumerate().map(move |(index, line)| { - ( - Some(file.path.as_path()), - file.source_line_start + index, - line, - ) - }) - })) +fn parse_included_files( + files: &[IncludedFile], + reported_diagnostics: &mut HashSet, +) -> Result> { + parse_lines( + files.iter().flat_map(|file| { + file.content.lines().enumerate().map(move |(index, line)| { + ( + Some(file.path.as_path()), + file.source_line_start + index, + line, + ) + }) + }), + reported_diagnostics, + ) } fn parse_lines<'a>( lines: impl IntoIterator, usize, &'a str)>, + reported_diagnostics: &mut HashSet, ) -> Result> { // Security: Set reasonable limits to prevent DoS attacks const MAX_LINE_LENGTH: usize = 8192; // 8KB per line should be more than enough @@ -124,8 +149,6 @@ fn parse_lines<'a>( let mut current_config: Option = None; let mut current_match: Option = None; let mut in_match_block = false; - let mut reported_diagnostics = HashSet::new(); - for (source_path, line_number, line) in lines { // Security: Check line length to prevent DoS if line.len() > MAX_LINE_LENGTH { @@ -232,7 +255,7 @@ fn parse_lines<'a>( &args, source_path, line_number, - &mut reported_diagnostics, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } @@ -243,7 +266,7 @@ fn parse_lines<'a>( &args, source_path, line_number, - &mut reported_diagnostics, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } else { @@ -262,7 +285,7 @@ fn parse_lines<'a>( &args, source_path, line_number, - &mut reported_diagnostics, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } diff --git a/src/ssh/ssh_config/parser/mod.rs b/src/ssh/ssh_config/parser/mod.rs index 5a446af3..35bc4b1b 100644 --- a/src/ssh/ssh_config/parser/mod.rs +++ b/src/ssh/ssh_config/parser/mod.rs @@ -28,7 +28,11 @@ mod options; mod tests; // Re-export public items from core module -pub(super) use core::{parse, parse_cli_options, parse_from_file}; +#[cfg(test)] +pub(super) use core::parse; +pub(super) use core::{ + parse_cli_options, parse_from_file_with_diagnostics, parse_with_diagnostics, +}; // Re-export helper functions that might be used elsewhere diff --git a/src/ssh/ssh_config/parser/options/mod.rs b/src/ssh/ssh_config/parser/options/mod.rs index ab60e768..f882068c 100644 --- a/src/ssh/ssh_config/parser/options/mod.rs +++ b/src/ssh/ssh_config/parser/options/mod.rs @@ -57,27 +57,16 @@ pub fn parse_option( }; let keyword = spec.canonical; - let tracking_issue = match spec.support { - support::KeywordSupport::Runtime(_) => None, - support::KeywordSupport::Delegated(issue) => Some(issue), - support::KeywordSupport::Unimplemented => Some(0), - }; - if let Some(issue) = - tracking_issue.filter(|_| reported_diagnostics.insert(format!("unsupported:{keyword}"))) + if spec.support == support::KeywordSupport::Unimplemented + && reported_diagnostics.insert(format!("unsupported:{keyword}")) { let location = source_path.map_or_else( || format!("line {line_number}"), |path| format!("{}:{line_number}", path.display()), ); - if issue == 0 { - crate::diagnosticln!( - "Unsupported SSH config option '{keyword}' at {location}; bssh parses this value for inspection but does not implement its runtime behavior" - ); - } else { - crate::diagnosticln!( - "SSH config option '{keyword}' at {location} is not implemented yet; tracked in #{issue}" - ); - } + crate::diagnosticln!( + "Unsupported SSH config option '{keyword}' at {location}; bssh parses this value for inspection but does not implement its runtime behavior" + ); } match keyword { diff --git a/src/ssh/ssh_config/parser/options/support.rs b/src/ssh/ssh_config/parser/options/support.rs index 25236732..39eef257 100644 --- a/src/ssh/ssh_config/parser/options/support.rs +++ b/src/ssh/ssh_config/parser/options/support.rs @@ -4,10 +4,6 @@ pub(super) enum KeywordSupport { Runtime(RuntimeConsumer), Unimplemented, - // Keep the classification available for future split issue waves even - // when the current wave has no remaining delegated keywords. - #[allow(dead_code)] - Delegated(u32), } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -260,7 +256,11 @@ pub(super) fn keyword_spec(keyword: &str) -> Option { #[cfg(test)] mod tests { use super::*; - use std::collections::{HashMap, HashSet}; + use std::collections::HashSet; + + const ACCEPTED_SPELLING_COUNT: usize = 91; + const RUNTIME_SPELLING_COUNT: usize = 51; + const UNIMPLEMENTED_SPELLING_COUNT: usize = 40; #[test] fn accepted_keywords_and_aliases_have_one_consistent_classification() { @@ -279,6 +279,20 @@ mod tests { assert_eq!(canonical_spec.canonical, *canonical); assert_eq!(canonical_spec.support, *support); } + + let runtime_count = ACCEPTED_KEYWORDS + .iter() + .filter(|(_, _, support)| matches!(support, KeywordSupport::Runtime(_))) + .count(); + let unimplemented_count = ACCEPTED_KEYWORDS + .iter() + .filter(|(_, _, support)| matches!(support, KeywordSupport::Unimplemented)) + .count(); + + assert_eq!(ACCEPTED_KEYWORDS.len(), ACCEPTED_SPELLING_COUNT); + assert_eq!(runtime_count, RUNTIME_SPELLING_COUNT); + assert_eq!(unimplemented_count, UNIMPLEMENTED_SPELLING_COUNT); + assert_eq!(runtime_count + unimplemented_count, ACCEPTED_KEYWORDS.len()); } #[test] @@ -347,7 +361,7 @@ mod tests { } match support { KeywordSupport::Runtime(consumer) => Some((*keyword, *consumer)), - KeywordSupport::Delegated(_) | KeywordSupport::Unimplemented => None, + KeywordSupport::Unimplemented => None, } }) .collect::>(); @@ -356,18 +370,55 @@ mod tests { } #[test] - fn first_wave_delegations_match_the_split_issue_dag() { - let expected = HashMap::new(); - let delegated = ACCEPTED_KEYWORDS + fn unimplemented_keywords_are_exactly_the_audited_named_set() { + let expected = [ + "addkeystoagent", + "identityagent", + "kbdinteractiveauthentication", + "gssapiauthentication", + "hostbasedauthentication", + "hostbasedacceptedalgorithms", + "enablesshkeysign", + "usekeychain", + "casignaturealgorithms", + "nohostauthenticationforlocalhost", + "visualhostkey", + "requiredrsasize", + "fingerprinthash", + "forwardagent", + "forwardx11", + "gatewayports", + "permitremoteopen", + "forwardx11timeout", + "forwardx11trusted", + "connecttimeout", + "controlmaster", + "controlpath", + "controlpersist", + "escapechar", + "loglevel", + "syslogfacility", + "protocol", + "forkafterauthentication", + "stdinnull", + "cipher", + "fallbacktorsh", + "globalknownhostsfile2", + "rhostsauthentication", + "securitykeyprovider", + "userknownhostsfile2", + "useroaming", + "usersh", + "useprivilegedport", + ]; + let unimplemented = ACCEPTED_KEYWORDS .iter() .filter_map(|(keyword, canonical, support)| match support { - KeywordSupport::Delegated(issue) if keyword == canonical => { - Some((*keyword, *issue)) - } + KeywordSupport::Unimplemented if keyword == canonical => Some(*keyword), _ => None, }) - .collect::>(); + .collect::>(); - assert_eq!(delegated, expected); + assert_eq!(unimplemented, expected); } } diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index 2c29049f..b1f029c2 100644 --- a/tests/ssh_compat_output_test.rs +++ b/tests/ssh_compat_output_test.rs @@ -97,6 +97,14 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( log.to_str().expect("UTF-8 log path"), "-F", config.to_str().expect("UTF-8 config path"), + "-o", + "ChallengeResponseAuthentication=no", + "-o", + "KbdInteractiveAuthentication=no", + "-o", + "DefinitelyUnknownOption=no", + "-o", + "AnotherUnknownOption=yes", "--connect-timeout=1", "--strict-host-key-checking=no", "127.0.0.1:1", @@ -106,6 +114,10 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( .expect("bssh should parse the ssh config"); assert!(!output.status.success()); + assert!( + output.stdout.is_empty(), + "config diagnostics leaked to stdout" + ); assert!(output.stderr.is_empty(), "-E diagnostics leaked to stderr"); let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); @@ -117,6 +129,7 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( "Unsupported SSH config option 'securitykeyprovider' at {included}:4; bssh parses this value for inspection but does not implement its runtime behavior" ); let unknown = format!("Unknown SSH config option 'definitelyunknownoption' at {included}:5"); + let distinct_unknown = "Unknown SSH config option 'anotherunknownoption' at line 4"; assert_eq!( diagnostics.lines().filter(|line| line == &alias).count(), @@ -136,6 +149,14 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( 1, "unknown diagnostic must be emitted once: {diagnostics:?}" ); + assert_eq!( + diagnostics + .lines() + .filter(|line| line == &distinct_unknown) + .count(), + 1, + "distinct unknown diagnostic must remain independent: {diagnostics:?}" + ); assert!(!diagnostics.contains("/spoofed/config")); } From 3d103064a31f3c16b020214cda285c65cf5a7163 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 30 Aug 2026 18:59:19 +0900 Subject: [PATCH 2/3] fix(security): escape SSH config diagnostic fields SSH config filenames and unknown keywords are untrusted diagnostic fields. Rendering control characters verbatim allowed newline injection and ANSI terminal manipulation in stderr or -E logs. Escape every Unicode control character at the output boundary while preserving printable Unicode, and model config lines versus -o options explicitly so CLI diagnostics identify `-o option #N`. Add unit and end-to-end regressions covering control escaping, printable Unicode, malicious newline filenames, ANSI-bearing unknown keywords, single-line log integrity, and ordinary provenance. Refs #281 --- src/ssh/ssh_config/parser/core.rs | 34 ++++--- src/ssh/ssh_config/parser/diagnostic.rs | 110 +++++++++++++++++++++++ src/ssh/ssh_config/parser/mod.rs | 1 + src/ssh/ssh_config/parser/options/mod.rs | 20 ++--- tests/ssh_compat_output_test.rs | 50 ++++++++++- 5 files changed, 184 insertions(+), 31 deletions(-) create mode 100644 src/ssh/ssh_config/parser/diagnostic.rs diff --git a/src/ssh/ssh_config/parser/core.rs b/src/ssh/ssh_config/parser/core.rs index 101815cb..cc0d8880 100644 --- a/src/ssh/ssh_config/parser/core.rs +++ b/src/ssh/ssh_config/parser/core.rs @@ -25,6 +25,7 @@ use anyhow::{Context, Result}; use std::collections::HashSet; use std::path::Path; +use super::diagnostic::DiagnosticSource; use super::options; /// Parse SSH configuration content with Include and Match support @@ -109,8 +110,7 @@ pub(crate) fn parse_cli_options( &mut overlay, &keyword, &args, - None, - option_number, + DiagnosticSource::CliOption { option_number }, reported_diagnostics, ) .with_context(|| format!("Invalid -o option #{option_number} ({keyword})"))?; @@ -253,8 +253,10 @@ fn parse_lines<'a>( &mut match_block.config, &keyword, &args, - source_path, - line_number, + DiagnosticSource::Config { + path: source_path, + line_number, + }, reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; @@ -264,8 +266,10 @@ fn parse_lines<'a>( config, &keyword, &args, - source_path, - line_number, + DiagnosticSource::Config { + path: source_path, + line_number, + }, reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; @@ -283,8 +287,10 @@ fn parse_lines<'a>( config, &keyword, &args, - source_path, - line_number, + DiagnosticSource::Config { + path: source_path, + line_number, + }, reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; @@ -310,19 +316,11 @@ fn parse_option_first( target: &mut SshHostConfig, keyword: &str, args: &[String], - source_path: Option<&Path>, - line_number: usize, + source: DiagnosticSource<'_>, reported_diagnostics: &mut HashSet, ) -> Result<()> { let mut parsed = SshHostConfig::default(); - options::parse_option( - &mut parsed, - keyword, - args, - source_path, - line_number, - reported_diagnostics, - )?; + options::parse_option(&mut parsed, keyword, args, source, reported_diagnostics)?; merge_host_config(target, &parsed); Ok(()) } diff --git a/src/ssh/ssh_config/parser/diagnostic.rs b/src/ssh/ssh_config/parser/diagnostic.rs new file mode 100644 index 00000000..8ff4a6e1 --- /dev/null +++ b/src/ssh/ssh_config/parser/diagnostic.rs @@ -0,0 +1,110 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::{borrow::Cow, path::Path}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum DiagnosticSource<'a> { + Config { + path: Option<&'a Path>, + line_number: usize, + }, + CliOption { + option_number: usize, + }, +} + +impl DiagnosticSource<'_> { + pub(super) fn number(self) -> usize { + match self { + Self::Config { line_number, .. } => line_number, + Self::CliOption { option_number } => option_number, + } + } + + pub(super) fn location(self) -> String { + match self { + Self::Config { + path: Some(path), + line_number, + } => format!("{}:{line_number}", escape_diagnostic_path(path)), + Self::Config { + path: None, + line_number, + } => format!("line {line_number}"), + Self::CliOption { option_number } => format!("-o option #{option_number}"), + } + } +} + +pub(super) fn escape_diagnostic_field(value: &str) -> Cow<'_, str> { + if !value.chars().any(char::is_control) { + return Cow::Borrowed(value); + } + + let mut escaped = String::with_capacity(value.len()); + for character in value.chars() { + if character.is_control() { + escaped.extend(character.escape_default()); + } else { + escaped.push(character); + } + } + Cow::Owned(escaped) +} + +fn escape_diagnostic_path(path: &Path) -> String { + escape_diagnostic_field(&path.to_string_lossy()).into_owned() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn diagnostic_fields_escape_controls_but_preserve_printable_unicode() { + assert_eq!( + escape_diagnostic_field("경로/é/λ\r\n\t\u{1b}\u{7f}\u{85}"), + "경로/é/λ\\r\\n\\t\\u{1b}\\u{7f}\\u{85}" + ); + assert!(matches!( + escape_diagnostic_field("경로/é/λ"), + Cow::Borrowed(_) + )); + } + + #[test] + fn diagnostic_locations_distinguish_files_lines_and_cli_options() { + assert_eq!( + DiagnosticSource::Config { + path: Some(Path::new("safe\nFORGED")), + line_number: 7, + } + .location(), + "safe\\nFORGED:7" + ); + assert_eq!( + DiagnosticSource::Config { + path: None, + line_number: 3, + } + .location(), + "line 3" + ); + assert_eq!( + DiagnosticSource::CliOption { option_number: 4 }.location(), + "-o option #4" + ); + } +} diff --git a/src/ssh/ssh_config/parser/mod.rs b/src/ssh/ssh_config/parser/mod.rs index 35bc4b1b..8a5937ab 100644 --- a/src/ssh/ssh_config/parser/mod.rs +++ b/src/ssh/ssh_config/parser/mod.rs @@ -21,6 +21,7 @@ //! - `tests`: Comprehensive test suite mod core; +mod diagnostic; mod helpers; mod options; diff --git a/src/ssh/ssh_config/parser/options/mod.rs b/src/ssh/ssh_config/parser/options/mod.rs index f882068c..092178f5 100644 --- a/src/ssh/ssh_config/parser/options/mod.rs +++ b/src/ssh/ssh_config/parser/options/mod.rs @@ -29,9 +29,10 @@ mod security; mod support; mod ui; +use super::diagnostic::{DiagnosticSource, escape_diagnostic_field}; use crate::ssh::ssh_config::types::SshHostConfig; use anyhow::Result; -use std::{collections::HashSet, path::Path}; +use std::collections::HashSet; /// Parse a configuration option for a host /// @@ -41,17 +42,15 @@ pub fn parse_option( host: &mut SshHostConfig, accepted_keyword: &str, args: &[String], - source_path: Option<&Path>, - line_number: usize, + source: DiagnosticSource<'_>, reported_diagnostics: &mut HashSet, ) -> Result<()> { + let line_number = source.number(); let Some(spec) = support::keyword_spec(accepted_keyword) else { if reported_diagnostics.insert(format!("unknown:{accepted_keyword}")) { - let location = source_path.map_or_else( - || format!("line {line_number}"), - |path| format!("{}:{line_number}", path.display()), - ); - crate::diagnosticln!("Unknown SSH config option '{accepted_keyword}' at {location}"); + let keyword = escape_diagnostic_field(accepted_keyword); + let location = source.location(); + crate::diagnosticln!("Unknown SSH config option '{keyword}' at {location}"); } return Ok(()); }; @@ -60,10 +59,7 @@ pub fn parse_option( if spec.support == support::KeywordSupport::Unimplemented && reported_diagnostics.insert(format!("unsupported:{keyword}")) { - let location = source_path.map_or_else( - || format!("line {line_number}"), - |path| format!("{}:{line_number}", path.display()), - ); + let location = source.location(); crate::diagnosticln!( "Unsupported SSH config option '{keyword}' at {location}; bssh parses this value for inspection but does not implement its runtime behavior" ); diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index b1f029c2..5e3ae191 100644 --- a/tests/ssh_compat_output_test.rs +++ b/tests/ssh_compat_output_test.rs @@ -129,7 +129,7 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( "Unsupported SSH config option 'securitykeyprovider' at {included}:4; bssh parses this value for inspection but does not implement its runtime behavior" ); let unknown = format!("Unknown SSH config option 'definitelyunknownoption' at {included}:5"); - let distinct_unknown = "Unknown SSH config option 'anotherunknownoption' at line 4"; + let distinct_unknown = "Unknown SSH config option 'anotherunknownoption' at -o option #4"; assert_eq!( diagnostics.lines().filter(|line| line == &alias).count(), @@ -160,6 +160,54 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( assert!(!diagnostics.contains("/spoofed/config")); } +#[cfg(unix)] +#[test] +fn config_diagnostics_escape_control_characters_in_paths_and_keywords() { + let directory = tempdir().expect("temporary directory should be created"); + let config = directory.path().join("ssh_config\nFORGED PATH"); + let log = directory.path().join("bssh.log"); + fs::write(&config, "Host *\nBad\u{1b}[31mKeyword yes\n") + .expect("maliciously named SSH config should be written"); + + let output = bssh() + .arg("-E") + .arg(&log) + .arg("-F") + .arg(&config) + .args([ + "--connect-timeout=1", + "--strict-host-key-checking=no", + "127.0.0.1:1", + "true", + ]) + .output() + .expect("bssh should parse the maliciously named SSH config"); + + assert!(!output.status.success()); + assert!(output.stdout.is_empty()); + assert!(output.stderr.is_empty()); + + let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); + let escaped_path = config.to_string_lossy().replace('\n', "\\n"); + let expected = + format!("Unknown SSH config option 'bad\\u{{1b}}[31mkeyword' at {escaped_path}:2"); + assert_eq!( + diagnostics + .lines() + .filter(|line| line.starts_with("Unknown SSH config option")) + .collect::>(), + [expected], + "untrusted diagnostic fields must remain on one escaped line: {diagnostics:?}" + ); + assert!(!diagnostics.contains('\u{1b}')); + assert!( + !diagnostics + .lines() + .any(|line| line.starts_with("FORGED PATH")), + "config path forged a separate diagnostic line: {diagnostics:?}" + ); +} + #[test] fn connection_refused_is_actionable_and_exits_255() { let directory = tempdir().expect("temporary directory should be created"); From a67548a5c319ab2f378e606b4f7efb097ccc9df3 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 30 Aug 2026 19:15:44 +0900 Subject: [PATCH 3/3] fix(security): harden SSH config path diagnostics SSH config path failures were still rendered through raw Path::display calls across cache loading, include resolution, and path validation. A newline-bearing missing -F path could inject forged -E log lines despite parser warning escaping. Move control escaping to a crate-visible SSH config diagnostic helper and reuse it at every user-visible path and pattern context in the SSH config and cache modules. The helper returns Cow so ordinary printable UTF-8 remains unchanged and allocation-free. Add an end-to-end missing-path regression alongside the existing warning-path coverage, and audit remaining raw display calls as test-only fixture construction. Refs #281 --- src/ssh/config_cache/manager.rs | 26 ++++---- src/ssh/ssh_config/diagnostic.rs | 61 +++++++++++++++++++ src/ssh/ssh_config/include/mod.rs | 24 +++++--- src/ssh/ssh_config/include/resolver.rs | 31 +++++++--- src/ssh/ssh_config/include/validation.rs | 28 ++++++--- src/ssh/ssh_config/mod.rs | 17 ++++-- src/ssh/ssh_config/parser/core.rs | 3 +- src/ssh/ssh_config/parser/diagnostic.rs | 37 +---------- src/ssh/ssh_config/parser/options/mod.rs | 5 +- src/ssh/ssh_config/path.rs | 5 +- src/ssh/ssh_config/security/checks.rs | 28 +++++---- .../ssh_config/security/path_validation.rs | 17 ++++-- tests/ssh_compat_output_test.rs | 38 ++++++++++++ 13 files changed, 222 insertions(+), 98 deletions(-) create mode 100644 src/ssh/ssh_config/diagnostic.rs diff --git a/src/ssh/config_cache/manager.rs b/src/ssh/config_cache/manager.rs index 9b3f5ff8..20b490b9 100644 --- a/src/ssh/config_cache/manager.rs +++ b/src/ssh/config_cache/manager.rs @@ -16,6 +16,7 @@ use super::config::CacheConfig; use super::entry::CacheEntry; use super::stats::CacheStats; use crate::ssh::SshConfig; +use crate::ssh::ssh_config::diagnostic::escape_path; use anyhow::{Context, Result}; use lru::LruCache; use std::path::{Path, PathBuf}; @@ -67,16 +68,16 @@ impl SshConfigCache { let path_ref = path.as_ref(); let path = tokio::fs::canonicalize(path_ref) .await - .with_context(|| format!("Failed to canonicalize path: {}", path_ref.display()))?; + .with_context(|| format!("Failed to canonicalize path: {}", escape_path(path_ref)))?; // Check if file exists and get its modification time let file_metadata = tokio::fs::metadata(&path) .await - .with_context(|| format!("Failed to read file metadata: {}", path.display()))?; + .with_context(|| format!("Failed to read file metadata: {}", escape_path(&path)))?; let current_mtime = file_metadata .modified() - .with_context(|| format!("Failed to get modification time: {}", path.display()))?; + .with_context(|| format!("Failed to get modification time: {}", escape_path(&path)))?; // Try to get from cache first if let Some(config) = self.try_get_cached(&path, current_mtime)? { @@ -84,10 +85,13 @@ impl SshConfigCache { } // Cache miss - load from file - trace!("Cache miss for SSH config: {}", path.display()); - let config = SshConfig::load_from_file(&path) - .await - .with_context(|| format!("Failed to load SSH config from file: {}", path.display()))?; + trace!("Cache miss for SSH config: {}", escape_path(&path)); + let config = SshConfig::load_from_file(&path).await.with_context(|| { + format!( + "Failed to load SSH config from file: {}", + escape_path(&path) + ) + })?; // Store in cache if let Err(e) = self.put(path, config.clone(), current_mtime) { @@ -120,7 +124,7 @@ impl SshConfigCache { if let Some(entry) = cache.get_mut(path) { // Check if entry is expired if entry.is_expired(self.config.ttl) { - debug!("SSH config cache entry expired: {}", path.display()); + debug!("SSH config cache entry expired: {}", escape_path(path)); cache.pop(path); let mut stats = self.stats.write().map_err(|e| { @@ -132,7 +136,7 @@ impl SshConfigCache { // Check if entry is stale (file was modified) if entry.is_stale(current_mtime) { - debug!("SSH config cache entry stale: {}", path.display()); + debug!("SSH config cache entry stale: {}", escape_path(path)); cache.pop(path); let mut stats = self.stats.write().map_err(|e| { @@ -153,7 +157,7 @@ impl SshConfigCache { stats.hits += 1; } - trace!("SSH config cache hit: {}", path.display()); + trace!("SSH config cache hit: {}", escape_path(path)); return Ok(Some(config)); } @@ -185,7 +189,7 @@ impl SshConfigCache { stats.current_entries = cache.len(); } - trace!("SSH config cached: {}", path.display()); + trace!("SSH config cached: {}", escape_path(&path)); Ok(()) } diff --git a/src/ssh/ssh_config/diagnostic.rs b/src/ssh/ssh_config/diagnostic.rs new file mode 100644 index 00000000..4da0aab8 --- /dev/null +++ b/src/ssh/ssh_config/diagnostic.rs @@ -0,0 +1,61 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::{borrow::Cow, path::Path}; + +pub(crate) fn escape_field(value: &str) -> Cow<'_, str> { + if !value.chars().any(char::is_control) { + return Cow::Borrowed(value); + } + + let mut escaped = String::with_capacity(value.len()); + for character in value.chars() { + if character.is_control() { + escaped.extend(character.escape_default()); + } else { + escaped.push(character); + } + } + Cow::Owned(escaped) +} + +pub(crate) fn escape_path(path: &Path) -> Cow<'_, str> { + match path.to_string_lossy() { + Cow::Borrowed(value) => escape_field(value), + Cow::Owned(value) => Cow::Owned(escape_field(&value).into_owned()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fields_escape_controls_but_preserve_printable_unicode() { + assert_eq!( + escape_field("경로/é/λ\r\n\t\u{1b}\u{7f}\u{85}"), + "경로/é/λ\\r\\n\\t\\u{1b}\\u{7f}\\u{85}" + ); + assert!(matches!(escape_field("경로/é/λ"), Cow::Borrowed(_))); + } + + #[test] + fn paths_use_the_same_control_escaping_contract() { + assert_eq!(escape_path(Path::new("safe\nFORGED")), "safe\\nFORGED"); + assert!(matches!( + escape_path(Path::new("printable/경로")), + Cow::Borrowed(_) + )); + } +} diff --git a/src/ssh/ssh_config/include/mod.rs b/src/ssh/ssh_config/include/mod.rs index 8755b946..e62b9610 100644 --- a/src/ssh/ssh_config/include/mod.rs +++ b/src/ssh/ssh_config/include/mod.rs @@ -21,6 +21,8 @@ use anyhow::{Context, Result}; use std::collections::HashSet; use std::path::{Path, PathBuf}; +use super::diagnostic::{escape_field, escape_path}; + mod resolver; mod validation; @@ -95,7 +97,7 @@ impl IncludeContext { // Canonicalize and cache the result let canonical = path .canonicalize() - .with_context(|| format!("Failed to canonicalize path: {}", path.display()))?; + .with_context(|| format!("Failed to canonicalize path: {}", escape_path(path)))?; self.canonical_cache .insert(path.to_path_buf(), canonical.clone()); canonical @@ -115,7 +117,7 @@ impl IncludeContext { if self.visited.contains(&canonical_str) { anyhow::bail!( "Include cycle detected: {} has already been processed", - path.display() + escape_path(path) ); } @@ -166,7 +168,7 @@ pub async fn resolve_includes(config_path: &Path, content: &str) -> Result= MAX_GLOB_RESULTS { anyhow::bail!( - "Glob pattern '{pattern}' matched too many files (>{MAX_GLOB_RESULTS}). \ - Please use a more specific pattern." + "Glob pattern '{}' matched too many files (>{MAX_GLOB_RESULTS}). Please use a more specific pattern.", + escape_field(pattern) ); } @@ -118,7 +119,11 @@ pub async fn resolve_include_pattern( Ok(c) => c, Err(_) if !path.exists() => continue, // Skip non-existent files Err(e) => { - tracing::debug!("Failed to canonicalize {}: {}", path.display(), e); + tracing::debug!( + "Failed to canonicalize {}: {}", + escape_path(&path), + escape_field(&e.to_string()) + ); continue; } }; @@ -127,7 +132,7 @@ pub async fn resolve_include_pattern( if !is_path_allowed(&canonical) { tracing::warn!( "Glob result {} escapes allowed directories, skipping", - path.display() + escape_path(&path) ); continue; } @@ -144,13 +149,21 @@ pub async fn resolve_include_pattern( } } Err(e) => { - tracing::debug!("Failed to get metadata for {}: {}", path.display(), e); + tracing::debug!( + "Failed to get metadata for {}: {}", + escape_path(&path), + escape_field(&e.to_string()) + ); } } } Err(e) => { // Log glob errors but continue - tracing::warn!("Error processing glob pattern '{}': {}", pattern_str, e); + tracing::warn!( + "Error processing glob pattern '{}': {}", + escape_field(pattern_str), + escape_field(&e.to_string()) + ); } } } @@ -162,7 +175,7 @@ pub async fn resolve_include_pattern( if files.is_empty() && !pattern.contains('*') && !pattern.contains('?') { tracing::debug!( "Include pattern '{}' matched no files (this may be intentional)", - pattern + escape_field(pattern) ); } diff --git a/src/ssh/ssh_config/include/validation.rs b/src/ssh/ssh_config/include/validation.rs index ccd9f0f1..acb1837f 100644 --- a/src/ssh/ssh_config/include/validation.rs +++ b/src/ssh/ssh_config/include/validation.rs @@ -17,6 +17,8 @@ use anyhow::{Context, Result}; use std::path::{Path, PathBuf}; +use super::super::diagnostic::{escape_field, escape_path}; + /// Validate a glob pattern for security pub fn validate_glob_pattern(pattern: &str) -> Result<()> { // Check for dangerous glob patterns @@ -27,13 +29,19 @@ pub fn validate_glob_pattern(pattern: &str) -> Result<()> { // Check for excessive wildcards that could cause exponential expansion let wildcard_count = pattern.chars().filter(|&c| c == '*').count(); if wildcard_count > 5 { - anyhow::bail!("Too many wildcards in pattern '{pattern}'. Maximum 5 wildcards allowed."); + anyhow::bail!( + "Too many wildcards in pattern '{}'. Maximum 5 wildcards allowed.", + escape_field(pattern) + ); } // Check for overly broad patterns that could match system files // But allow common SSH config patterns like ~/.ssh/config.d/* if (pattern == "*" || pattern == "/*") && !pattern.contains("ssh") { - anyhow::bail!("Pattern '{pattern}' is too broad and could match system files"); + anyhow::bail!( + "Pattern '{}' is too broad and could match system files", + escape_field(pattern) + ); } // Check pattern length @@ -69,32 +77,32 @@ pub fn validate_include_path(path: &Path) -> Result<()> { // Get metadata without following symlinks let metadata = std::fs::symlink_metadata(path) - .with_context(|| format!("Failed to get metadata for {}", path.display()))?; + .with_context(|| format!("Failed to get metadata for {}", escape_path(path)))?; // Reject symbolic links for security if metadata.is_symlink() { anyhow::bail!( "Include path {} is a symbolic link. Symlinks are not allowed for security reasons.", - path.display() + escape_path(path) ); } // Check if it's a regular file if !metadata.is_file() { - anyhow::bail!("Include path is not a regular file: {}", path.display()); + anyhow::bail!("Include path is not a regular file: {}", escape_path(path)); } // Canonicalize and verify the path doesn't escape expected directories let canonical = path .canonicalize() - .with_context(|| format!("Failed to canonicalize {}", path.display()))?; + .with_context(|| format!("Failed to canonicalize {}", escape_path(path)))?; // Check for directory traversal attempts let path_str = canonical.to_string_lossy(); if path_str.contains("../") || path_str.contains("..\\") { anyhow::bail!( "Include path {} contains directory traversal sequences", - path.display() + escape_path(path) ); } @@ -113,7 +121,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if !is_safe { tracing::warn!( "Include path {} is outside of standard SSH config directories. This may be a security risk.", - canonical.display() + escape_path(&canonical) ); } @@ -130,7 +138,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if mode & 0o002 != 0 { anyhow::bail!( "SSH config file {} is world-writable. This is a security vulnerability.", - path.display() + escape_path(path) ); } @@ -138,7 +146,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if mode & 0o020 != 0 { tracing::warn!( "SSH config file {} is group-writable. This is a potential security risk.", - path.display() + escape_path(path) ); } } diff --git a/src/ssh/ssh_config/mod.rs b/src/ssh/ssh_config/mod.rs index 7e8eb250..1132d8d9 100644 --- a/src/ssh/ssh_config/mod.rs +++ b/src/ssh/ssh_config/mod.rs @@ -24,6 +24,7 @@ use std::{ }; // Internal modules +pub(crate) mod diagnostic; mod env_cache; mod include; #[cfg(test)] @@ -65,13 +66,21 @@ impl SshConfig { /// Load SSH configuration from a file with Include support pub async fn load_from_file>(path: P) -> Result { let path = path.as_ref(); - let content = tokio::fs::read_to_string(path) - .await - .with_context(|| format!("Failed to read SSH config file: {}", path.display()))?; + let content = tokio::fs::read_to_string(path).await.with_context(|| { + format!( + "Failed to read SSH config file: {}", + diagnostic::escape_path(path) + ) + })?; Self::parse_from_file_with_content(path, &content) .await - .with_context(|| format!("Failed to parse SSH config file: {}", path.display())) + .with_context(|| { + format!( + "Failed to parse SSH config file: {}", + diagnostic::escape_path(path) + ) + }) } /// Load SSH configuration from a file with caching diff --git a/src/ssh/ssh_config/parser/core.rs b/src/ssh/ssh_config/parser/core.rs index cc0d8880..6c385701 100644 --- a/src/ssh/ssh_config/parser/core.rs +++ b/src/ssh/ssh_config/parser/core.rs @@ -27,6 +27,7 @@ use std::path::Path; use super::diagnostic::DiagnosticSource; use super::options; +use crate::ssh::ssh_config::diagnostic::escape_path; /// Parse SSH configuration content with Include and Match support #[cfg(test)] @@ -53,7 +54,7 @@ pub(crate) async fn parse_from_file_with_diagnostics( // Pass 1: Resolve all Include directives let included_files = resolve_includes(path, content) .await - .with_context(|| format!("Failed to resolve includes for {}", path.display()))?; + .with_context(|| format!("Failed to resolve includes for {}", escape_path(path)))?; parse_included_files(&included_files, reported_diagnostics) } diff --git a/src/ssh/ssh_config/parser/diagnostic.rs b/src/ssh/ssh_config/parser/diagnostic.rs index 8ff4a6e1..411d4974 100644 --- a/src/ssh/ssh_config/parser/diagnostic.rs +++ b/src/ssh/ssh_config/parser/diagnostic.rs @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{borrow::Cow, path::Path}; +use crate::ssh::ssh_config::diagnostic::escape_path; +use std::path::Path; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum DiagnosticSource<'a> { @@ -38,7 +39,7 @@ impl DiagnosticSource<'_> { Self::Config { path: Some(path), line_number, - } => format!("{}:{line_number}", escape_diagnostic_path(path)), + } => format!("{}:{line_number}", escape_path(path)), Self::Config { path: None, line_number, @@ -48,42 +49,10 @@ impl DiagnosticSource<'_> { } } -pub(super) fn escape_diagnostic_field(value: &str) -> Cow<'_, str> { - if !value.chars().any(char::is_control) { - return Cow::Borrowed(value); - } - - let mut escaped = String::with_capacity(value.len()); - for character in value.chars() { - if character.is_control() { - escaped.extend(character.escape_default()); - } else { - escaped.push(character); - } - } - Cow::Owned(escaped) -} - -fn escape_diagnostic_path(path: &Path) -> String { - escape_diagnostic_field(&path.to_string_lossy()).into_owned() -} - #[cfg(test)] mod tests { use super::*; - #[test] - fn diagnostic_fields_escape_controls_but_preserve_printable_unicode() { - assert_eq!( - escape_diagnostic_field("경로/é/λ\r\n\t\u{1b}\u{7f}\u{85}"), - "경로/é/λ\\r\\n\\t\\u{1b}\\u{7f}\\u{85}" - ); - assert!(matches!( - escape_diagnostic_field("경로/é/λ"), - Cow::Borrowed(_) - )); - } - #[test] fn diagnostic_locations_distinguish_files_lines_and_cli_options() { assert_eq!( diff --git a/src/ssh/ssh_config/parser/options/mod.rs b/src/ssh/ssh_config/parser/options/mod.rs index 092178f5..05b6a09f 100644 --- a/src/ssh/ssh_config/parser/options/mod.rs +++ b/src/ssh/ssh_config/parser/options/mod.rs @@ -29,7 +29,8 @@ mod security; mod support; mod ui; -use super::diagnostic::{DiagnosticSource, escape_diagnostic_field}; +use super::diagnostic::DiagnosticSource; +use crate::ssh::ssh_config::diagnostic::escape_field; use crate::ssh::ssh_config::types::SshHostConfig; use anyhow::Result; use std::collections::HashSet; @@ -48,7 +49,7 @@ pub fn parse_option( let line_number = source.number(); let Some(spec) = support::keyword_spec(accepted_keyword) else { if reported_diagnostics.insert(format!("unknown:{accepted_keyword}")) { - let keyword = escape_diagnostic_field(accepted_keyword); + let keyword = escape_field(accepted_keyword); let location = source.location(); crate::diagnosticln!("Unknown SSH config option '{keyword}' at {location}"); } diff --git a/src/ssh/ssh_config/path.rs b/src/ssh/ssh_config/path.rs index bc1ff31d..04f3ea07 100644 --- a/src/ssh/ssh_config/path.rs +++ b/src/ssh/ssh_config/path.rs @@ -20,6 +20,7 @@ use anyhow::Result; use std::path::PathBuf; +use super::diagnostic::{escape_field, escape_path}; use super::env_cache::GLOBAL_ENV_CACHE; /// Expand tilde and environment variables in a path (secure implementation) @@ -59,8 +60,8 @@ pub(super) fn expand_path_internal(path: &str) -> Result { } else { tracing::warn!( "Environment variable expansion failed for '{}': {}. Using original path.", - path_str, - e + escape_path(&path), + escape_field(&e.to_string()) ); Ok(path) } diff --git a/src/ssh/ssh_config/security/checks.rs b/src/ssh/ssh_config/security/checks.rs index 98c6b040..e4a79214 100644 --- a/src/ssh/ssh_config/security/checks.rs +++ b/src/ssh/ssh_config/security/checks.rs @@ -19,10 +19,13 @@ use anyhow::Result; use std::os::unix::fs::PermissionsExt; use std::path::Path; +use super::super::diagnostic::escape_path; + /// Validate security properties of identity files pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Result<()> { // Check for sensitive system paths let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files let sensitive_patterns = [ @@ -44,7 +47,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Identity file path '{path_str}' at line {line_number} points to sensitive system location. \ + "Security violation: Identity file path '{escaped_path}' at line {line_number} points to sensitive system location. \ Access to system files is not allowed for security reasons." ); } @@ -64,7 +67,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul tracing::warn!( "Security warning: Identity file '{}' at line {} is world-readable. \ Private SSH keys should not be readable by other users (chmod 600 recommended).", - path_str, + escaped_path, line_number ); } @@ -74,7 +77,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul tracing::warn!( "Security warning: Identity file '{}' at line {} is group-readable. \ Private SSH keys should only be readable by the owner (chmod 600 recommended).", - path_str, + escaped_path, line_number ); } @@ -82,7 +85,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul // Check if file is world-writable (very dangerous) if mode & 0o002 != 0 { anyhow::bail!( - "Security violation: Identity file '{path_str}' at line {line_number} is world-writable. \ + "Security violation: Identity file '{escaped_path}' at line {line_number} is world-writable. \ This is extremely dangerous and must be fixed immediately." ); } @@ -94,6 +97,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul /// Validate security properties of known_hosts files pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files let sensitive_patterns = [ @@ -115,7 +119,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Known hosts file path '{path_str}' at line {line_number} points to sensitive system location. \ + "Security violation: Known hosts file path '{escaped_path}' at line {line_number} points to sensitive system location. \ Access to system files is not allowed for security reasons." ); } @@ -134,7 +138,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Known hosts file '{}' at line {} is in an unusual location. \ Ensure this is intentional and the file is trustworthy.", - path_str, + escaped_path, line_number ); } @@ -145,6 +149,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re /// Validate security properties of certificate files pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files that should never be certificates let forbidden_patterns = [ @@ -191,7 +196,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re // Check if it's exactly the private key name without certificate suffix if path_str.ends_with(pattern) || path_str.ends_with(&format!("{pattern}.pub")) { anyhow::bail!( - "Security violation: Certificate file path '{path_str}' at line {line_number} appears to be a private key or regular public key. \ + "Security violation: Certificate file path '{escaped_path}' at line {line_number} appears to be a private key or regular public key. \ SSH certificate files should end with '-cert.pub' or similar suffix. Use CertificateFile for certificates, not regular keys." ); } @@ -199,7 +204,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re } anyhow::bail!( - "Security violation: Certificate file path '{path_str}' at line {line_number} points to forbidden system location. \ + "Security violation: Certificate file path '{escaped_path}' at line {line_number} points to forbidden system location. \ System files and sensitive locations cannot be used as SSH certificates." ); } @@ -214,7 +219,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Certificate file '{}' at line {} has an unusual extension. \ SSH certificates typically end with '.pub', '-cert.pub', '.pem', or '.crt'.", - path_str, + escaped_path, line_number ); } @@ -234,7 +239,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Certificate file '{}' at line {} is in an unusual location. \ Ensure this is intentional and the file is a valid SSH certificate.", - path_str, + escaped_path, line_number ); } @@ -245,6 +250,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re /// Validate security properties of general files pub fn validate_general_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to the most critical system files let forbidden_patterns = [ @@ -267,7 +273,7 @@ pub fn validate_general_file_security(path: &Path, line_number: usize) -> Result for pattern in &forbidden_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: File path '{path_str}' at line {line_number} points to forbidden system location. \ + "Security violation: File path '{escaped_path}' at line {line_number} points to forbidden system location. \ Access to this location is not allowed for security reasons." ); } diff --git a/src/ssh/ssh_config/security/path_validation.rs b/src/ssh/ssh_config/security/path_validation.rs index 946377c8..73b8b72b 100644 --- a/src/ssh/ssh_config/security/path_validation.rs +++ b/src/ssh/ssh_config/security/path_validation.rs @@ -18,6 +18,7 @@ use anyhow::{Context, Result}; use std::path::PathBuf; use super::checks; +use crate::ssh::ssh_config::diagnostic::{escape_field, escape_path}; use crate::ssh::ssh_config::path::expand_path_internal; /// Securely validate and expand a file path to prevent path traversal attacks @@ -40,8 +41,12 @@ use crate::ssh::ssh_config::path::expand_path_internal; /// * `Err(anyhow::Error)` if the path is unsafe or invalid pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> Result { // First expand the path using the existing logic - let expanded_path = expand_path_internal(path) - .with_context(|| format!("Failed to expand path '{path}' at line {line_number}"))?; + let expanded_path = expand_path_internal(path).with_context(|| { + format!( + "Failed to expand path '{}' at line {line_number}", + escape_field(path) + ) + })?; // Convert to string for analysis let path_str = expanded_path.to_string_lossy(); @@ -70,9 +75,9 @@ pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> tracing::debug!( "Could not canonicalize {} path '{}' at line {}: {}. Using expanded path as-is.", path_type, - path_str, + escape_path(&expanded_path), line_number, - e + escape_field(&e.to_string()) ); expanded_path.clone() } @@ -91,8 +96,8 @@ pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> || canonical_str.split('\\').any(|component| component == "..") { anyhow::bail!( - "Security violation: Canonicalized {path_type} path '{canonical_str}' contains parent directory references at line {line_number}. \ - This could indicate a path traversal attempt." + "Security violation: Canonicalized {path_type} path '{}' contains parent directory references at line {line_number}. This could indicate a path traversal attempt.", + escape_path(&canonical_path) ); } } diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index 5e3ae191..069fce9c 100644 --- a/tests/ssh_compat_output_test.rs +++ b/tests/ssh_compat_output_test.rs @@ -208,6 +208,44 @@ fn config_diagnostics_escape_control_characters_in_paths_and_keywords() { ); } +#[cfg(unix)] +#[test] +fn config_load_errors_escape_control_characters_in_nonexistent_paths() { + let directory = tempdir().expect("temporary directory should be created"); + let missing = directory.path().join("missing\nFORGED-ERROR\u{1b}[31m"); + let log = directory.path().join("bssh.log"); + + let output = bssh() + .arg("-E") + .arg(&log) + .arg("-F") + .arg(&missing) + .args(["127.0.0.1", "true"]) + .output() + .expect("bssh should report the missing malicious SSH config path"); + + assert_eq!(output.status.code(), Some(1)); + assert!(output.stdout.is_empty()); + assert!(output.stderr.is_empty()); + + let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); + let escaped_path = missing + .to_string_lossy() + .replace('\n', "\\n") + .replace('\u{1b}', "\\u{1b}"); + assert!( + diagnostics.contains(&format!("Failed to canonicalize path: {escaped_path}")), + "escaped missing path is absent from diagnostic chain: {diagnostics:?}" + ); + assert!(!diagnostics.contains('\u{1b}')); + assert!( + !diagnostics + .lines() + .any(|line| line.starts_with("FORGED-ERROR")), + "missing config path forged a separate error line: {diagnostics:?}" + ); +} + #[test] fn connection_refused_is_actionable_and_exits_255() { let directory = tempdir().expect("temporary directory should be created");