diff --git a/.github/workflows/CICD.yml b/.github/workflows/CICD.yml index f22f3873399..40091074f37 100644 --- a/.github/workflows/CICD.yml +++ b/.github/workflows/CICD.yml @@ -900,10 +900,10 @@ jobs: persist-credentials: false - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - - name: Install strace - run: sudo apt-get update && sudo apt-get install -y strace + - name: Install strace and libselinux headers (needed to build chcon) + run: sudo apt-get update && sudo apt-get install -y strace libselinux1-dev - name: Build utilities with safe traversal - run: cargo build --profile=release-small -p uu_rm -p uu_chmod -p uu_chown -p uu_chgrp -p uu_mv -p uu_du -p uu_split + run: cargo build --profile=release-small -p uu_rm -p uu_chmod -p uu_chown -p uu_chgrp -p uu_mv -p uu_du -p uu_split -p uu_chcon - name: Run safe traversal verification run: ./util/check-safe-traversal.sh diff --git a/Cargo.lock b/Cargo.lock index 5ca43096104..6bea0cd88b6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3273,6 +3273,7 @@ dependencies = [ "fluent", "fts-sys", "libc", + "rustix", "selinux", "thiserror 2.0.18", "uucore", diff --git a/src/uu/chcon/Cargo.toml b/src/uu/chcon/Cargo.toml index 04665ba37e2..73398f0cd72 100644 --- a/src/uu/chcon/Cargo.toml +++ b/src/uu/chcon/Cargo.toml @@ -27,6 +27,7 @@ selinux = { workspace = true } thiserror = { workspace = true } libc = { workspace = true } fts-sys = { workspace = true } +rustix = { workspace = true, features = ["fs"] } fluent = { workspace = true } [[bin]] diff --git a/src/uu/chcon/src/chcon.rs b/src/uu/chcon/src/chcon.rs index 8543c166e2c..eea40398f63 100644 --- a/src/uu/chcon/src/chcon.rs +++ b/src/uu/chcon/src/chcon.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (vars) RFILE +// spell-checker:ignore (vars) RFILE RDONLY CLOEXEC fgetfilecon fsetfilecon #![cfg(any(target_os = "linux", target_os = "android"))] #![allow(clippy::upper_case_acronyms)] @@ -14,10 +14,12 @@ use uucore::translate; use uucore::{display::Quotable, format_usage, show_error, show_warning}; use clap::{Arg, ArgAction, ArgMatches, Command}; +use rustix::fs::{Mode, OFlags, openat}; use selinux::{OpaqueSecurityContext, SecurityContext}; use std::borrow::Cow; use std::ffi::{CStr, CString, OsStr, OsString}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}; use std::os::raw::c_int; use std::path::{Path, PathBuf}; use std::{fs, io}; @@ -499,111 +501,117 @@ fn process_file( fts: &mut fts::FTS, root_dev_ino: Option, ) -> Result<()> { - let mut entry = fts.last_entry_ref().unwrap(); + let (file_full_name, fts_access_path, mut result) = { + let mut entry = fts + .last_entry_ref() + .expect("last_entry_ref is present after successful read_next_entry"); + + let file_full_name = entry.path().map(PathBuf::from).ok_or_else(|| { + Error::from_io( + translate!("chcon-op-file-name-validation"), + io::ErrorKind::InvalidInput.into(), + ) + })?; + + let fts_access_path = entry.access_path().map(PathBuf::from).ok_or_else(|| { + let err = io::ErrorKind::InvalidInput.into(); + Error::from_io1( + translate!("chcon-op-file-name-validation"), + &file_full_name, + err, + ) + })?; + + let err = |s, k: io::ErrorKind| Error::from_io1(s, &file_full_name, k.into()); + + let fts_err = |s| { + let r = io::Error::from_raw_os_error(entry.errno()); + Err(Error::from_io1(s, &file_full_name, r)) + }; + + // SAFETY: If `entry.fts_statp` is not null, then it is assumed to be valid. + let file_dev_ino: DeviceAndINode = if let Some(st) = entry.stat() { + st.try_into()? + } else { + return Err(err( + translate!("chcon-op-getting-meta-data"), + io::ErrorKind::InvalidInput, + )); + }; - let file_full_name = entry.path().map(PathBuf::from).ok_or_else(|| { - Error::from_io( - translate!("chcon-op-file-name-validation"), - io::ErrorKind::InvalidInput.into(), - ) - })?; + let mut result = Ok(()); - let fts_access_path = entry.access_path().ok_or_else(|| { - let err = io::ErrorKind::InvalidInput.into(); - Error::from_io1( - translate!("chcon-op-file-name-validation"), - &file_full_name, - err, - ) - })?; + match entry.flags() { + fts_sys::FTS_D if options.recursive_mode.is_recursive() => { + if root_dev_ino_check(root_dev_ino, file_dev_ino) { + // This happens e.g., with "chcon -R --preserve-root ... /" + // and with "chcon -RH --preserve-root ... symlink-to-root". + root_dev_ino_warn(&file_full_name); - let err = |s, k: io::ErrorKind| Error::from_io1(s, &file_full_name, k.into()); + // Tell fts not to traverse into this hierarchy. + let _ignored = fts.set(fts_sys::FTS_SKIP); - let fts_err = |s| { - let r = io::Error::from_raw_os_error(entry.errno()); - Err(Error::from_io1(s, &file_full_name, r)) - }; + // Ensure that we do not process "/" on the second visit. + let _ignored = fts.read_next_entry(); - // SAFETY: If `entry.fts_statp` is not null, then it is assumed to be valid. - let file_dev_ino: DeviceAndINode = if let Some(st) = entry.stat() { - st.try_into()? - } else { - return Err(err( - translate!("chcon-op-getting-meta-data"), - io::ErrorKind::InvalidInput, - )); - }; - - let mut result = Ok(()); + return Err(err( + translate!("chcon-op-modifying-root-path"), + io::ErrorKind::PermissionDenied, + )); + } - match entry.flags() { - fts_sys::FTS_D if options.recursive_mode.is_recursive() => { - if root_dev_ino_check(root_dev_ino, file_dev_ino) { - // This happens e.g., with "chcon -R --preserve-root ... /" - // and with "chcon -RH --preserve-root ... symlink-to-root". - root_dev_ino_warn(&file_full_name); + return Ok(()); + } - // Tell fts not to traverse into this hierarchy. - let _ignored = fts.set(fts_sys::FTS_SKIP); + fts_sys::FTS_DP if !options.recursive_mode.is_recursive() => { + return Ok(()); + } - // Ensure that we do not process "/" on the second visit. - let _ignored = fts.read_next_entry(); + fts_sys::FTS_NS => { + // For a top-level file or directory, this FTS_NS (stat failed) indicator is determined + // at the time of the initial fts_open call. With programs like chmod, chown, and chgrp, + // that modify permissions, it is possible that the file in question is accessible when + // control reaches this point. So, if this is the first time we've seen the FTS_NS for + // this file, tell fts_read to stat it "again". + if entry.level() == 0 && entry.number() == 0 { + entry.set_number(1); + let _ignored = fts.set(fts_sys::FTS_AGAIN); + return Ok(()); + } - return Err(err( - translate!("chcon-op-modifying-root-path"), - io::ErrorKind::PermissionDenied, - )); + result = fts_err(translate!("chcon-op-accessing")); } - return Ok(()); - } + fts_sys::FTS_ERR => result = fts_err(translate!("chcon-op-accessing")), - fts_sys::FTS_DP if !options.recursive_mode.is_recursive() => { - return Ok(()); - } + fts_sys::FTS_DNR => result = fts_err(translate!("chcon-op-reading-directory")), - fts_sys::FTS_NS => { - // For a top-level file or directory, this FTS_NS (stat failed) indicator is determined - // at the time of the initial fts_open call. With programs like chmod, chown, and chgrp, - // that modify permissions, it is possible that the file in question is accessible when - // control reaches this point. So, if this is the first time we've seen the FTS_NS for - // this file, tell fts_read to stat it "again". - if entry.level() == 0 && entry.number() == 0 { - entry.set_number(1); - let _ignored = fts.set(fts_sys::FTS_AGAIN); - return Ok(()); + fts_sys::FTS_DC + if cycle_warning_required(options.recursive_mode.fts_open_options(), &entry) => + { + emit_cycle_warning(&file_full_name); + return Err(err( + translate!("chcon-op-reading-cyclic-directory"), + io::ErrorKind::InvalidData, + )); } - result = fts_err(translate!("chcon-op-accessing")); + _ => {} } - fts_sys::FTS_ERR => result = fts_err(translate!("chcon-op-accessing")), - - fts_sys::FTS_DNR => result = fts_err(translate!("chcon-op-reading-directory")), - - fts_sys::FTS_DC - if cycle_warning_required(options.recursive_mode.fts_open_options(), &entry) => + if entry.flags() == fts_sys::FTS_DP + && result.is_ok() + && root_dev_ino_check(root_dev_ino, file_dev_ino) { - emit_cycle_warning(&file_full_name); - return Err(err( - translate!("chcon-op-reading-cyclic-directory"), - io::ErrorKind::InvalidData, + root_dev_ino_warn(&file_full_name); + result = Err(err( + translate!("chcon-op-modifying-root-path"), + io::ErrorKind::PermissionDenied, )); } - _ => {} - } - - if entry.flags() == fts_sys::FTS_DP - && result.is_ok() - && root_dev_ino_check(root_dev_ino, file_dev_ino) - { - root_dev_ino_warn(&file_full_name); - result = Err(err( - translate!("chcon-op-modifying-root-path"), - io::ErrorKind::PermissionDenied, - )); - } + (file_full_name, fts_access_path, result) + }; if result.is_ok() { if options.verbose { @@ -613,7 +621,17 @@ fn process_file( ); } - result = change_file_context(options, context, fts_access_path); + let traversal_dir_fd = fts + .current_dir_fd() + .map_err(|r| Error::from_io1(translate!("chcon-op-accessing"), &file_full_name, r))?; + + result = change_file_context( + options, + context, + traversal_dir_fd.as_fd(), + &fts_access_path, + &file_full_name, + ); } if !options.recursive_mode.is_recursive() { @@ -622,13 +640,51 @@ fn process_file( result } +fn open_target_fd( + traversal_dir_fd: BorrowedFd<'_>, + target_path: &Path, + affect_symlink_referent: bool, + display_path: &Path, +) -> Result { + // Anchor the open to the traversal directory fd so a concurrent rename or + // symlink swap cannot redirect the relabel off-tree. O_PATH hands back the + // entry itself without opening it for I/O: it never blocks on a FIFO and + // does not require read permission, yet the SELinux get/set still work + // through /proc/self/fd (see change_file_context). O_NOFOLLOW keeps us on + // the symlink itself unless we were asked to act on its referent. + let mut flags = OFlags::PATH | OFlags::CLOEXEC; + if !affect_symlink_referent { + flags |= OFlags::NOFOLLOW; + } + + openat(traversal_dir_fd, target_path, flags, Mode::empty()) + .map_err(io::Error::from) + .map_err(|err| Error::from_io1(translate!("chcon-op-accessing"), display_path, err)) +} + fn change_file_context( options: &Options, context: &SELinuxSecurityContext, - path: &Path, + traversal_dir_fd: BorrowedFd<'_>, + target_path: &Path, + display_path: &Path, ) -> Result<()> { type SetValueProc = fn(&OpaqueSecurityContext, &CStr) -> selinux::errors::Result<()>; + let target_fd = open_target_fd( + traversal_dir_fd, + target_path, + options.affect_symlink_referent, + display_path, + )?; + + // The fd is O_PATH, so fgetfilecon/fsetfilecon would fail with EBADF. Reach + // the same inode through its /proc/self/fd entry and always dereference it: + // the open above already encoded the follow/no-follow choice, so this magic + // symlink resolves to exactly the inode we anchored. `target_fd` must stay + // alive for as long as this path is used. + let target = PathBuf::from(format!("/proc/self/fd/{}", target_fd.as_raw_fd())); + match &options.mode { CommandLineMode::Custom { user, @@ -641,21 +697,20 @@ fn change_file_context( // components, there isn't really an obvious default. Thus, we just give up. let op = translate!("chcon-op-applying-partial-context"); let err = io::ErrorKind::InvalidInput.into(); - Err(Error::from_io1(op, path, err)) + Err(Error::from_io1(op, display_path, err)) }; - let file_context = - match SecurityContext::of_path(path, options.affect_symlink_referent, false) { - Ok(Some(context)) => context, + let file_context = match SecurityContext::of_path(&target, true, false) { + Ok(Some(context)) => context, - Ok(None) => return err0(), - Err(r) => { - return Err(Error::from_selinux( - translate!("chcon-op-getting-security-context"), - r, - )); - } - }; + Ok(None) => return err0(), + Err(r) => { + return Err(Error::from_selinux( + translate!("chcon-op-getting-security-context"), + r, + )); + } + }; let c_file_context = match file_context.to_c_string() { Ok(Some(context)) => context, @@ -672,7 +727,11 @@ fn change_file_context( let se_context = OpaqueSecurityContext::from_c_str(c_file_context.as_ref()).map_err(|_r| { let err = io::ErrorKind::InvalidInput.into(); - Error::from_io1(translate!("chcon-op-creating-security-context"), path, err) + Error::from_io1( + translate!("chcon-op-creating-security-context"), + display_path, + err, + ) })?; let list: &[(&Option, SetValueProc)] = &[ @@ -686,7 +745,11 @@ fn change_file_context( if let Some(new_value) = new_value { let c_new_value = os_str_to_c_string(new_value).map_err(|_r| { let err = io::ErrorKind::InvalidInput.into(); - Error::from_io1(translate!("chcon-op-creating-security-context"), path, err) + Error::from_io1( + translate!("chcon-op-creating-security-context"), + display_path, + err, + ) })?; set_value_proc(&se_context, &c_new_value).map_err(|r| { @@ -703,7 +766,7 @@ fn change_file_context( Ok(()) // Nothing to change. } else { SecurityContext::from_c_str(&context_string, false) - .set_for_path(path, options.affect_symlink_referent, false) + .set_for_path(&target, true, false) .map_err(|r| { Error::from_selinux(translate!("chcon-op-setting-security-context"), r) }) @@ -713,7 +776,7 @@ fn change_file_context( CommandLineMode::ReferenceBased { .. } | CommandLineMode::ContextBased { .. } => { if let Some(c_context) = context.to_c_string()? { SecurityContext::from_c_str(c_context.as_ref(), false) - .set_for_path(path, options.affect_symlink_referent, false) + .set_for_path(&target, true, false) .map_err(|r| { Error::from_selinux(translate!("chcon-op-setting-security-context"), r) }) @@ -721,7 +784,7 @@ fn change_file_context( let err = io::ErrorKind::InvalidInput.into(); Err(Error::from_io1( translate!("chcon-op-setting-security-context"), - path, + display_path, err, )) } diff --git a/src/uu/chcon/src/fts.rs b/src/uu/chcon/src/fts.rs index 8214058a770..043d3ad4c95 100644 --- a/src/uu/chcon/src/fts.rs +++ b/src/uu/chcon/src/fts.rs @@ -3,14 +3,19 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. +// spell-checker:ignore (vars) RDONLY CLOEXEC + #![cfg(any(target_os = "linux", target_os = "android"))] use std::ffi::{CStr, CString, OsStr}; use std::marker::PhantomData; +use std::os::fd::OwnedFd; use std::os::raw::{c_int, c_long, c_short}; use std::path::Path; use std::{io, iter, ptr, slice}; +use rustix::fs::{Mode, OFlags, open}; + use crate::errors::{Error, Result}; use crate::os_str_to_c_string; @@ -98,8 +103,16 @@ impl FTS { Ok(()) } } -} + pub(crate) fn current_dir_fd(&self) -> io::Result { + open( + Path::new("."), + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + ) + .map_err(io::Error::from) + } +} impl Drop for FTS { fn drop(&mut self) { // SAFETY: We assume calling fts_close() is safe with a non-null `fts` diff --git a/util/check-safe-traversal.sh b/util/check-safe-traversal.sh index f1842a9267f..a049a88e35d 100755 --- a/util/check-safe-traversal.sh +++ b/util/check-safe-traversal.sh @@ -191,7 +191,7 @@ if [ "$USE_MULTICALL" -eq 1 ]; then AVAILABLE_UTILS=$($COREUTILS_BIN --list) else AVAILABLE_UTILS="" - for util in rm chmod chown chgrp du mv cp split; do + for util in rm chmod chown chgrp du mv cp chcon split; do if [ -f "$PROJECT_ROOT/target/${PROFILE}/$util" ]; then AVAILABLE_UTILS="$AVAILABLE_UTILS $util" fi @@ -241,6 +241,50 @@ if echo "$AVAILABLE_UTILS" | grep -q "chgrp"; then assert_descent_nofollow "chgrp" strace_chgrp_recursive_chgrp.log fi +# chcon recursive relabel must resolve each target relative to the traversal +# directory fd with O_NOFOLLOW and operate on the resulting fd (fd-based xattr, +# or /proc/self/fd), never re-resolving the entry by path. Otherwise a +# rename/symlink race could redirect a privileged recursive relabel off-tree +# (issue #11402). This holds even without SELinux: the fd-anchored open happens +# before the SELinux get/set, so the syscalls are observable regardless. +if echo "$AVAILABLE_UTILS" | grep -q "chcon"; then + if [ "$USE_MULTICALL" -eq 1 ]; then + chcon_cmd="$COREUTILS_BIN chcon" + else + chcon_cmd="$PROJECT_ROOT/target/${PROFILE}/chcon" + fi + + mkdir -p chcon_tree/sub + echo a > chcon_tree/file + echo b > chcon_tree/sub/nested + strace -f -e trace=openat,getxattr,lgetxattr,fgetxattr,setxattr,lsetxattr,fsetxattr \ + -o strace_chcon_recursive.log \ + $chcon_cmd -R -t etc_t chcon_tree 2>/dev/null || true + + # Each relabel target is opened relative to a numeric dirfd with O_NOFOLLOW. + if ! grep -qE 'openat\([0-9]+, "(file|sub|nested)", [^)]*O_NOFOLLOW' strace_chcon_recursive.log; then + cat strace_chcon_recursive.log + fail_immediately "chcon -R must open relabel targets relative to the traversal dirfd with O_NOFOLLOW (issue #11402)" + fi + # SELinux xattr ops must be fd-anchored: fgetxattr/fsetxattr on a numeric fd, + # or *xattr on /proc/self/fd. A path-based xattr on the traversal entry is the + # TOCTOU pattern the fix removes. + path_based_xattr=$(grep -E '\bl?(get|set)xattr\("' strace_chcon_recursive.log | grep -v '"/proc/self/fd/' || true) + if [ -n "$path_based_xattr" ]; then + echo "$path_based_xattr" + fail_immediately "chcon -R is using path-based SELinux xattr (TOCTOU; expected fd-anchored access, issue #11402)" + fi + # The relabel must actually reach SELinux through the anchored fd. Without + # this positive check, a chcon that aborts before the get/set (e.g. EBADF + # from f*filecon on an O_PATH fd) would still pass the assertions above. + if ! grep -qE '(get|set)xattr\("/proc/self/fd/|f(get|set)xattr\([0-9]+,' strace_chcon_recursive.log; then + cat strace_chcon_recursive.log + fail_immediately "chcon -R never reached an fd-anchored SELinux xattr op (relabel aborted before get/set?, issue #11402)" + fi + echo "✓ chcon -R anchors relabel to the traversal dirfd (O_NOFOLLOW, fd-based access)" + rm -rf chcon_tree +fi + # Test du - should use openat, newfstatat if echo "$AVAILABLE_UTILS" | grep -q "du"; then cp -r test_dir test_du