mkfifo: drop path-based chmod to close TOCTOU race (#10020)

src/uu/mkfifo/Cargo.toml

@@ -25,7 +25,10 @@ uucore = { workspace = true, features = ["fs", "mode"] }
 fluent = { workspace = true }
 
 [target.'cfg(unix)'.dependencies]
-nix = { workspace = true, features = ["fs"] }
+rustix = { workspace = true, features = ["fs", "process"] }
+
+[target.'cfg(target_vendor = "apple")'.dependencies]
+libc = { workspace = true }
 
 [features]
 selinux = ["uucore/selinux"]

src/uu/mkfifo/src/mkfifo.rs

@@ -4,10 +4,8 @@
 // file that was distributed with this source code.
 
 use clap::{Arg, ArgAction, Command, value_parser};
-use nix::sys::stat::Mode;
-use nix::unistd::mkfifo;
-use std::fs;
-use std::os::unix::fs::PermissionsExt;
+use rustix::fs::Mode;
+use rustix::process::umask;
 use uucore::display::Quotable;
 use uucore::error::{UResult, USimpleError};
 use uucore::translate;
@@ -48,47 +46,48 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     };
 
     for f in fifos {
-        if mkfifo(f.as_str(), Mode::from_bits_truncate(0o666)).is_err() {
+        // Clear umask around mkfifo so the kernel applies the exact
+        // requested mode atomically. Skipping the path-based chmod
+        // that used to follow this call closes the TOCTOU window an
+        // attacker could use to swap the FIFO for a symlink between
+        // mkfifo and chmod (issue #10020).
+        let prev_umask = umask(Mode::empty());
+        let mkfifo_result = create_fifo(f.as_str(), mode);
+        umask(prev_umask);
+
+        if mkfifo_result.is_err() {
             show!(USimpleError::new(
                 1,
                 translate!("mkfifo-error-cannot-create-fifo", "path" => f.quote()),
             ));
-            continue;
-        }
-
-        // Explicitly set the permissions to ignore umask
-        if let Err(e) = fs::set_permissions(&f, fs::Permissions::from_mode(mode)) {
-            return Err(USimpleError::new(
-                1,
-                translate!("mkfifo-error-cannot-set-permissions", "path" => f.quote(), "error" => e),
-            ));
-        }
-
-        // Apply SELinux context if requested
-        #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
-        {
-            // Extract the SELinux related flags and options
-            let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
-            let context = matches.get_one::<String>(options::CONTEXT);
-
-            if set_security_context || context.is_some() {
-                use std::path::Path;
-                if let Err(e) =
-                    uucore::selinux::set_selinux_security_context(Path::new(&f), context)
-                {
-                    let _ = fs::remove_file(f);
-                    return Err(USimpleError::new(1, e.to_string()));
+        } else {
+            // Apply SELinux context if requested
+            #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
+            {
+                let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
+                let context = matches.get_one::<String>(options::CONTEXT);
+
+                if set_security_context || context.is_some() {
+                    use std::path::Path;
+                    if let Err(e) =
+                        uucore::selinux::set_selinux_security_context(Path::new(&f), context)
+                    {
+                        let _ = std::fs::remove_file(f);
+                        return Err(USimpleError::new(1, e.to_string()));
+                    }
                 }
             }
-        }
 
-        // Apply SMACK context if requested
-        #[cfg(all(feature = "smack", target_os = "linux"))]
-        {
-            let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
-            let context = matches.get_one::<String>(options::CONTEXT);
-            if set_security_context || context.is_some() {
-                uucore::smack::set_smack_label_and_cleanup(&f, context, |p| fs::remove_file(p))?;
+            // Apply SMACK context if requested
+            #[cfg(all(feature = "smack", target_os = "linux"))]
+            {
+                let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
+                let context = matches.get_one::<String>(options::CONTEXT);
+                if set_security_context || context.is_some() {
+                    uucore::smack::set_smack_label_and_cleanup(&f, context, |p| {
+                        std::fs::remove_file(p)
+                    })?;
+                }
             }
         }
     }
@@ -133,6 +132,25 @@ pub fn uu_app() -> Command {
         )
 }
 
+// `rustix::fs::mkfifoat` is unavailable on Apple targets, so fall back to
+// libc's path-based `mkfifo` there. Both rely on the caller having cleared
+// the umask so the requested mode is applied atomically (see issue #10020).
+#[cfg(not(target_vendor = "apple"))]
+fn create_fifo(path: &str, mode: u32) -> Result<(), ()> {
+    use rustix::fs::{CWD, mkfifoat};
+    mkfifoat(CWD, path, Mode::from_bits_truncate(mode)).map_err(|_| ())
+}
+
+#[cfg(target_vendor = "apple")]
+fn create_fifo(path: &str, mode: u32) -> Result<(), ()> {
+    use std::ffi::CString;
+    let c_path = CString::new(path).map_err(|_| ())?;
+    // SAFETY: `c_path` is a valid NUL-terminated C string and `mode` is a
+    // standard mode_t bit pattern.
+    let rc = unsafe { libc::mkfifo(c_path.as_ptr(), mode as libc::mode_t) };
+    if rc == 0 { Ok(()) } else { Err(()) }
+}
+
 fn calculate_mode(mode_option: Option<&String>) -> Result<u32, String> {
     let umask = uucore::mode::get_umask();
     let mode = 0o666; // Default mode for FIFOs

util/check-toctou.sh

@@ -0,0 +1,109 @@
+#!/bin/bash
+#
+# spell-checker:ignore mknod mknodat fchmod fchmodat mkfifoat strace
+#
+# TOCTOU (time-of-check / time-of-use) verification.
+#
+# These strace-based checks assert that utilities do not split a
+# security-sensitive operation across two path-based syscalls (e.g. a
+# stat() before open() that an attacker can race). The companion
+# script check-safe-traversal.sh covers a different concern: that
+# recursive walkers use the openat() family rather than re-resolving
+# multi-component paths during traversal.
+#
+
+set -e
+
+: ${PROFILE:=release-small}
+export PROFILE
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
+TEMP_DIR=$(mktemp -d)
+
+fail_immediately() {
+    echo "❌ FAILED: $1"
+    echo ""
+    echo "Debug information available in: $TEMP_DIR/strace_*.log"
+    exit 1
+}
+
+cleanup() {
+    rm -rf "$TEMP_DIR"
+}
+trap cleanup EXIT
+
+echo "=== TOCTOU Verification ==="
+
+if [ -f "$PROJECT_ROOT/target/${PROFILE}/coreutils" ]; then
+    echo "Using multicall binary"
+    USE_MULTICALL=1
+    COREUTILS_BIN="$PROJECT_ROOT/target/${PROFILE}/coreutils"
+elif [ -f "$PROJECT_ROOT/target/${PROFILE}/mkfifo" ]; then
+    echo "Using individual binaries"
+    USE_MULTICALL=0
+else
+    echo "Error: No binaries found. Build first with 'cargo build --profile=${PROFILE}'"
+    exit 1
+fi
+
+cd "$TEMP_DIR"
+
+util_cmd() {
+    if [ "$USE_MULTICALL" -eq 1 ]; then
+        echo "$COREUTILS_BIN $1"
+    else
+        echo "$PROJECT_ROOT/target/${PROFILE}/$1"
+    fi
+}
+
+if [ "$USE_MULTICALL" -eq 1 ]; then
+    AVAILABLE_UTILS=$($COREUTILS_BIN --list)
+else
+    AVAILABLE_UTILS=""
+    # The list intentionally holds a single util today; more will be added.
+    # shellcheck disable=SC2043
+    for util in mkfifo; do
+        if [ -f "$PROJECT_ROOT/target/${PROFILE}/$util" ]; then
+            AVAILABLE_UTILS="$AVAILABLE_UTILS $util"
+        fi
+    done
+fi
+
+# mkfifo must not call a path-based chmod after creating the FIFO: the
+# second syscall would re-resolve the path and could be redirected by an
+# attacker who swaps the FIFO for a symlink in between (issue #10020).
+# After the fix, the kernel applies the requested mode atomically via
+# mkfifo with cleared umask.
+if echo "$AVAILABLE_UTILS" | grep -q "mkfifo"; then
+    mkfifo_cmd=$(util_cmd mkfifo)
+    rm -f test_fifo
+    # mkfifo(3)/mkfifoat(3) are libc wrappers; the underlying syscall
+    # is mknodat (or mknod on older kernels). Trace those plus any
+    # chmod variants.
+    strace -f -e trace=mknod,mknodat,chmod,fchmod,fchmodat,fchmodat2 \
+        -o strace_mkfifo.log \
+        $mkfifo_cmd -m 666 test_fifo 2>/dev/null || true
+
+    if [ ! -s strace_mkfifo.log ]; then
+        fail_immediately "strace produced no output for mkfifo"
+    fi
+    if ! grep -qE 'mknodat?\(' strace_mkfifo.log; then
+        cat strace_mkfifo.log
+        fail_immediately "mkfifo must call mknod/mknodat to create the FIFO"
+    fi
+
+    if grep -qE '\bchmod\([^,]*"test_fifo"' strace_mkfifo.log; then
+        cat strace_mkfifo.log
+        fail_immediately "mkfifo must not call path-based chmod after creation (issue #10020)"
+    fi
+    if grep -qE 'fchmodat2?\([^,]+, "test_fifo"' strace_mkfifo.log; then
+        cat strace_mkfifo.log
+        fail_immediately "mkfifo must not call fchmodat after creation (issue #10020)"
+    fi
+    echo "✓ mkfifo does not chmod after creation"
+    rm -f test_fifo
+fi
+
+echo ""
+echo "✓ TOCTOU verification completed"