diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index 8bca9beda..975f5f6fd 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -383,12 +383,7 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> { }; if dst.is_symlink() || dst.exists() { - backup_path = match settings.backup { - BackupMode::None => None, - BackupMode::Simple => Some(simple_backup_path(dst, &settings.suffix)), - BackupMode::Numbered => Some(numbered_backup_path(dst)), - BackupMode::Existing => Some(existing_backup_path(dst, &settings.suffix)), - }; + backup_path = backup_control::get_backup_path(settings.backup, dst, &settings.suffix); if settings.backup == BackupMode::Existing && !settings.symbolic { // when ln --backup f f, it should detect that it is the same file if paths_refer_to_same_file(src, dst, true) { @@ -463,31 +458,6 @@ fn link(src: &Path, dst: &Path, settings: &Settings) -> UResult<()> { Ok(()) } -fn simple_backup_path(path: &Path, suffix: &OsString) -> PathBuf { - let mut file_name = path.file_name().unwrap_or_default().to_os_string(); - file_name.push(suffix); - path.with_file_name(file_name) -} - -fn numbered_backup_path(path: &Path) -> PathBuf { - let mut i: u64 = 1; - loop { - let new_path = simple_backup_path(path, &OsString::from(format!(".~{i}~"))); - if !new_path.exists() { - return new_path; - } - i += 1; - } -} - -fn existing_backup_path(path: &Path, suffix: &OsString) -> PathBuf { - let test_path = simple_backup_path(path, &OsString::from(".~1~")); - if test_path.exists() { - return numbered_backup_path(path); - } - simple_backup_path(path, suffix) -} - #[cfg(windows)] pub fn symlink, P2: AsRef>(src: P1, dst: P2) -> std::io::Result<()> { if src.as_ref().is_dir() { diff --git a/src/uucore/src/lib/features/backup_control.rs b/src/uucore/src/lib/features/backup_control.rs index 9da6d09e3..f56d131c7 100644 --- a/src/uucore/src/lib/features/backup_control.rs +++ b/src/uucore/src/lib/features/backup_control.rs @@ -89,6 +89,7 @@ use clap::ArgMatches; use std::{ env, error::Error, + ffi::{OsStr, OsString}, fmt::{Debug, Display}, path::{Path, PathBuf}, }; @@ -243,16 +244,21 @@ pub mod arguments { /// /// 1. From the '-S' or '--suffix' CLI argument, if present /// 2. From the "SIMPLE_BACKUP_SUFFIX" environment variable, if present -/// 3. By using the default '~' if none of the others apply +/// 3. By using the default '~' if none of the others apply, or if they contained slashes /// /// This function directly takes [`ArgMatches`] as argument and looks for /// the '-S' and '--suffix' arguments itself. pub fn determine_backup_suffix(matches: &ArgMatches) -> String { let supplied_suffix = matches.get_one::(arguments::OPT_SUFFIX); - if let Some(suffix) = supplied_suffix { + let suffix = if let Some(suffix) = supplied_suffix { String::from(suffix) } else { env::var("SIMPLE_BACKUP_SUFFIX").unwrap_or_else(|_| DEFAULT_BACKUP_SUFFIX.to_owned()) + }; + if suffix.contains('/') { + DEFAULT_BACKUP_SUFFIX.to_owned() + } else { + suffix } } @@ -413,48 +419,42 @@ fn match_method(method: &str, origin: &str) -> UResult { } } -pub fn get_backup_path( +pub fn get_backup_path>( backup_mode: BackupMode, backup_path: &Path, - suffix: &str, + suffix: S, ) -> Option { match backup_mode { BackupMode::None => None, - BackupMode::Simple => Some(simple_backup_path(backup_path, suffix)), + BackupMode::Simple => Some(simple_backup_path(backup_path, suffix.as_ref())), BackupMode::Numbered => Some(numbered_backup_path(backup_path)), - BackupMode::Existing => Some(existing_backup_path(backup_path, suffix)), + BackupMode::Existing => Some(existing_backup_path(backup_path, suffix.as_ref())), } } -fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf { +fn simple_backup_path>(path: &Path, suffix: S) -> PathBuf { let mut file_name = path.file_name().unwrap_or_default().to_os_string(); - file_name.push(suffix); + file_name.push(suffix.as_ref()); path.with_file_name(file_name) } fn numbered_backup_path(path: &Path) -> PathBuf { - let file_name = path.file_name().unwrap_or_default(); - for i in 1_u64.. { - let mut numbered_file_name = file_name.to_os_string(); - numbered_file_name.push(format!(".~{i}~")); - let path = path.with_file_name(numbered_file_name); - if !path.exists() { - return path; + let mut i: u64 = 1; + loop { + let new_path = simple_backup_path(path, OsString::from(format!(".~{i}~"))); + if !new_path.exists() { + return new_path; } + i += 1; } - panic!("cannot create backup") } -fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf { - let file_name = path.file_name().unwrap_or_default(); - let mut numbered_file_name = file_name.to_os_string(); - numbered_file_name.push(".~1~"); - let test_path = path.with_file_name(numbered_file_name); +fn existing_backup_path>(path: &Path, suffix: S) -> PathBuf { + let test_path = simple_backup_path(path, OsString::from(".~1~")); if test_path.exists() { - numbered_backup_path(path) - } else { - simple_backup_path(path, suffix) + return numbered_backup_path(path); } + simple_backup_path(path, suffix.as_ref()) } /// Returns true if the source file is likely to be the simple backup file for the target file. @@ -695,6 +695,16 @@ mod tests { assert_eq!(result, "-v"); } + #[test] + fn test_suffix_rejects_path_traversal() { + let _dummy = TEST_MUTEX.lock().unwrap(); + let matches = + make_app().get_matches_from(vec!["command", "-b", "--suffix", "_/../../dest"]); + + let result = determine_backup_suffix(&matches); + assert_eq!(result, DEFAULT_BACKUP_SUFFIX); + } + #[test] fn test_numbered_backup_path() { assert_eq!(numbered_backup_path(Path::new("")), PathBuf::from(".~1~")); diff --git a/tests/by-util/test_ln.rs b/tests/by-util/test_ln.rs index 26e109f12..114063895 100644 --- a/tests/by-util/test_ln.rs +++ b/tests/by-util/test_ln.rs @@ -1023,3 +1023,23 @@ fn test_ln_hard_link_dir() { .fails() .stderr_contains("hard link not allowed for directory"); } + +#[test] +fn test_ln_backup_no_path_traversal() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.touch("a"); + at.touch("b"); + at.mkdir("b_"); + + scene + .ucmd() + .args(&["-S", "_/../c", "-s", "a", "b"]) + .succeeds(); + + assert!(!at.file_exists("c")); + assert!(at.plus("b").is_symlink()); + assert!(at.file_exists("b~")); + assert!(!at.plus("b~").is_symlink()); +}