Repository navigation
No verification that setuid/setgid actually succeeded #32
Description
When the sandbox drops privileges before executing the child process, it calls setgid(group.gid) and then setuid(user.uid). The code propagates errors from these syscalls via .into_diagnostic()?, so if the kernel returns an error, the process will fail. However, there is no post-condition check that the effective uid and gid actually changed. Verifying the effective ids immediately after setuid/setgid would ensure that privileges were actually dropped before the child continues.
Source code:
- In crates/navigator-sandbox/src/process.rs, drop_privileges (lines 302-365) calls nix::unistd::setgid(group.gid).into_diagnostic()? (line 358) and nix::unistd::setuid(user.uid).into_diagnostic()? (line 362). The function then returns Ok(()) without calling nix::unistd::geteuid() or nix::unistd::getegid() to confirm the effective ids match the intended values.
- The same function is used from the process spawn path (e.g. lines 150-152 and 229-231 in the same file) and from the SSH shell handler in crates/navigator-sandbox/src/ssh.rs (line 554). None of the call sites perform a subsequent check that privileges were actually dropped.
Originally by @igurel on 2026-02-20T05:45:16.932-08:00
Activity
johntmyers commentedon Mar 5, 2026
Security Review
Determination: Legitimate concern
Summary
The
drop_privilegesfunction incrates/navigator-sandbox/src/process.rs(lines 352-416) callssetgid()andsetuid()and propagates errors via?, but does not verify the effective UID/GID after the calls. While the Linux kernel guarantees that a successfulsetuid()/setgid()changes the effective IDs, adding a post-condition check is a well-established defense-in-depth best practice (CWE-250) for security-critical privilege-dropping code.Severity Assessment
setuid()/setgid()success means the IDs changed. A failure would require a kernel bug. However, the cost of the check is negligible (two syscalls), making this a worthwhile hardening measure.crates/navigator-sandbox/src/process.rs—drop_privileges()(line 352), called frompre_execclosures at lines 180 and 269crates/navigator-sandbox/src/ssh.rs— line 1017, SSH shell handlerAttack Scenario
setuid()/setgid()to return success (0) without actually changing the effective UID/GID.Note: This scenario requires a kernel-level defect. No known current Linux kernel version exhibits this behavior. The value is purely defense-in-depth.
Remediation Plan
setgid()call on line 409, add a check: verifynix::unistd::getegid() == group.gid, and return an error if they don't match.setuid()call on line 412, add a check: verifynix::unistd::geteuid() == user.uid, and return an error if they don't match.setuid(0)fails after dropping privileges (a common hardening pattern to confirm root cannot be re-acquired).Additional Notes
?handles the primary failure mode (syscall returning an error).johntmyers commentedon Mar 5, 2026
Implementation Plan
Issue type:
fixComplexity: Low
Confidence: High
Summary
The
drop_privileges()function in the sandbox process module callssetgid()/setuid()and propagates kernel errors, but never verifies the effective UID/GID actually changed. This is a defense-in-depth hardening fix: after each syscall, verify the effective IDs match the intended values, and optionally confirm that root cannot be re-acquired. The change is confined to a single function (~10 lines added).Scope
crates/navigator-sandbox/src/process.rs: Add post-condition checks indrop_privileges()aftersetgid()andsetuid()calls. Verifygetegid() == group.gidandgeteuid() == user.uid. Optionally verifysetuid(Uid::from_raw(0))fails when user privileges were dropped (confirms root cannot be re-acquired).Implementation Steps
setgid()(after line 409): Callnix::unistd::getegid()and compare againstgroup.gid. Return a descriptive error if they don't match.setuid()(after line 412): Callnix::unistd::geteuid()and compare againstuser.uid. Return a descriptive error if they don't match.user_name.is_some()): Attemptnix::unistd::setuid(Uid::from_raw(0))and return an error if it succeeds — this confirms the process cannot escalate back to root.drop_privileges(): Test the happy path (current user/group passes verification), and test that the function returnsOk(())when norun_as_user/run_as_groupis configured (early return path). Note: full privilege-drop testing requires root, so the verification logic can be tested via the error-path structure.architecture/security-policy.md(around line 369) andarchitecture/sandbox.md(around line 956-959) mentioning the post-condition verification.Test Plan
crates/navigator-sandbox/src/process.rs(inline#[cfg(test)]module) verifying: (1)drop_privilegesreturnsOk(())when policy has norun_as_user/run_as_group(no-op path), (2)drop_privilegeswith current user/group succeeds and effective IDs match (non-root test environment). If running as non-root, test that specifying a different user returns the expected lookup error.pre_exec(post-fork, pre-exec) which is inherently difficult to integration-test without root. The existing sandbox E2E tests exercise this path implicitly.Risks & Open Questions
geteuid(),getegid(), andsetuid()are all async-signal-safe per POSIX, so they are safe to call inpre_execcontext. No risk here.setuid(0)guard is a well-known hardening pattern (CERT POS37-C). If the process was not running as root to begin with,setuid(0)will fail regardless — the check is meaningful only when dropping from root, which is the primary sandbox use case.#[cfg(unix)], andgeteuid/getegid/setuidare available on all Unix platforms vianix. No platform risk.Documentation Impact
architecture/security-policy.md(~line 369) to mention post-condition verification of effective UID/GID.architecture/sandbox.md(~line 956-959) in thedrop_privileges()description to note the verification steps.Revision 1 — initial plan
fix(sandbox): verify effective UID/GID after privilege drop
johntmyers commentedon Mar 5, 2026
Implementation Complete
PR: #132
What was built
Added defense-in-depth post-condition checks in
drop_privileges()to verify effective UID/GID aftersetgid()/setuid(), and a root re-acquisition guard (setuid(0)must fail). This hardens the sandbox privilege-drop path per CWE-250 and CERT POS37-C.Tests
drop_privileges_noop_when_no_user_or_group,drop_privileges_noop_when_empty_strings,drop_privileges_succeeds_for_current_user,drop_privileges_fails_for_nonexistent_user,drop_privileges_fails_for_nonexistent_group)Docs updated
architecture/security-policy.md— enforcement sequence updated with verification stepsarchitecture/sandbox.md—drop_privileges()description updatedThe issue will auto-close when the PR is merged.
fix(sandbox): verify effective UID/GID after privilege drop (#132)
fix(sandbox): verify effective UID/GID after privilege drop (#132)
fix: openclaw-nvidia rename, device pairing, default model (NVIDIA#32)
feat(helm): add isolation mode and inference injection values
Merge pull request NVIDIA#32 from EmilienM/rhel-9.8-migration