Repository navigation
fix(orch): keep root-executed helpers unwritable under the world-writable /usr/local - #3439
tomassrnka wants to merge 2 commits into
Conversation
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit 42b5793. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
22894ce to
fd3a1ef
Compare
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit fd3a1ef. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any additional bugs, but this PR changes the permission and ownership model for root-executed boot helpers (envd's ExecStartPre chain) across every sandbox template's finalize phase, so it's worth a human look given the security-sensitive nature of the change and the fact that Bugbot already caught and required a fix for an ancestor-chain sticky-bit gap during this same review cycle.
What was reviewed: the sticky-bit/ownership restoration logic in configure.sh covering the full /usr/local ancestor chain and the helper binaries, and the ssl-certs.tar hardening added in configure.go. Candidate issues examined and ruled out this run: symlink-following in the chown/chmod restore loops, a pre-planted symlink at the ssl-certs.tar path being followed by tar/chown/chmod, and the sticky bit blocking legitimate sudo-less reinstall/removal of files in /usr/local/bin.
Extended reasoning...
Overview
This PR modifies packages/orchestrator/pkg/template/build/phases/finalize/configure.sh and configure.go, the guest-side script run as root during template finalize. It adds sticky bits to /usr/local and its subdirectories (which are chmod -R 777'd earlier in the same script to allow sudo-less installs) and restores root ownership/non-writable modes on the helper files that envd executes as root at boot (e2b-seed-certs, e2b-provision-runner, envd.openrc, distro.env, ssl-certs.tar). The intent is defense-in-depth: prevent a sandbox user (who already has NOPASSWD sudo) from swapping the content of files root executes at boot, without breaking sudo-less installs into /usr/local.
Security risks
This is inherently a file-permission hardening change to a boot-time root-execution path that runs inside every sandbox template. The main risk categories are: (1) incompleteness — missing a directory or file in the ancestor chain that still allows a swap (this already happened once during review: Bugbot found that sticky bits on only the leaf directories could be bypassed by renaming a non-sticky parent, and the author fixed it by sticky-bitting the whole chain with KVM verification across four distro families), and (2) regressions to legitimate sudo-less installs into /usr/local (verified by the author not to be broken). No injection, auth-bypass, or data-exposure vectors are introduced; this is a hardening measure on top of an already-privileged execution context (the sandboxed user already has full root via sudo).
Level of scrutiny
Permission and ownership handling for root-executed code is squarely in the security-sensitive category, and it applies to the shared finalize path for all templates, so I think a human maintainer should confirm the approach even though the bug-hunting system found nothing further and the author already responded thoroughly to Bugbot's finding with real hardware verification across ubuntu/fedora/arch/alpine plus a fromTemplate rebuild.
Other factors
The author's own KVM testing (before/after probes showing the swap succeeding then failing, and ExecStartPre still exiting 0) is strong practical validation, but the failing integration tests reported by Codecov on this PR's earlier commit (TestDeleteTemplate, TestTemplateBuildFuseConfiguration, TestCommandKillNextApp, all failing at the finalize step with a generic internal error) are unresolved in the timeline. Given TestTemplateBuildRUN shows a known ~13-18% flake rate on main, these may be pre-existing CI flakiness rather than caused by this change, but that hasn't been confirmed either way, which is another reason for a human to check CI status before merging.
fd3a1ef to
e9db1cb
Compare
1 participant
finalize/configure.shrunschmod -R 777 /usr/localso sandboxes can install there without sudo, which also left the helpers root executes at boot (e2b-seed-certs,envd.openrc,ssl-certs.tar,distro.env) replaceable by any user. Finalize now sticky-bits/usr/localand the three subdirectories holding them and restores root-owned755/644on the files — sticky keeps sudo-less installs working while limiting unlink/rename to the owner. The whole ancestor chain needs it: a sticky directory under a non-sticky world-writable parent can be renamed aside wholesale (thanks Bugbot).Defense in depth, not an escalation fix — the sandbox user has
NOPASSWD: ALLby design; this stops root from executing files anything in the sandbox could have swapped. Verified on real KVM per family plus afromTemplatechild: envd'sExecStartPrestill exits 0 (including a live envd restart), and asuseran install into/usr/local/binsucceeds while replacinge2b-seed-certsfails.