-
-
Notifications
You must be signed in to change notification settings - Fork 589
Fix ENOENT when resolving kernel pseudo-paths in ona_open #1054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
seks99x
wants to merge
5
commits into
RsyncProject:master
Choose a base branch
from
seks99x:seks99x-fd-link-fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
84a3b71
Fix ENOENT when resolving kernel pseudo-paths in ona_open
seks99x 4f4186d
Fix TOCTOU race condition and use fd-backed identity with dup()
seks99x 1bb3c5b
syscall: fix reading from /dev/fd and bash process substitution
seks99x 916db38
Refine FD duplication flag inheritance and restrict pseudo-paths to o…
seks99x ce157e4
Enforce strict access mode and status flag validation on inherited st…
seks99x File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,32 +145,38 @@ static const char *confinement_root(unsigned int *lenp) | |
|
|
||
| /* Split a recognised fd-pin prefix off `p`, returning the tail -- "" for the | ||
| * pin directory itself, otherwise a string starting with '/'. NULL when `p` | ||
| * is not in an fd-pin namespace. */ | ||
| static const char *fd_pin_tail(const char *p) | ||
| { | ||
| const char *s; | ||
|
|
||
| if (strncmp(p, "/dev/fd", 7) == 0) { | ||
| s = p + 7; | ||
| return (*s == '\0' || *s == '/') ? s : NULL; | ||
| } | ||
|
|
||
| if (strncmp(p, "/proc/", 6) != 0) | ||
| return NULL; | ||
| s = p + 6; | ||
| if (strncmp(s, "self/", 5) == 0) /* "/proc/self/..." */ | ||
| s += 4; | ||
| else { /* "/proc/<pid>/..." */ | ||
| const char *d = s; | ||
| while (*s >= '0' && *s <= '9') | ||
| s++; | ||
| if (s == d || *s != '/') | ||
| return NULL; | ||
| } | ||
| if (strncmp(s, "/fd", 3) != 0) | ||
| return NULL; | ||
| s += 3; | ||
| return (*s == '\0' || *s == '/') ? s : NULL; | ||
| * is not in an fd-pin namespace. | ||
| * If `strict_self` is non-zero, it strictly rejects /proc/<pid>/ formats. */ | ||
| static const char *fd_pin_tail(const char *p, int require_own_pid) | ||
| { | ||
| const char *s; | ||
|
|
||
| if (strncmp(p, "/dev/fd", 7) == 0) { | ||
| s = p + 7; | ||
| return (*s == '\0' || *s == '/') ? s : NULL; | ||
| } | ||
|
|
||
| if (strncmp(p, "/proc/", 6) != 0) | ||
| return NULL; | ||
| s = p + 6; | ||
|
|
||
| if (strncmp(s, "self/", 5) == 0) { /* "/proc/self/..." */ | ||
| s += 4; | ||
| } else { /* "/proc/<pid>/..." */ | ||
| const char *d = s; | ||
| while (*s >= '0' && *s <= '9') | ||
| s++; | ||
| if (s == d || *s != '/') | ||
| return NULL; | ||
|
|
||
| if (require_own_pid && atoi(d) != (int)getpid()) | ||
| return NULL; | ||
| } | ||
|
|
||
| if (strncmp(s, "/fd", 3) != 0) | ||
| return NULL; | ||
| s += 3; | ||
| return (*s == '\0' || *s == '/') ? s : NULL; | ||
| } | ||
|
|
||
| /* An EXACT pin entry, such as "/proc/self/fd/7" or "/dev/fd/7", whose target is | ||
|
|
@@ -180,7 +186,7 @@ static const char *fd_pin_tail(const char *p) | |
| * digits keeps a planted name like ".../fd/outside-secret" out. */ | ||
| static int is_exact_fd_pin(const char *p) | ||
| { | ||
| const char *tail = fd_pin_tail(p); | ||
| const char *tail = fd_pin_tail(p,0); | ||
|
|
||
| if (!tail || *tail != '/') | ||
| return 0; | ||
|
|
@@ -216,7 +222,7 @@ static int abspath_outside_confinement(const char *abspath) | |
| * resolve to an absolute path is refused, not waved through: an unreadable | ||
| * pin is exactly the case where we cannot say where the open would land. */ | ||
| if (!am_daemon) { | ||
| const char *tail = fd_pin_tail(abspath); | ||
| const char *tail = fd_pin_tail(abspath,0); | ||
| if (tail && !*tail) | ||
| return 0; /* the pin directory: transit, opens nothing */ | ||
| if (is_exact_fd_pin(abspath)) { | ||
|
|
@@ -319,8 +325,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz | |
| * (abspath_outside_confinement). A relative operator path starts at the | ||
| * daemon's cwd == the module root; an absolute one (or a followed absolute | ||
| * symlink target) restarts at "/". */ | ||
| char abspath[MAXPATHLEN]; | ||
| abspath[0] = '\0'; | ||
| char abspath[MAXPATHLEN] = {0}; | ||
| if (am_daemon && module_dir && module_dir[0] == '/') | ||
| strlcpy(abspath, module_dir, sizeof abspath); /* "/" for a path=/ module */ | ||
| else if (confine_root) { | ||
|
|
@@ -344,7 +349,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz | |
| * reach the magic link. This only suspends the check for that prefix: | ||
| * following the link restarts the walk at its absolute target, and every | ||
| * component of THAT is checked, so a pin aimed outside is still refused. */ | ||
| int pin_transit = !am_daemon && confine_root && fd_pin_tail(path) != NULL; | ||
| int pin_transit = !am_daemon && confine_root && fd_pin_tail(path,0) != NULL; | ||
|
|
||
| /* Path-walk state. `remaining` is the unconsumed tail; we splice | ||
| * symlink targets back into it as we go. Sized 2x MAXPATHLEN so a | ||
|
|
@@ -411,6 +416,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz | |
| } | ||
|
|
||
| if (S_ISLNK(lst.st_mode)) { | ||
|
|
||
| /* Symlink: untrusted owner is refused; trusted owner is followed | ||
| * via readlinkat + splice. In a user namespace the /proc/self and | ||
| * /dev/fd symlinks may report the overflow uid, so | ||
|
|
@@ -434,6 +440,85 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz | |
| } | ||
| target[n] = '\0'; | ||
|
|
||
| /* Detect Linux kernel pseudo-paths (pipes, sockets, anon_inodes). | ||
| * These are not real paths on disk and never contain slashes. */ | ||
| const char *ptail = fd_pin_tail(abspath,1); | ||
| int is_fd_dir = (ptail != NULL && *ptail == '\0'); | ||
|
|
||
| if (is_fd_dir && strchr(target, '/') == NULL && | ||
| (strncmp(target, "pipe:[", 6) == 0 || | ||
| strncmp(target, "socket:[", 8) == 0 || | ||
| strncmp(target, "anon_inode:", 11) == 0)) { | ||
|
|
||
| if (!is_last) { | ||
| /* Cannot traverse a pseudo-path like a directory */ | ||
| saved_errno = ENOTDIR; | ||
| goto out; | ||
| } | ||
|
|
||
| /* Verify comp is entirely numeric, it must be the FD number */ | ||
| int is_num = 1; | ||
| for (int i = 0; comp[i] != '\0'; i++) { | ||
| if (comp[i] < '0' || comp[i] > '9') { | ||
| is_num = 0; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (is_num) { | ||
|
|
||
| int fd_num = atoi(comp); | ||
|
|
||
|
|
||
| /* Get the actual state of the inherited file descriptor */ | ||
| int real_flags = fcntl(fd_num, F_GETFL); | ||
| if (real_flags < 0) { | ||
| saved_errno = EBADF; | ||
| goto out; | ||
| } | ||
|
|
||
| /* 1. Check Access Mode (Category 1) | ||
| * Reject if rsync wants to write to a read-only pipe, or vice versa. */ | ||
| if ((flags & O_ACCMODE) != (real_flags & O_ACCMODE)) { | ||
| saved_errno = EACCES; | ||
| goto out; | ||
| } | ||
|
|
||
| /* 2. Check Directory Constraint (Category 2) | ||
| * A pipe is not a directory. Reject traversing it. */ | ||
| if (flags & O_DIRECTORY) { | ||
| saved_errno = ENOTDIR; | ||
| goto out; | ||
| } | ||
|
|
||
| /* 3. Check Status Flags (Category 3) | ||
| * dup() shares the underlying struct file. If rsync demands O_APPEND, | ||
| * but the pipe wasn't opened with O_APPEND, dup() cannot fulfill this. | ||
| * (Note: O_CREAT and O_TRUNC are safely ignored per POSIX spec for FIFOs). */ | ||
| if ((flags & O_APPEND) && !(real_flags & O_APPEND)) { | ||
| saved_errno = EINVAL; | ||
| goto out; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still does not preserve the callers open() contract. A duplicated descriptor retains unrequested flags such as |
||
|
|
||
| /* Safely duplicate the descriptor, immune to TOCTOU symlink races */ | ||
| #ifdef F_DUPFD_CLOEXEC | ||
| retfd = fcntl(fd_num, F_DUPFD_CLOEXEC, 0); | ||
| #else | ||
| retfd = dup(fd_num); | ||
| #endif | ||
| saved_errno = (retfd >= 0) ? 0 : errno; | ||
| goto out; | ||
| } | ||
| else { | ||
| /* | ||
| * We are in a secure FD directory, but the component is NOT a number | ||
| * (e.g. /proc/self/fd/abc). This is an invalid descriptor reference. | ||
| * We treat it as a dead link and abort. | ||
| */ | ||
| saved_errno = ENOENT; | ||
| goto out; | ||
| } | ||
| } | ||
| /* Splice: new `remaining` = <target> + <tail-after-comp>. | ||
| * Absolute target restarts the walk from "/". */ | ||
| char tail[MAXPATHLEN]; | ||
|
|
@@ -461,7 +546,7 @@ static int ona_open(const char *path, int flags, mode_t mode, char *out_abs, siz | |
| /* "self" resolves to "<pid>", still inside the pin; | ||
| * the magic link itself lands elsewhere and ends the | ||
| * exemption. Never turns back on. */ | ||
| pin_transit = pin_transit && fd_pin_tail(rebuilt) != NULL; | ||
| pin_transit = pin_transit && fd_pin_tail(rebuilt,0) != NULL; | ||
| char *p = rebuilt; | ||
| while (*p == '/') p++; | ||
| strlcpy(remaining, p, sizeof remaining); | ||
|
|
@@ -3988,3 +4073,4 @@ int do_stat_atfd(int dfd, const char *name, STRUCT_STAT *st) | |
| return -1; | ||
| #endif | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| #!/usr/bin/env python3 | ||
| """Cross-PID file descriptor duplication must be rejected.""" | ||
|
|
||
| import os | ||
| import shutil | ||
| import sys | ||
| import subprocess | ||
| from pathlib import Path | ||
|
|
||
| from rsyncfns import SCRATCHDIR, makepath, rmtree, rsync_argv, test_fail, test_skipped | ||
|
|
||
| if not sys.platform.startswith('linux'): | ||
| test_skipped('This edge case and pipe:[] syntax are specific to Linux') | ||
|
|
||
| bash = shutil.which('bash') | ||
| if bash is None: | ||
| test_skipped('bash is unavailable, cannot test pseudo-paths') | ||
|
|
||
| # Verify the host actually supports process substitution and /dev/fd routing. | ||
| # If this fails, the OS likely does not support the /proc/self/fd mechanisms | ||
| # required to trigger this bug, so we skip the test. | ||
| probe = subprocess.run( | ||
| [bash, '-c', 'cat <(echo "probe")'], | ||
| capture_output=True | ||
| ) | ||
| if probe.returncode != 0: | ||
| test_skipped('bash process substitution is not supported on this system') | ||
|
|
||
| base = Path(SCRATCHDIR) / 'cross-pid-test' | ||
| src = base / 'src' | ||
| dest = base / 'dest' | ||
| makepath(src, dest) | ||
|
|
||
| # 1. Create files in the source directory | ||
| (src / 'EXCLUDE_ME_SECRET').write_text('secret data\n') | ||
| (src / 'keep_me.txt').write_text('normal data\n') | ||
|
|
||
| # 2. This is the secret data we will hide inside our internal FD 99 | ||
| secret_file = base / 'secret.txt' | ||
| secret_file.write_text('EXCLUDE_ME_SECRET\n') | ||
|
|
||
| # 3. Create a decoy process that maps a pipe to FD 99 and keeps it open. | ||
| victim_script = base / 'victim.py' | ||
| victim_script.write_text("""import os, time | ||
| r, w = os.pipe() | ||
| os.dup2(r, 99) | ||
| print("READY", flush=True) | ||
| time.sleep(10) | ||
| """) | ||
|
|
||
| # Launch the decoy process in the background | ||
| victim_proc = subprocess.Popen( | ||
| [sys.executable, str(victim_script)], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| pass_fds=() | ||
| ) | ||
| victim_proc.stdout.readline() # Wait for READY | ||
|
|
||
| # 4. Map the secret data to FD 99 in this Python process. | ||
| # When we launch rsync, it will inherit this internal FD. | ||
| secret_fd = os.open(str(secret_file), os.O_RDONLY) | ||
| if secret_fd != 99: | ||
| os.dup2(secret_fd, 99) | ||
| os.close(secret_fd) | ||
|
|
||
| # 5. Construct the cross-PID test path | ||
| target_path = f"/proc/{victim_proc.pid}/fd/99" | ||
|
|
||
| # We use --exclude-from to FORCE rsync to call ona_open() and read the file | ||
| cmd = rsync_argv('-a', f'--exclude-from={target_path}', str(src) + '/', str(dest) + '/') | ||
|
|
||
| try: | ||
| proc = subprocess.run( | ||
| cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| pass_fds=(99,) | ||
| ) | ||
| except subprocess.TimeoutExpired: | ||
| os.close(99) | ||
| victim_proc.terminate() | ||
| rmtree(base) | ||
| test_fail('rsync cross-PID test timed out') | ||
|
|
||
| # Clean up immediately | ||
| os.close(99) | ||
| victim_proc.terminate() | ||
| victim_proc.wait() | ||
|
|
||
| ctx = f'rc={proc.returncode}, stderr={proc.stderr.strip()!r}' | ||
|
|
||
| # VERIFICATION 1: Did rsync erroneously read the internal FD? | ||
| # If the file 'EXCLUDE_ME_SECRET' is MISSING in dest, it means rsync incorrectly | ||
| # bypassed path validation, read our internal FD 99, and applied the exclusion! | ||
| if not (dest / 'EXCLUDE_ME_SECRET').is_file() and proc.returncode == 0: | ||
| rmtree(base) | ||
| test_fail(f'BUG DETECTED: rsync erroneously duplicated its own FD via a cross-PID path! ({ctx})') | ||
|
|
||
| # VERIFICATION 2: Did it fail with the correct POSIX error? | ||
| # Because the patched fd_pin_tail returns NULL, rsync should fall back to | ||
| # normal symlink resolution, fail to open the pipe, and exit with an error. | ||
| if proc.returncode == 0: | ||
| rmtree(base) | ||
| test_fail(f'rsync unexpectedly succeeded on a cross-PID pseudo-path ({ctx})') | ||
|
|
||
| rmtree(base) | ||
| print('rsync successfully rejected cross-PID pseudo-path duplication') | ||
| raise SystemExit(0) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are sockets and anonymous inodes included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that vacation actually affected my focus in a bad way, but I'm back now!
I took a step back and used strace to map out exactly how rsync is handling this flags.
I initially tried to strictly validate
O_APPEND, but realized that doing so breaks legitimate use cases like--log-file=>(...). Bash process substitution creates pipes withoutO_APPEND(justO_WRONLY), while rsync explicitly requests it. If we enforce a strict match, it throws a silentEINVAL:By safely ignoring
O_APPENDduring the inheritance check, it clones the descriptor and then automatically corrects the flags:To handle the actual dangerous flags you mentioned (like
O_NONBLOCK), I implemented a strictSHARED_STATUS_FLAGSmacro. If the pipe possesses an unrequested dangerous state, it is safely rejected.Regarding
socket:[andanon_inode:[, they are whitelisted because they must be handled the exact same way viadup().3< /dev/tcp/...) resolves tosocket:[...].eventfdorepoll) resolve toanon_inode:[...]. (You can actually see this by running:python3 -c 'import select, os; ep = select.epoll(); print(os.readlink(f"/proc/self/fd/{ep.fileno()}"))').I’ve updated the test suite to include a edge case ensuring that an
O_RDWRsocket correctly satisfies anO_RDONLYrequest without being falsely blocked by strictO_ACCMODEequality.Let me know how this looks!