-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
base: master
Are you sure you want to change the base?
Changes from all commits
84a3b71
4f4186d
1bb3c5b
916db38
ce157e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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,92 @@ 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. Access Mode (Category 1) | ||
| * Reject if rsync wants to write to a read-only pipe, or read | ||
| * from a write-only pipe. (Note: O_RDWR sockets satisfy both). */ | ||
| int req_mode = flags & O_ACCMODE; | ||
| int real_mode = real_flags & O_ACCMODE; | ||
| if (real_mode != O_RDWR && real_mode != req_mode) { | ||
| 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 Dangerous Status Flags (Category 3) | ||
| * dup() shares the underlying struct file. We must prevent rsync from | ||
| * inheriting dangerous unrequested states (like O_NONBLOCK or O_ASYNC) | ||
| * which could cause fatal interrupts or read/write failures. | ||
| * | ||
| * Note: O_APPEND is deliberately excluded. Bash process substitution >(...) | ||
| * creates pipes without O_APPEND. Excluding it allows rsync's subsystems | ||
| * (like --log-file) to safely inherit the pipe and apply O_APPEND manually | ||
| * via fcntl() later. O_CREAT and O_TRUNC are also safely ignored. */ | ||
| if ((flags & (SHARED_STATUS_FLAGS)) != (real_flags & (SHARED_STATUS_FLAGS))) { | ||
| saved_errno = EINVAL; | ||
| goto out; | ||
| } | ||
| /* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This avoids silently inheriting The updated
Member
Author
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. 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. 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. |
||
| #else | ||
| retfd = dup(fd_num); | ||
| #endif | ||
| saved_errno = (retfd >= 0) ? 0 : errno; | ||
| goto out; | ||
|
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 return still happens before the Could we apply the confinement decision before returning and add an in-band merge regression? A command-line |
||
| } | ||
| 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 +553,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 +4080,4 @@ int do_stat_atfd(int dfd, const char *name, STRUCT_STAT *st) | |
| return -1; | ||
| #endif | ||
| } | ||
|
|
||
| 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) |
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!