Restrict htoprc symlink resolution with owner check#1947
Conversation
Explorer09
force-pushed
the
settings-symlink
branch
6 times, most recently
from
4043a07 to
7c38b17
Compare
Explorer09
force-pushed
the
settings-symlink
branch
3 times, most recently
from
5b96667 to
c465f9c
Compare
Explorer09
force-pushed
the
settings-symlink
branch
3 times, most recently
from
708d953 to
168431a
Compare
Explorer09
force-pushed
the
settings-symlink
branch
3 times, most recently
from
d59a397 to
a32f17e
Compare
Explorer09
force-pushed
the
settings-symlink
branch
from
a32f17e to
f2168a1
Compare
📝 WalkthroughImplementationAdds security restriction on symlink resolution for htoprc configuration files by introducing a static Code QualityThe implementation is well-structured with portable flag handling ( Commit StructureSingle focused commit with clear security-oriented message and proper sign-off. Directly integrates the new function into Security AssessmentDirectly addresses symlink attack vectors by enforcing owner verification before resolution. Allows read-only access to untrusted symlink paths themselves while preventing redirection to unintended targets. Complements existing file writability checks ( WalkthroughSettings.c adds Settings_resolveSymlink, which opens a path without following symlinks using Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Explorer09
force-pushed
the
settings-symlink
branch
from
f2168a1 to
ee7832c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Settings.c (1)
844-852:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the path separator when rebuilding relative symlink targets.
Line 847 computes the prefix length without the trailing
/, so a link like/home/u/.config/htop/htoprc -> ../shared/htoprcis rebuilt as/home/u/.config/htop../shared/htoprc. That makesrealpath()fail and breaks valid trusted relative symlinks.Proposed fix
size_t dirnameLen = 0; if (buf[0] != '/') { const char *lastSlash = strrchr(path, '/'); - dirnameLen = lastSlash ? (size_t)(lastSlash - (const char*)path) : 0; + dirnameLen = lastSlash ? (size_t)(lastSlash - (const char*)path) + 1 : 0; } char* intermediatePath = xMalloc(dirnameLen + (size_t)len + 1); memcpy(intermediatePath, path, dirnameLen); memcpy(intermediatePath + dirnameLen, buf, (size_t)len + 1);
Explorer09
force-pushed
the
settings-symlink
branch
from
ee7832c to
c0df87f
Compare
Restrict the possibility of a symlink attack on the htoprc file. If the htoprc file to be read is a symlink, only resolve the link if it's owned by the same EUID or root user (UID 0). Signed-off-by: Kang-Che Sung <explorer09@gmail.com>
Explorer09
force-pushed
the
settings-symlink
branch
from
c0df87f to
1935d54
Compare
| #ifdef O_PATH | ||
| // O_PATH is specific to Linux and FreeBSD. | ||
| openFlags |= O_PATH; | ||
| #endif | ||
| do { | ||
| fd = open(path, openFlags); | ||
| } while (fd < 0 && errno == EINTR); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect supported targets and existing feature checks related to O_PATH/readlinkat.
# Expectation: every supported target either supports opening symlinks by fd plus readlinkat(fd, ""),
# or the PR adds an intentional platform-specific fallback/guard.
set -euo pipefail
printf '\n== Build/platform references ==\n'
rg -n -C3 '\b(linux|freebsd|openbsd|netbsd|darwin|macos|solaris|bsd)\b' \
-g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' || true
printf '\n== Feature checks and direct usage ==\n'
rg -n -C4 '\b(O_PATH|O_EXEC|O_NOFOLLOW|readlinkat|realpath|lstat|readlink)\b' \
-g '!**/build/**' -g '!**/dist/**' -g '!**/node_modules/**' || true
printf '\n== Candidate build/config files ==\n'
fd -a 'configure|configure.ac|meson.build|CMakeLists.txt|Makefile|ci|workflow|Dockerfile|README|INSTALL' . || trueRepository: htop-dev/htop
Length of output: 50369
🏁 Script executed:
cat -n Settings.c | sed -n '800,870p'Repository: htop-dev/htop
Length of output: 2414
🏁 Script executed:
rg -n -A 5 -B 5 'readlinkat|O_PATH|O_EXEC|O_NOFOLLOW' Settings.cRepository: htop-dev/htop
Length of output: 1345
🏁 Script executed:
rg -n '`#ifdef`|`#if` defined' Settings.c | head -30Repository: htop-dev/htop
Length of output: 369
🏁 Script executed:
# Search for any readlinkat-related comments or conditionals that may indicate platform issues
rg -n 'readlinkat|empty.?path' --type-list | head -20Repository: htop-dev/htop
Length of output: 447
🏁 Script executed:
# Check if there are any FreeBSD, OpenBSD, NetBSD, Darwin, or Solaris specific workarounds
rg -B5 -A5 'noPath|readlinkat' Settings.cRepository: htop-dev/htop
Length of output: 1073
🏁 Script executed:
# Check build/configure for readlinkat checks
rg -n 'readlinkat|AC_CHECK' configure.ac | head -20Repository: htop-dev/htop
Length of output: 1087
🌐 Web query:
readlinkat empty path "" support OpenBSD NetBSD Darwin Solaris
💡 Result:
Support for passing an empty string ("") as the path argument to readlinkat varies significantly across operating systems. On Linux, readlinkat explicitly supports an empty string path argument, provided that the AT_EMPTY_PATH flag is used or the kernel version is 2.6.39 or later [1][2][3][4]. In this specific case, the call operates directly on the symbolic link referred to by the file descriptor (dirfd), which must have been opened appropriately (typically with O_PATH and O_NOFOLLOW) [1][3]. Conversely, on OpenBSD, NetBSD, Darwin (macOS), and according to the POSIX standards, passing an empty string as a path argument to readlinkat is not supported and will result in an error [5][6][7][8][9]. POSIX specifically defines that readlinkat shall fail with ENOENT if the path argument is an empty string [9]. Similarly, these BSD-derived systems do not implement the Linux-specific AT_EMPTY_PATH extension, meaning standard path resolution rules apply where an empty string is treated as an invalid path name [5][6][10]. Summary: - Linux: Supported (requires specific flags/kernel version) [1][2][3]. - OpenBSD, NetBSD, Darwin, POSIX: Not supported (returns error, typically ENOENT) [5][6][7][8][9].
Citations:
- 1: https://manpages.debian.org/unstable/manpages-dev/readlinkat.2.en.html
- 2: https://man.archlinux.org/man/readlinkat.2.en
- 3: https://man7.org/linux/man-pages/man2/readlink.2.html
- 4: https://manpages.ubuntu.com/manpages/stonking/man2/readlinkat.2.html
- 5: https://man.openbsd.org/readlinkat.2
- 6: https://man.netbsd.org/readlinkat.2
- 7: https://pubs.opengroup.org/onlinepubs/9699919799.orig/functions/readlinkat.html
- 8: https://pubs.opengroup.org/onlinepubs/9799919799/functions/readlinkat.html
- 9: https://pubs.opengroup.org/onlinepubs/9699919799/functions/readlinkat.html
- 10: https://man.freebsd.org/cgi/man.cgi?query=readlink&sektion=2&apropos=0&manpath=FreeBSD+13.1-RELEASE+and+Ports
Symlink resolution will fail silently on FreeBSD, OpenBSD, NetBSD, and Darwin.
The readlinkat(fd, "") call at line 840 is Linux-specific and unsupported on FreeBSD, OpenBSD, NetBSD, Darwin, and Solaris—it will return ENOENT. When this fails, the function falls back to returning an empty string (line 866), which breaks trusted symlink config resolution on those platforms without any diagnostic. Add #ifdef HAVE_DECL_READLINKAT_EMPTY_PATH or platform-specific guards to fall back to an alternative resolution method (e.g., readlink() if symlink was already opened with O_PATH) on non-Linux systems, or document this as a known limitation.
Evidence
Line 840 uses readlinkat(fd, "", buf, PATH_MAX) unconditionally. According to POSIX and BSD implementations, an empty string path argument is invalid and causes ENOENT. This is a Linux-specific extension not available on FreeBSD, OpenBSD, NetBSD, Darwin, or Solaris.
1 participant
Restrict the possibility of a symlink attack on the htoprc file. If the htoprc file to be read is a symlink, only resolve the link if it's owned by the same EUID or root user (UID 0).