Skip to content

Commit 1514ccd

Browse files
committed
mv,cp: fix xattr TOCTOU by using file descriptor-based operations
Closes: #10014
1 parent ff6dd99 commit 1514ccd

4 files changed

Lines changed: 168 additions & 6 deletions

File tree

src/uu/cp/src/cp.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use std::os::unix::net::UnixListener;
1616
use std::path::{Path, PathBuf, StripPrefixError};
1717
use std::{fmt, io};
1818
#[cfg(all(unix, not(target_os = "android")))]
19-
use uucore::fsxattr::{copy_xattrs, copy_xattrs_skip_selinux};
19+
use uucore::fsxattr::{copy_xattrs_fd, copy_xattrs_skip_selinux};
2020
use uucore::translate;
2121

2222
use clap::{Arg, ArgAction, ArgMatches, Command, builder::ValueParser, value_parser};
@@ -1715,6 +1715,8 @@ pub(crate) fn set_selinux_context(path: &Path, context: Option<&String>) -> Copy
17151715
/// or if xattr copying fails.
17161716
#[cfg(all(unix, not(target_os = "android")))]
17171717
fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyResult<()> {
1718+
use std::fs::File;
1719+
use uucore::fsxattr::copy_xattrs;
17181720
let metadata = fs::symlink_metadata(dest)?;
17191721

17201722
// Check if the destination file is currently read-only for the user.
@@ -1734,6 +1736,13 @@ fn copy_extended_attrs(source: &Path, dest: &Path, skip_selinux: bool) -> CopyRe
17341736
// When -Z is used, skip copying security.selinux xattr so that
17351737
// the default context can be set instead of preserving from source
17361738
copy_xattrs_skip_selinux(source, dest)
1739+
} else if metadata.is_file() {
1740+
// Use file descriptor-based operations for regular files to avoid TOCTOU races.
1741+
// Directories cannot be opened with write mode for xattr operations
1742+
// Symlinks (especially dangling ones) cannot be opened via File::open
1743+
let source_file = File::open(source)?;
1744+
let dest_file = OpenOptions::new().write(true).open(dest)?;
1745+
copy_xattrs_fd(&source_file, &dest_file)
17371746
} else {
17381747
copy_xattrs(source, dest)
17391748
};

src/uu/mv/src/mv.rs

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -967,8 +967,15 @@ fn rename_dir_fallback(
967967
(_, _) => None,
968968
};
969969

970+
// Retrieve xattrs before copying (directories use path-based operations
971+
// since they cannot be opened in write mode for xattr operations)
970972
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
971-
let xattrs = fsxattr::retrieve_xattrs(from).unwrap_or_else(|_| FxHashMap::default());
973+
let xattrs = {
974+
use std::fs::File;
975+
File::open(from)
976+
.and_then(|f| fsxattr::retrieve_xattrs_fd(&f))
977+
.unwrap_or_else(|_| FxHashMap::default())
978+
};
972979

973980
// Use directory copying (with or without hardlink support)
974981
let result = copy_dir_contents(
@@ -983,8 +990,14 @@ fn rename_dir_fallback(
983990
display_manager,
984991
);
985992

993+
// Apply xattrs after directory contents are copied, ignoring ENOTSUP errors
994+
// (filesystem doesn't support xattrs, which is acceptable for cross-device moves)
986995
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
987-
fsxattr::apply_xattrs(to, xattrs)?;
996+
if let Err(e) = fsxattr::apply_xattrs(to, xattrs) {
997+
if e.raw_os_error() != Some(libc::EOPNOTSUPP) {
998+
return Err(e);
999+
}
1000+
}
9881001

9891002
result?;
9901003

@@ -1082,7 +1095,16 @@ fn copy_dir_contents_recursive(
10821095
rename_symlink_fallback(&from_path, &to_path)?;
10831096
}
10841097

1085-
print_verbose(&from_path, &to_path);
1098+
// Print verbose message for symlink
1099+
if verbose {
1100+
let message = translate!("mv-verbose-renamed", "from" => from_path.quote(), "to" => to_path.quote());
1101+
match display_manager {
1102+
Some(pb) => pb.suspend(|| {
1103+
println!("{message}");
1104+
}),
1105+
None => println!("{message}"),
1106+
}
1107+
}
10861108
} else if from_path.is_dir() {
10871109
// Recursively copy subdirectory (only real directories, not symlinks)
10881110
fs::create_dir_all(&to_path)?;
@@ -1215,12 +1237,19 @@ fn rename_file_fallback(
12151237
Ok(())
12161238
}
12171239

1218-
/// Copy xattrs from source to destination, ignoring ENOTSUP/EOPNOTSUPP errors.
1240+
/// Copy xattrs from source to destination using file descriptors, ignoring ENOTSUP/EOPNOTSUPP errors.
1241+
/// This version avoids TOCTOU races by operating on file descriptors rather than paths.
12191242
/// These errors indicate the filesystem doesn't support extended attributes,
12201243
/// which is acceptable when moving files across filesystems.
12211244
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
12221245
fn copy_xattrs_if_supported(from: &Path, to: &Path) -> io::Result<()> {
1223-
match fsxattr::copy_xattrs(from, to) {
1246+
use std::fs::{File, OpenOptions};
1247+
1248+
// Open both files to pin their inodes, avoiding TOCTOU races during xattr operations
1249+
let source_file = File::open(from)?;
1250+
let dest_file = OpenOptions::new().write(true).open(to)?;
1251+
1252+
match fsxattr::copy_xattrs_fd(&source_file, &dest_file) {
12241253
Ok(()) => Ok(()),
12251254
Err(e) if e.raw_os_error() == Some(libc::EOPNOTSUPP) => Ok(()),
12261255
Err(e) => Err(e),

src/uucore/src/lib/features/fsxattr.rs

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@
99
use itertools::Itertools;
1010
use rustc_hash::FxHashMap;
1111
use std::ffi::{OsStr, OsString};
12+
use std::fs::File;
1213
#[cfg(unix)]
1314
use std::os::unix::ffi::OsStrExt;
1415
use std::path::Path;
16+
use xattr::FileExt;
1517

1618
/// Copies extended attributes (xattrs) from one file or directory to another.
1719
///
@@ -45,6 +47,29 @@ pub fn copy_xattrs_skip_selinux<P: AsRef<Path>>(source: P, dest: P) -> std::io::
4547
Ok(())
4648
}
4749

50+
/// Copies extended attributes (xattrs) from one file to another using file descriptors.
51+
///
52+
/// This version avoids TOCTOU (time-of-check to time-of-use) races by operating
53+
/// on open file descriptors rather than paths, ensuring all operations target
54+
/// the same inodes throughout.
55+
///
56+
/// # Arguments
57+
///
58+
/// * `source` - A reference to the source file (open file descriptor).
59+
/// * `dest` - A reference to the destination file (open file descriptor).
60+
///
61+
/// # Returns
62+
///
63+
/// A result indicating success or failure.
64+
pub fn copy_xattrs_fd(source: &File, dest: &File) -> std::io::Result<()> {
65+
for attr_name in source.list_xattr()? {
66+
if let Some(value) = source.get_xattr(&attr_name)? {
67+
dest.set_xattr(&attr_name, &value)?;
68+
}
69+
}
70+
Ok(())
71+
}
72+
4873
/// Retrieves the extended attributes (xattrs) of a given file or directory.
4974
///
5075
/// # Arguments
@@ -64,6 +89,28 @@ pub fn retrieve_xattrs<P: AsRef<Path>>(source: P) -> std::io::Result<FxHashMap<O
6489
Ok(attrs)
6590
}
6691

92+
/// Retrieves the extended attributes (xattrs) of a given file using a file descriptor.
93+
///
94+
/// This version avoids TOCTOU races by operating on an open file descriptor
95+
/// rather than a path, ensuring all operations target the same inode.
96+
///
97+
/// # Arguments
98+
///
99+
/// * `source` - A reference to the file (open file descriptor).
100+
///
101+
/// # Returns
102+
///
103+
/// A result containing a HashMap of attribute names and values, or an error.
104+
pub fn retrieve_xattrs_fd(source: &File) -> std::io::Result<FxHashMap<OsString, Vec<u8>>> {
105+
let mut attrs = FxHashMap::default();
106+
for attr_name in source.list_xattr()? {
107+
if let Some(value) = source.get_xattr(&attr_name)? {
108+
attrs.insert(attr_name, value);
109+
}
110+
}
111+
Ok(attrs)
112+
}
113+
67114
/// Applies extended attributes (xattrs) to a given file or directory.
68115
///
69116
/// # Arguments
@@ -84,6 +131,26 @@ pub fn apply_xattrs<P: AsRef<Path>>(
84131
Ok(())
85132
}
86133

134+
/// Applies extended attributes (xattrs) to a given file using a file descriptor.
135+
///
136+
/// This version avoids TOCTOU races by operating on an open file descriptor
137+
/// rather than a path, ensuring all operations target the same inode.
138+
///
139+
/// # Arguments
140+
///
141+
/// * `dest` - A reference to the file (open file descriptor).
142+
/// * `xattrs` - A HashMap containing attribute names and their corresponding values.
143+
///
144+
/// # Returns
145+
///
146+
/// A result indicating success or failure.
147+
pub fn apply_xattrs_fd(dest: &File, xattrs: FxHashMap<OsString, Vec<u8>>) -> std::io::Result<()> {
148+
for (attr, value) in xattrs {
149+
dest.set_xattr(&attr, &value)?;
150+
}
151+
Ok(())
152+
}
153+
87154
/// Checks if a file has an Access Control List (ACL) based on its extended attributes.
88155
///
89156
/// # Arguments
@@ -299,4 +366,59 @@ mod tests {
299366
assert!(has_security_cap_acl(&file_path));
300367
}
301368
}
369+
370+
#[test]
371+
fn test_copy_xattrs_fd() {
372+
use std::fs::OpenOptions;
373+
374+
let temp_dir = tempdir().unwrap();
375+
let source_path = temp_dir.path().join("source.txt");
376+
let dest_path = temp_dir.path().join("dest.txt");
377+
378+
File::create(&source_path).unwrap();
379+
File::create(&dest_path).unwrap();
380+
381+
let test_attr = "user.test_fd";
382+
let test_value = b"test value fd";
383+
xattr::set(&source_path, test_attr, test_value).unwrap();
384+
385+
let source_file = File::open(&source_path).unwrap();
386+
let dest_file = OpenOptions::new().write(true).open(&dest_path).unwrap();
387+
388+
copy_xattrs_fd(&source_file, &dest_file).unwrap();
389+
390+
let copied_value = xattr::get(&dest_path, test_attr).unwrap().unwrap();
391+
assert_eq!(copied_value, test_value);
392+
}
393+
394+
#[test]
395+
fn test_apply_and_retrieve_xattrs_fd() {
396+
use std::fs::OpenOptions;
397+
398+
let temp_dir = tempdir().unwrap();
399+
let file_path = temp_dir.path().join("test_file.txt");
400+
401+
File::create(&file_path).unwrap();
402+
403+
let mut test_xattrs = FxHashMap::default();
404+
let test_attr = "user.test_attr_fd";
405+
let test_value = b"test value fd";
406+
test_xattrs.insert(OsString::from(test_attr), test_value.to_vec());
407+
408+
// Apply using file descriptor
409+
let file = OpenOptions::new().write(true).open(&file_path).unwrap();
410+
apply_xattrs_fd(&file, test_xattrs).unwrap();
411+
drop(file);
412+
413+
// Retrieve using file descriptor
414+
let file = File::open(&file_path).unwrap();
415+
let retrieved_xattrs = retrieve_xattrs_fd(&file).unwrap();
416+
assert!(retrieved_xattrs.contains_key(OsString::from(test_attr).as_os_str()));
417+
assert_eq!(
418+
retrieved_xattrs
419+
.get(OsString::from(test_attr).as_os_str())
420+
.unwrap(),
421+
test_value
422+
);
423+
}
302424
}

tests/by-util/test_mv.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2882,6 +2882,7 @@ fn test_mv_cross_device_symlink_preserved() {
28822882
at.write("src_dir/local.txt", "local content");
28832883
symlink("/etc", at.plus("src_dir/etc_link")).expect("Failed to create symlink");
28842884

2885+
// Verify initial state
28852886
assert!(at.is_symlink("src_dir/etc_link"));
28862887

28872888
// Force cross-filesystem move using /dev/shm (tmpfs)
@@ -2896,6 +2897,7 @@ fn test_mv_cross_device_symlink_preserved() {
28962897
.succeeds()
28972898
.no_stderr();
28982899

2900+
// Verify source was removed
28992901
assert!(!at.dir_exists("src_dir"));
29002902

29012903
// Verify the symlink was preserved (not expanded)

0 commit comments

Comments
 (0)