diff --git a/crates/engine/src/rename/fs.rs b/crates/engine/src/rename/fs.rs index 71de1a7843..30b6a3e30c 100644 --- a/crates/engine/src/rename/fs.rs +++ b/crates/engine/src/rename/fs.rs @@ -46,6 +46,9 @@ pub(crate) trait MoveFs { fn packet_version(&self, path: &Path) -> std::io::Result { use std::io::Read; let mut file = std::fs::File::open(path)?; + if !file.metadata()?.is_file() { + return Err(std::io::Error::other(format!("{} is not a regular file", path.display()))); + } let mut hash = lightcraft_preview::Hasher128::new(); let mut len = 0u64; let mut buffer = [0u8; 16 * 1024]; @@ -141,8 +144,14 @@ impl MoveFs for RealFs { .map(|k| b.with_file_name(format!(".{name}.lc-partial-{}-{k}", std::process::id()))) .find(|p| !self.exists(p)) .ok_or_else(|| std::io::Error::other(format!("no free temporary name beside {}", b.display())))?; + // a sidecar found beside the photo by its name is copied only when it is a plain file: a + // link there would have its target's contents copied into a new, readable file + let checked = std::fs::symlink_metadata(a)?; + if !checked.is_file() { + return Err(std::io::Error::other(format!("{} is not a regular file", a.display()))); + } let copied = (|| { - let mut from = std::fs::File::open(a)?; + let mut from = open_regular(a, &checked)?; let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(&partial)?; std::io::copy(&mut from, &mut f)?; f.sync_all() @@ -163,6 +172,33 @@ impl MoveFs for RealFs { } } +/// Opens `a` for reading and checks the open handle is the regular file `checked` (the +/// `symlink_metadata` of `a` taken before): the path check alone can be outrun by a swap for a +/// link, whose target the open would follow. +fn open_regular(a: &Path, checked: &std::fs::Metadata) -> std::io::Result { + let mut options = std::fs::OpenOptions::new(); + options.read(true); + // FILE_FLAG_OPEN_REPARSE_POINT: a symbolic link is opened itself, and then refused below + #[cfg(windows)] + std::os::windows::fs::OpenOptionsExt::custom_flags(&mut options, 0x0020_0000); + let file = options.open(a)?; + let opened = file.metadata()?; + #[cfg(unix)] + let same = { + use std::os::unix::fs::MetadataExt; + (opened.dev(), opened.ino()) == (checked.dev(), checked.ino()) + }; + #[cfg(not(unix))] + let same = { + let _ = checked; + true + }; + if !opened.is_file() || !same { + return Err(std::io::Error::other(format!("{} is not a regular file", a.display()))); + } + Ok(file) +} + /// Move `a` to `b` on another volume: copy (never replacing), compare the copy with the original /// byte for byte, and only then remove the original. A copy that doesn't match is removed. fn move_across_volumes(fs: &RealFs, a: &Path, b: &Path) -> std::io::Result<()> { @@ -240,3 +276,25 @@ pub(super) fn move_one(fs: &dyn MoveFs, a: &Path, b: &Path) -> std::io::Result<( Err(e2) => std::io::Error::other(format!("{e}; it could not be given its old name back either ({e2}): it is now {}", tmp.display())), }) } + +#[cfg(all(test, unix))] +mod open_regular_tests { + use super::open_regular; + + #[test] + fn a_link_swapped_in_after_the_check_is_not_followed() { + let dir = std::env::temp_dir().join(format!("lightcraft-open-regular-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let (file, other, link) = (dir.join("a.xmp"), dir.join("secret"), dir.join("link.xmp")); + std::fs::write(&file, b"x").unwrap(); + std::fs::write(&other, b"secret").unwrap(); + let checked = std::fs::symlink_metadata(&file).unwrap(); + assert!(open_regular(&file, &checked).is_ok()); + // the name now holds a link to another file: the handle is not the one checked + std::fs::remove_file(&file).unwrap(); + std::os::unix::fs::symlink(&other, &link).unwrap(); + std::fs::rename(&link, &file).unwrap(); + assert!(open_regular(&file, &checked).is_err(), "the link's target must not be opened"); + std::fs::remove_dir_all(&dir).unwrap(); + } +} diff --git a/crates/engine/src/sidecar.rs b/crates/engine/src/sidecar.rs index 9f3e518a49..252dd911a6 100644 --- a/crates/engine/src/sidecar.rs +++ b/crates/engine/src/sidecar.rs @@ -455,6 +455,11 @@ fn write_atomic(path: &Path, data: &[u8]) -> std::io::Result<()> { /// Copy `path` to a new `.bak-` (`-1`, `-2`… if taken; never overwriting). fn backup(path: &Path, stamp: &str) -> std::io::Result { use std::io::Write; + // only a plain file is copied aside: a link under the sidecar's name would have its target's + // contents copied into a new, readable file + if !std::fs::symlink_metadata(path)?.is_file() { + return Err(std::io::Error::other(format!("{} is not a regular file", path.display()))); + } let bytes = std::fs::read(path)?; let stamp: String = stamp.chars().filter(char::is_ascii_alphanumeric).collect(); let name = path.file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_default(); diff --git a/crates/engine/src/tests/move_to_share.rs b/crates/engine/src/tests/move_to_share.rs index ed824b29d8..e7602818e4 100644 --- a/crates/engine/src/tests/move_to_share.rs +++ b/crates/engine/src/tests/move_to_share.rs @@ -254,6 +254,19 @@ fn other_device(here: &std::path::Path) -> Option { } } +#[cfg(unix)] +#[test] +fn a_link_is_not_copied_as_its_target() { + use crate::rename::MoveFs; + let dir = Scratch::new("copy-link"); + std::fs::create_dir_all(dir.0.join("local")).unwrap(); + std::fs::create_dir_all(dir.0.join("share")).unwrap(); + std::fs::write(dir.0.join("local/private.txt"), b"secret").unwrap(); + std::os::unix::fs::symlink(dir.0.join("local/private.txt"), dir.0.join("local/photo.xmp")).unwrap(); + assert!(crate::rename::RealFs.copy_no_replace(&dir.0.join("local/photo.xmp"), &dir.0.join("share/photo.xmp")).is_err()); + assert!(!dir.0.join("share/photo.xmp").exists()); +} + #[test] fn the_copy_across_volumes_is_whole_or_absent_and_never_replaces() { use crate::rename::MoveFs;