From 9d0a3cd6dd996118616cb3cede3064296bda2fda Mon Sep 17 00:00:00 2001 From: Chen Chen Date: Wed, 18 Dec 2024 14:52:03 -0500 Subject: [PATCH] install: create destination file with safer modes before copy --- src/uu/install/src/install.rs | 74 ++++++++++++++++++++++++++++------- tests/by-util/test_install.rs | 64 ++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 15 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 331a50f6741..8428624b69f 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -13,9 +13,12 @@ use filetime::{set_file_times, FileTime}; use std::error::Error; use std::fmt::{Debug, Display}; use std::fs; +#[cfg(not(unix))] use std::fs::File; use std::os::unix::fs::MetadataExt; #[cfg(unix)] +use std::os::unix::fs::OpenOptionsExt; +#[cfg(unix)] use std::os::unix::prelude::OsStrExt; use std::path::{Path, PathBuf, MAIN_SEPARATOR}; use std::process; @@ -748,29 +751,70 @@ fn perform_backup(to: &Path, b: &Behavior) -> UResult> { /// Returns an empty Result or an error in case of failure. /// fn copy_file(from: &Path, to: &Path) -> UResult<()> { + // open the source file and get a file descriptor, making sure the source file exists + #[cfg(unix)] + let src = fs::OpenOptions::new().read(true).open(from); + #[cfg(not(unix))] + let src = File::open(from); + if let Err(e) = src { + show_error!( + "Failed to open source file {}. Error: {:?}", + from.display(), + e + ); + return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), e).into()); + } + let mut src = src.unwrap(); + // fs::copy fails if destination is a invalid symlink. // so lets just remove all existing files at destination before copy. - if let Err(e) = fs::remove_file(to) { - if e.kind() != std::io::ErrorKind::NotFound { - show_error!( - "Failed to remove existing file {}. Error: {:?}", - to.display(), - e - ); + let remove_destination = || { + if let Err(e) = fs::remove_file(to) { + if e.kind() != std::io::ErrorKind::NotFound { + show_error!( + "Failed to remove existing file {}. Error: {:?}", + to.display(), + e + ); + } } + }; + remove_destination(); + + // create the destination file first. Using safer mode on unix to avoid + // potential unsafe mode between time-of-creation and time-of-chmod. + #[cfg(unix)] + let dst = fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(to); + #[cfg(not(unix))] + let dst = File::create(to); + + if let Err(e) = dst { + show_error!( + "Failed to create destination file {}. Error: {:?}", + to.display(), + e + ); + return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), e).into()); } - - if from.as_os_str() == "/dev/null" { - /* workaround a limitation of fs::copy - * https://github.com/rust-lang/rust/issues/79390 - */ - if let Err(err) = File::create(to) { + let mut dst = dst.unwrap(); + + /* workaround a limitation of fs::copy: skip copy if source is /dev/null + * https://github.com/rust-lang/rust/issues/79390 + */ + if from.as_os_str() != "/dev/null" { + // use `std::io::copy` to avoid unsafe file modes + if let Err(err) = std::io::copy(&mut src, &mut dst) { + drop(src); + drop(dst); + remove_destination(); return Err( InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into(), ); } - } else if let Err(err) = fs::copy(from, to) { - return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into()); } Ok(()) } diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index f1e3302e138..e42cefcc287 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -1717,3 +1717,67 @@ fn test_install_root_combined() { run_and_check(&["-Cv", "c", "d"], "d", 0, 0); run_and_check(&["-Cv", "c", "d"], "d", 0, 0); } + +#[cfg(all(unix))] +#[test] +fn test_install_only_when_source_exists() { + let scene = TestScenario::new(util_name!()); + + let at = &scene.fixtures; + + let file1 = "source_file"; + let file2 = "target_file"; + + at.touch(file2); + at.append(file2, "test"); + + // if source file does not exist, the install should fail and the + // destination file should not be removed, and content is not tampered + scene + .ucmd() + .arg(file1) + .arg(file2) + .arg("--mode=400") + .fails() + .stderr_contains("No such file or directory"); + assert!(at.file_exists(file2)); + assert_eq!(at.read(file2), "test"); +} + +#[cfg(all(unix, feature = "chmod"))] +#[test] +fn test_install_copy_failures() { + let scene = TestScenario::new(util_name!()); + + let at = &scene.fixtures; + + let file1 = "source_file"; + let file2 = "target_file"; + + at.touch(file1); + scene.ccmd("chmod").arg("000").arg(file1).succeeds(); + + // if source file is not readable, it will raise a permission error. + // since we create the file with mode 0600 before `fs::copy`, if the + // copy failed, the file should be removed. + scene + .ucmd() + .arg(file1) + .arg(file2) + .arg("--mode=400") + .fails() + .stderr_contains("permission denied"); + assert!(!at.file_exists(file2)); + + // if source file is good to copy, it should succeed and set the + // destination file permissions accordingly + scene.ccmd("chmod").arg("644").arg(file1).succeeds(); + scene + .ucmd() + .arg(file1) + .arg(file2) + .arg("--mode=400") + .succeeds(); + assert!(at.file_exists(file2)); + assert_eq!(0o100_400_u32, at.metadata(file2).permissions().mode()); +}