New functions/API for secure launching of external programs#1915
New functions/API for secure launching of external programs#1915Explorer09 wants to merge 6 commits into
Conversation
BenBE
added
needs-discussion 🤔
Explorer09
force-pushed
the
program-launcher
branch
from
f13b24b to
7d3eac6
Compare
Explorer09
force-pushed
the
program-launcher
branch
from
7d3eac6 to
380d1bc
Compare
Explorer09
force-pushed
the
program-launcher
branch
2 times, most recently
from
d918d6d to
36a2a7c
Compare
Explorer09
force-pushed
the
program-launcher
branch
3 times, most recently
from
027d0c7 to
b9ddd81
Compare
Explorer09
force-pushed
the
program-launcher
branch
3 times, most recently
from
be5f28c to
028acee
Compare
Explorer09
force-pushed
the
program-launcher
branch
3 times, most recently
from
53db71a to
5efab29
Compare
Explorer09
force-pushed
the
program-launcher
branch
3 times, most recently
from
4c72d75 to
2de4352
Compare
Explorer09
force-pushed
the
program-launcher
branch
from
2de4352 to
d70be19
Compare
Explorer09
force-pushed
the
program-launcher
branch
2 times, most recently
from
c54692f to
8a644b5
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughProgramLauncher Security API ImplementationThis PR introduces a new Core ImplementationThe implementation adds 482 lines across
The API uses an Integration PatternThe PR uniformly refactors four existing modules—
Build SystemUpdates to Implementation AssessmentThe solution is cleanly implemented with clear ownership of file-scope launcher instances and minimal intrusion into existing logic. The API design is straightforward and the integration pattern is consistent across all consumers. Core security logic is encapsulated in the module and tested by validation at execution time. The code properly handles platform-specific function availability and includes appropriate fallbacks. WalkthroughAdds a ProgramLauncher module that resolves executables (PATH search, shebang detection), validates trust (ownership, group/execute bits), manages privilege drop/restore, and executes via fexecve when available (execve fallback). Replaces direct execlp calls in OpenFilesScreen, TraceScreen, OpenRCMeter, and SystemdMeter. Updates configure.ac to probe for fexecve and group_member and Makefile.am to build/install the new ProgramLauncher sources/headers. Assessment against linked issues
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7662cd63-1c90-4724-8e2c-fd1e07fbf5cb
📒 Files selected for processing (8)
Makefile.amOpenFilesScreen.cProgramLauncher.cProgramLauncher.hTraceScreen.cconfigure.aclinux/OpenRCMeter.clinux/SystemdMeter.c
| ProgramLauncher_setPath(&OpenFiles_ProgramLauncher, "lsof"); | ||
| if (OpenFiles_ProgramLauncher.lastErrno != 0) { | ||
| pdata->error = 1; | ||
| return pdata; |
There was a problem hiding this comment.
Preserve the actionable lsof resolution error.
OpenFilesScreen_scan() only shows the specific PATH hint when pdata->error == 127. Returning 1 here turns ProgramLauncher_setPath() failures such as ENOENT into the generic "Failed listing open files." path.
| static bool ProgramLauncher_canTrustExecStat(const struct stat* sb) { | ||
| if (sb->st_uid == geteuid()) | ||
| return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXUSR); | ||
| if (sb->st_gid == getegid() || group_member(sb->st_gid)) | ||
| return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXGRP); | ||
|
|
||
| // To prevent users from executing programs they might not trust, | ||
| // ignore S_IXOTH except for programs owned by the root user. | ||
| // This is stricter than what OS would permit executing. | ||
| if (sb->st_uid == 0) | ||
| return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXOTH); | ||
|
|
||
| return false; |
There was a problem hiding this comment.
Elevated mode currently trusts non-root-owned executables via group permissions.
With PROGRAM_LAUNCH_KEEP_SETUID, Line 140/141 can still accept st_gid-matched g+x files even when st_uid != 0. That weakens the “trusted by root” requirement for elevated exec paths.
Suggested fix
-static bool ProgramLauncher_canTrustExecStat(const struct stat* sb) {
+static bool ProgramLauncher_canTrustExecStat(const struct stat* sb, ProgramLauncherOptions options) {
+ if (options & PROGRAM_LAUNCH_KEEP_SETUID)
+ return S_ISREG(sb->st_mode) && sb->st_uid == 0 && (sb->st_mode & S_IXUSR);
+
if (sb->st_uid == geteuid())
return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXUSR);
if (sb->st_gid == getegid() || group_member(sb->st_gid))
return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXGRP);
@@
- if (!ProgramLauncher_canTrustExecStat(&sb)) {
+ if (!ProgramLauncher_canTrustExecStat(&sb, this->options)) {
savedErrno = EACCES;
goto fail;
}Also applies to: 226-227
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 141-141: The function 'isNonnegative' is never used.
(unusedFunction)
[style] 146-146: The function 'isPositive' is never used.
(unusedFunction)
There was a problem hiding this comment.
False positive. Program associated with GID of which the effective user is a member is assumed to be trusted, and the point is irrelevant to the PROGRAM_LAUNCH_KEEP_SETUID option.
There was a problem hiding this comment.
@Explorer09, understood — the trust evaluation in ProgramLauncher_canTrustExecStat is based on credential ownership (euid, egid, supplementary groups), which is orthogonal to PROGRAM_LAUNCH_KEEP_SETUID. The latter controls privilege-dropping behavior at exec time, not what constitutes a trusted executable. The group-membership path (lines 140–141) is a valid and intentional trust criterion. I'll retract this comment.
✏️ Learnings added
Learnt from: Explorer09
Repo: htop-dev/htop PR: 1915
File: ProgramLauncher.c:137-149
Timestamp: 2026-05-17T10:03:07.557Z
Learning: In `ProgramLauncher.c` (htop project), `ProgramLauncher_canTrustExecStat()` uses three independent trust tiers: (1) file owned by euid → u+x required; (2) file's gid matches egid or a supplementary group → g+x required; (3) file owned by root → o+x required. This is intentional policy and is orthogonal to the `PROGRAM_LAUNCH_KEEP_SETUID` option, which only controls whether privileges are dropped before exec. Do not flag group-membership trust as a security issue related to elevated-mode execution.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Explorer09
force-pushed
the
program-launcher
branch
from
8a644b5 to
7961647
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ProgramLauncher.c (1)
137-150:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEnforce root-owned executable trust in KEEP_SETUID mode
Line 137 and Line 226 still allow non-root-owned executables when
PROGRAM_LAUNCH_KEEP_SETUIDis set. That violates the elevated-launch trust model.Proposed fix
-static bool ProgramLauncher_canTrustExecStat(const struct stat* sb) { +static bool ProgramLauncher_canTrustExecStat(const struct stat* sb, ProgramLauncherOptions options) { + if (options & PROGRAM_LAUNCH_KEEP_SETUID) + return S_ISREG(sb->st_mode) && sb->st_uid == 0 && (sb->st_mode & S_IXUSR); + if (sb->st_uid == geteuid()) return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXUSR); if (sb->st_gid == getegid() || group_member(sb->st_gid)) return S_ISREG(sb->st_mode) && (sb->st_mode & S_IXGRP); @@ - if (!ProgramLauncher_canTrustExecStat(&sb)) { + if (!ProgramLauncher_canTrustExecStat(&sb, this->options)) { savedErrno = EACCES; goto fail; }Also applies to: 226-229
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 568c65f9-0913-4b72-a397-d3c4bc346fcc
📒 Files selected for processing (8)
Makefile.amOpenFilesScreen.cProgramLauncher.cProgramLauncher.hTraceScreen.cconfigure.aclinux/OpenRCMeter.clinux/SystemdMeter.c
Explorer09
force-pushed
the
program-launcher
branch
from
7961647 to
fa07874
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
linux/OpenRCMeter.c (1)
39-46:⚠️ Potential issue | 🟠 Major | 💤 Low valueAdd
ProgramLauncher_done()call to clean up static programLauncher resource.The static
OpenRCMeter_programLaunchermanages a file reference for fexecve that requires explicit cleanup viaProgramLauncher_done(). The currentOpenRCMeter_done()implementation omits this call, causing the fileRef and its path to leak when the meter is destroyed.static void OpenRCMeter_done(ATTR_UNUSED Meter* this) { OpenRCMeterContext_t* ctx = String_eq(Meter_name(this), "OpenRCUser") ? &ctx_user : &ctx_system; free(ctx->runlevel); ctx->runlevel = NULL; ProgramLauncher_done(&OpenRCMeter_programLauncher); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0edbef1a-0228-4815-9c9c-91c796e1b653
📒 Files selected for processing (8)
Makefile.amOpenFilesScreen.cProgramLauncher.cProgramLauncher.hTraceScreen.cconfigure.aclinux/OpenRCMeter.clinux/SystemdMeter.c
| void ProgramLauncher_done(ProgramLauncher* this) { | ||
| if (this->fileRef) { | ||
| free(((const ProgramFileRef*)this->fileRef)->path); | ||
| } | ||
| free(this->fileRef); | ||
| this->fileRef = NULL; | ||
| this->lastErrno = 0; | ||
| } |
There was a problem hiding this comment.
Do not store derived file-type state in the public options bitmask.
PROGRAM_LAUNCH_NO_SCRIPT is a caller-visible policy flag, but Line 321 repurposes it as cache state for the currently resolved file. After ProgramLauncher_done() or any launcher reuse, a previously resolved binary permanently changes how later paths are validated, even if the caller never requested “no scripts”. Keep the cached “known non-script” state in private data instead of mutating options.
Also applies to: 321-322
🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 36-36: The function 'ProgramLauncher_done' is never used.
(unusedFunction)
There was a problem hiding this comment.
@coderabbitai What's the real issue of mutating options?
There was a problem hiding this comment.
@Explorer09 There are three concrete problems:
-
_done()doesn't clearoptions.ProgramLauncher_done()resetsfileRefandlastErrnobut leavesoptionsuntouched. After cleanup and reuse,PROGRAM_LAUNCH_NO_SCRIPTfrom a prior resolution persists — even though no file has been resolved yet and the caller never requested that policy. -
One bit, two meanings. As a caller flag,
PROGRAM_LAUNCH_NO_SCRIPTmeans "I want to forbid scripts." As internal cache, it means "the resolved file is known to be a binary." These are semantically different states sharing the same bit. Code that readsoptionsto apply policy cannot distinguish between them. -
Cross-path contamination on reuse. If the same launcher is pointed at a new executable via
setPath, the bit set during the previous resolution carries over. The shebang check for the new file is skipped based on stale state from an unrelated file.
The fix is straightforward: track the "is binary" hint in a private field (e.g., inside ProgramFileRef or a separate private bool), and keep PROGRAM_LAUNCH_NO_SCRIPT in options as pure caller policy.
There was a problem hiding this comment.
Not sure if this change is worth it. For now, there're no use case in htop codebase where a ProgramLauncher object will be reused for different executables.
@coderabbitai Do not treat this reply as a definite answer. This question can be left open for other human developers to discuss.
There was a problem hiding this comment.
@Explorer09 Acknowledged — the absence of a reuse case today is a fair counter-argument against the added complexity right now.
For the record, the concern is primarily a defensive API design one: the ProgramLauncher struct is part of the public header, its fields are directly accessible, and the _done() / _setPath() lifecycle implies reuse is a supported pattern. If a future caller does reuse a launcher for a second executable, the stale PROGRAM_LAUNCH_NO_SCRIPT bit would silently skip the shebang check with no warning. Moving the "known binary" hint into ProgramFileRef (private/opaque data) would eliminate that class of latent bug at minimal cost — essentially one bool field.
Leaving this open for the team to weigh in. If the consensus is that launchers are always single-use and the options field can be treated as partially internal state, that should ideally be documented in the header.
This module introduces function interfaces for checking permissions of external programs and launching them with dropped privileges whenever it is appliable. These functions are intended to replace the naive execlp(3) and execvp(3) calls as the latter can launch potentially malicious programs without checking, when given root privileges. This module contains these APIs: * ProgramLauncher_setPath() * ProgramLauncher_execve() Features: * It can drop set-UID (SUID) privileges before launching a program. * It checks the owner ID, group ID, and the permission (mode) bits of the program. The policy of executing is stricter than what the OS would permit execution. In particular it respects the "other execute" (o+x) bit only if the program is owned by the root user. * It uses fexecve(3) rather than execve(2) whenever it is supported by the OS. fexecve() is safe against data race. (Unfortunately macOS and OpenBSD don't yet support fexecve() at the time of writing.) * It can search the program through PATH variable and cache the result. It caches the inode as well as the full program path. If the program executable is quietly swapped during htop runtime, htop will refuse to launch the program until the htop instance ends. Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Signed-off-by: Kang-Che Sung <explorer09@gmil.com>
Explorer09
force-pushed
the
program-launcher
branch
from
fa07874 to
4cd17d5
Compare
2 participants
This feature branch implements an interface for checking an external program's permission and launching a program with dropped privileges when it's applicable.
I wrote this as a sepeate module, tentatively named ProgramLauncher (and I welcome suggestions for better names). It has these features:
o+x) bit is now respected only of the program is owned by root. (Theu+xandg+xbehaviors remain the same, but the code now checks for UID and GID explicitly so that root user won't bypass it.)PATHenvironment variable, the "ProgramLauncher" API can cache the result - keeping the full program path and the inode in the filesystem. If someone quietly swaps the program while an htop instance is running, that htop instance will stop trusting or running the external program (until htop restarts).Included in the branch are changes to lsof (OpenFileScreen), strace/truss (TraceScreen), systemctl (SystemdMeter) and rc-status (OpenRCMeter) to all launch using the new framework. These are examples of how the new module/APIs may be called.
Resolves #1844.
Note that this conflicts with #1893. I have no plans to change the
fork()andfexecve()calls to useposix_spawn()becauseposix_spawn()can't achieve the security I want (I needposix_fspawn()instead, but no libc had implemented such interface for me).