fix(libc): correct privilege drop order and sentinel in js_os_exec#1487
Open
andreasrosdal wants to merge 1 commit into
Open
fix(libc): correct privilege drop order and sentinel in js_os_exec#1487andreasrosdal wants to merge 1 commit into
andreasrosdal wants to merge 1 commit into
Conversation
Two related bugs in os.exec({uid, gid}):
1. The drop ran setuid() before setgid()/setgroups(). After setuid()
to a non-root uid, the process no longer holds the capability
required for setgid()/setgroups() to succeed, so the gid drop
silently failed and the spawned program kept the parent's gid.
Reorder: groups, then gid, then uid.
2. The "was the option supplied" check used `uid == (uint32_t)-1`
and `gid == (uint32_t)-1` as the "unset" sentinel. The legal
POSIX value 0xFFFFFFFF collides with this sentinel, so passing
exec({uid: 0xFFFFFFFF}) silently skipped the drop. Replace with
explicit uid_set / gid_set bools.
No test: setuid()/setgid() require either root or capabilities and
behave differently across platforms (Linux vs. *BSD vs. macOS), so a
portable automated test isn't viable. Verified by reading the diff
against the Linux setuid(2) and setgid(2) man pages.
bnoordhuis
reviewed
May 17, 2026
Comment on lines
+3583
to
+3584
| uint32_t uid = 0, gid = 0; | ||
| bool uid_set = false, gid_set = false; |
Contributor
There was a problem hiding this comment.
You can leave uid and gid uninitialized, that way the compiler hopefully complains if we use them without initializing them first. Or does it give off false negatives?
The names uid_set and gid_set imply the action is already done. A better name is do_setuid or must_setuid or something along those lines.
Comment on lines
+3761
to
+3765
| /* Drop privileges in the correct order: supplementary groups and the | ||
| primary gid must change before setuid(), because setuid(non-root) | ||
| strips the capability needed for setgroups()/setgid() to succeed. | ||
| Track "was set" with explicit bools instead of a (uint32_t)-1 | ||
| sentinel, which collided with the legitimate value 0xFFFFFFFF. */ |
Contributor
There was a problem hiding this comment.
It's got that waffling LLM quality to it and I don't like seeing that in code I have to look at.
Suggested change
| /* Drop privileges in the correct order: supplementary groups and the | |
| primary gid must change before setuid(), because setuid(non-root) | |
| strips the capability needed for setgroups()/setgid() to succeed. | |
| Track "was set" with explicit bools instead of a (uint32_t)-1 | |
| sentinel, which collided with the legitimate value 0xFFFFFFFF. */ |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Two related bugs in os.exec({uid, gid}):
The drop ran setuid() before setgid()/setgroups(). After setuid() to a non-root uid, the process no longer holds the capability required for setgid()/setgroups() to succeed, so the gid drop silently failed and the spawned program kept the parent's gid. Reorder: groups, then gid, then uid.
The "was the option supplied" check used
uid == (uint32_t)-1andgid == (uint32_t)-1as the "unset" sentinel. The legal POSIX value 0xFFFFFFFF collides with this sentinel, so passing exec({uid: 0xFFFFFFFF}) silently skipped the drop. Replace with explicit uid_set / gid_set bools.No test: setuid()/setgid() require either root or capabilities and behave differently across platforms (Linux vs. *BSD vs. macOS), so a portable automated test isn't viable. Verified by reading the diff against the Linux setuid(2) and setgid(2) man pages.