uucore: Disallow slashes in determine_backup_suffix (#11149)

* uucore: Disallow slashes in determine_backup_suffix; use ln's OsStr-compatible implementations of *_backup_path.

* ln: Add test for backup suffix containing slashes.

---------

Co-authored-by: aweinstock <avi@zellic.io>

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<P1: AsRef<Path>, P2: AsRef<Path>>(src: P1, dst: P2) -> std::io::Result<()> {
     if src.as_ref().is_dir() {

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::<String>(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<BackupMode> {
     }
 }
 
-pub fn get_backup_path(
+pub fn get_backup_path<S: AsRef<OsStr>>(
     backup_mode: BackupMode,
     backup_path: &Path,
-    suffix: &str,
+    suffix: S,
 ) -> Option<PathBuf> {
     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<S: AsRef<OsStr>>(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<S: AsRef<OsStr>>(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~"));

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());
+}