Skip to content

Commit 73587d1

Browse files
sylvestresylvestre
committed
mkfifo: drop path-based chmod to close TOCTOU race; switch to rustix (#10020)
(except on mac)
1 parent 6276737 commit 73587d1

4 files changed

Lines changed: 168 additions & 39 deletions

File tree

‎Cargo.lock‎

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/uu/mkfifo/Cargo.toml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ uucore = { workspace = true, features = ["fs", "mode"] }
2525
fluent = { workspace = true }
2626

2727
[target.'cfg(unix)'.dependencies]
28-
nix = { workspace = true, features = ["fs"] }
28+
rustix = { workspace = true, features = ["fs", "process"] }
29+
30+
[target.'cfg(target_vendor = "apple")'.dependencies]
31+
libc = { workspace = true }
2932

3033
[features]
3134
selinux = ["uucore/selinux"]

‎src/uu/mkfifo/src/mkfifo.rs‎

Lines changed: 57 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
// file that was distributed with this source code.
55

66
use clap::{Arg, ArgAction, Command, value_parser};
7-
use nix::sys::stat::Mode;
8-
use nix::unistd::mkfifo;
7+
use rustix::fs::Mode;
8+
use rustix::process::umask;
9+
#[cfg(any(feature = "selinux", feature = "smack"))]
910
use std::fs;
10-
use std::os::unix::fs::PermissionsExt;
1111
use uucore::display::Quotable;
1212
use uucore::error::{UResult, USimpleError};
1313
use uucore::translate;
@@ -48,47 +48,48 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
4848
};
4949

5050
for f in fifos {
51-
if mkfifo(f.as_str(), Mode::from_bits_truncate(0o666)).is_err() {
51+
// Clear umask around mkfifo so the kernel applies the exact
52+
// requested mode atomically. Skipping the path-based chmod
53+
// that used to follow this call closes the TOCTOU window an
54+
// attacker could use to swap the FIFO for a symlink between
55+
// mkfifo and chmod (issue #10020).
56+
let prev_umask = umask(Mode::empty());
57+
let mkfifo_result = create_fifo(f.as_str(), mode);
58+
umask(prev_umask);
59+
60+
if mkfifo_result.is_err() {
5261
show!(USimpleError::new(
5362
1,
5463
translate!("mkfifo-error-cannot-create-fifo", "path" => f.quote()),
5564
));
56-
continue;
57-
}
58-
59-
// Explicitly set the permissions to ignore umask
60-
if let Err(e) = fs::set_permissions(&f, fs::Permissions::from_mode(mode)) {
61-
return Err(USimpleError::new(
62-
1,
63-
translate!("mkfifo-error-cannot-set-permissions", "path" => f.quote(), "error" => e),
64-
));
65-
}
66-
67-
// Apply SELinux context if requested
68-
#[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
69-
{
70-
// Extract the SELinux related flags and options
71-
let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
72-
let context = matches.get_one::<String>(options::CONTEXT);
73-
74-
if set_security_context || context.is_some() {
75-
use std::path::Path;
76-
if let Err(e) =
77-
uucore::selinux::set_selinux_security_context(Path::new(&f), context)
78-
{
79-
let _ = fs::remove_file(f);
80-
return Err(USimpleError::new(1, e.to_string()));
65+
} else {
66+
// Apply SELinux context if requested
67+
#[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))]
68+
{
69+
let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
70+
let context = matches.get_one::<String>(options::CONTEXT);
71+
72+
if set_security_context || context.is_some() {
73+
use std::path::Path;
74+
if let Err(e) =
75+
uucore::selinux::set_selinux_security_context(Path::new(&f), context)
76+
{
77+
let _ = fs::remove_file(f);
78+
return Err(USimpleError::new(1, e.to_string()));
79+
}
8180
}
8281
}
83-
}
8482

85-
// Apply SMACK context if requested
86-
#[cfg(all(feature = "smack", target_os = "linux"))]
87-
{
88-
let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
89-
let context = matches.get_one::<String>(options::CONTEXT);
90-
if set_security_context || context.is_some() {
91-
uucore::smack::set_smack_label_and_cleanup(&f, context, |p| fs::remove_file(p))?;
83+
// Apply SMACK context if requested
84+
#[cfg(all(feature = "smack", target_os = "linux"))]
85+
{
86+
let set_security_context = matches.get_flag(options::SECURITY_CONTEXT);
87+
let context = matches.get_one::<String>(options::CONTEXT);
88+
if set_security_context || context.is_some() {
89+
uucore::smack::set_smack_label_and_cleanup(&f, context, |p| {
90+
fs::remove_file(p)
91+
})?;
92+
}
9293
}
9394
}
9495
}
@@ -133,6 +134,25 @@ pub fn uu_app() -> Command {
133134
)
134135
}
135136

137+
// `rustix::fs::mkfifoat` is unavailable on Apple targets, so fall back to
138+
// libc's path-based `mkfifo` there. Both rely on the caller having cleared
139+
// the umask so the requested mode is applied atomically (see issue #10020).
140+
#[cfg(not(target_vendor = "apple"))]
141+
fn create_fifo(path: &str, mode: u32) -> Result<(), ()> {
142+
use rustix::fs::{CWD, mkfifoat};
143+
mkfifoat(CWD, path, Mode::from_bits_truncate(mode)).map_err(|_| ())
144+
}
145+
146+
#[cfg(target_vendor = "apple")]
147+
fn create_fifo(path: &str, mode: u32) -> Result<(), ()> {
148+
use std::ffi::CString;
149+
let c_path = CString::new(path).map_err(|_| ())?;
150+
// SAFETY: `c_path` is a valid NUL-terminated C string and `mode` is a
151+
// standard mode_t bit pattern.
152+
let rc = unsafe { libc::mkfifo(c_path.as_ptr(), mode as libc::mode_t) };
153+
if rc == 0 { Ok(()) } else { Err(()) }
154+
}
155+
136156
fn calculate_mode(mode_option: Option<&String>) -> Result<u32, String> {
137157
let umask = uucore::mode::get_umask();
138158
let mode = 0o666; // Default mode for FIFOs

‎util/check-toctou.sh‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
#!/bin/bash
2+
#
3+
# TOCTOU (time-of-check / time-of-use) verification.
4+
#
5+
# These strace-based checks assert that utilities do not split a
6+
# security-sensitive operation across two path-based syscalls (e.g. a
7+
# stat() before open() that an attacker can race). The companion
8+
# script check-safe-traversal.sh covers a different concern: that
9+
# recursive walkers use the openat() family rather than re-resolving
10+
# multi-component paths during traversal.
11+
#
12+
13+
set -e
14+
15+
: ${PROFILE:=release-small}
16+
export PROFILE
17+
18+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
19+
PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
20+
TEMP_DIR=$(mktemp -d)
21+
22+
fail_immediately() {
23+
echo "❌ FAILED: $1"
24+
echo ""
25+
echo "Debug information available in: $TEMP_DIR/strace_*.log"
26+
exit 1
27+
}
28+
29+
cleanup() {
30+
rm -rf "$TEMP_DIR"
31+
}
32+
trap cleanup EXIT
33+
34+
echo "=== TOCTOU Verification ==="
35+
36+
if [ -f "$PROJECT_ROOT/target/${PROFILE}/coreutils" ]; then
37+
echo "Using multicall binary"
38+
USE_MULTICALL=1
39+
COREUTILS_BIN="$PROJECT_ROOT/target/${PROFILE}/coreutils"
40+
elif [ -f "$PROJECT_ROOT/target/${PROFILE}/mkfifo" ]; then
41+
echo "Using individual binaries"
42+
USE_MULTICALL=0
43+
else
44+
echo "Error: No binaries found. Build first with 'cargo build --profile=${PROFILE}'"
45+
exit 1
46+
fi
47+
48+
cd "$TEMP_DIR"
49+
50+
util_cmd() {
51+
if [ "$USE_MULTICALL" -eq 1 ]; then
52+
echo "$COREUTILS_BIN $1"
53+
else
54+
echo "$PROJECT_ROOT/target/${PROFILE}/$1"
55+
fi
56+
}
57+
58+
if [ "$USE_MULTICALL" -eq 1 ]; then
59+
AVAILABLE_UTILS=$($COREUTILS_BIN --list)
60+
else
61+
AVAILABLE_UTILS=""
62+
for util in mkfifo; do
63+
if [ -f "$PROJECT_ROOT/target/${PROFILE}/$util" ]; then
64+
AVAILABLE_UTILS="$AVAILABLE_UTILS $util"
65+
fi
66+
done
67+
fi
68+
69+
# mkfifo must not call a path-based chmod after creating the FIFO: the
70+
# second syscall would re-resolve the path and could be redirected by an
71+
# attacker who swaps the FIFO for a symlink in between (issue #10020).
72+
# After the fix, the kernel applies the requested mode atomically via
73+
# mkfifo with cleared umask.
74+
if echo "$AVAILABLE_UTILS" | grep -q "mkfifo"; then
75+
mkfifo_cmd=$(util_cmd mkfifo)
76+
rm -f test_fifo
77+
# mkfifo(3)/mkfifoat(3) are libc wrappers; the underlying syscall
78+
# is mknodat (or mknod on older kernels). Trace those plus any
79+
# chmod variants.
80+
strace -f -e trace=mknod,mknodat,chmod,fchmod,fchmodat,fchmodat2 \
81+
-o strace_mkfifo.log \
82+
$mkfifo_cmd -m 666 test_fifo 2>/dev/null || true
83+
84+
if [ ! -s strace_mkfifo.log ]; then
85+
fail_immediately "strace produced no output for mkfifo"
86+
fi
87+
if ! grep -qE 'mknodat?\(' strace_mkfifo.log; then
88+
cat strace_mkfifo.log
89+
fail_immediately "mkfifo must call mknod/mknodat to create the FIFO"
90+
fi
91+
92+
if grep -qE '\bchmod\([^,]*"test_fifo"' strace_mkfifo.log; then
93+
cat strace_mkfifo.log
94+
fail_immediately "mkfifo must not call path-based chmod after creation (issue #10020)"
95+
fi
96+
if grep -qE 'fchmodat2?\([^,]+, "test_fifo"' strace_mkfifo.log; then
97+
cat strace_mkfifo.log
98+
fail_immediately "mkfifo must not call fchmodat after creation (issue #10020)"
99+
fi
100+
echo "✓ mkfifo does not chmod after creation"
101+
rm -f test_fifo
102+
fi
103+
104+
echo ""
105+
echo "✓ TOCTOU verification completed"

0 commit comments

Comments
 (0)