internal/as_user: drop supplementary groups - #2301
JasonColapietro wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: coreos/ignition/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (2)**Naming**: PascalCase exported, camelCase unexported, snake_case filenames📄 CodeRabbit inference engine (AGENTS.md) Files:
**Formatting**: `gofmt` enforced (CI and `./test`) **License header**: Required on all `.go` files (Apache 2.0, 13-line header) **Imports**: stdlib, blank line, project packages, blank line, external deps📄 CodeRabbit inference engine (AGENTS.md) Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthrough
ChangesPrivilege-drop hardening
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Binary size report (
|
| Size | |
|---|---|
Base (main) |
33MiB |
| PR (#2301) | 33MiB |
| Delta | -4.6KiB (-0.01%) |
yasminvalim
left a comment
yasminvalim
left a comment
There was a problem hiding this comment.
Hey, @JasonColapietro! Nice work!
I guess you just need to rebase and handle the conflicts, then we can run the workflow again :)
c6e73c9 to
27bbd35
Compare
set_eids() switched the effective gid and uid to the target user but never touched the supplementary group list, so the privilege-dropped thread kept the caller's group memberships while acting as that user. A directory reachable only through one of root's supplementary groups stayed reachable for the duration of the switch. Drop the list with setgroups(0, NULL) before relinquishing the gid and uid, while the thread still holds the privilege required to make that call, following the revocation order described in CERT POS36-C. Fixes coreos#2242 Signed-off-by: Jason Colapietro <55137770+JasonColapietro@users.noreply.github.com>
27bbd35 to
838740f
Compare
prestist
left a comment
prestist
left a comment
There was a problem hiding this comment.
@JasonColapietro Nice fix,lgtm.
One thought; we already have root-gated tests in internal/exec/util/user_group_lookup_test.go using the t.Skip("test requires root") pattern. Might be worth adding something similar here in internal/as_user/ to verify the groups actually get dropped. Like creating a dir that's only group-accessible (0070), then calling OpenFile as a user not in that group and checking it gets EACCES.
Not blocking, just wanted to know your thoughts?
Exercise OpenFile with a group-only directory and a directory owned by the target user. Require EACCES for access granted only by the caller's supplementary group, while preserving access owned by the target user. Set the caller's groups in a helper subprocess so the test runner's credentials remain unchanged, and check that OpenFile leaves the helper's own supplementary groups intact. Skip the test when not running as root. Signed-off-by: Jason Colapietro <jasoncola1@gmail.com>
Describe the access and caller-group guarantees of the root-gated test. Signed-off-by: Jason Colapietro <jasoncola1@gmail.com>
3 participants
Summary
set_eids()changes the effective gid and uid but leaves the caller'ssupplementary groups in place. The privilege-dropped thread can therefore
access files through root's group memberships while acting as another user.
Clear the supplementary group list before relinquishing gid and uid, while
setgroups()is still permitted. Return an error if clearing the list fails.The release note records this behavior change.
Regression coverage
Adds
TestOpenFileDropsSupplementaryGroups, using the existing root-gated testpattern suggested in review. It exercises the public
OpenFilefunction:root:4242, mode0070, rejects creation as uid/gid65534 even though the caller belongs to supplementary group 4242;
A helper subprocess receives the group-4242 credential before it starts, so the
test never changes the test runner's credentials. The test needs Linux, cgo,
and root, and skips when run without root. Numeric test IDs avoid NSS lookups.
Validation
because the group-only creation succeeds; the target-owned case still passes.
go test -count=1 -race -v ./internal/as_userpasses.go vet ./internal/as_userpasses../build butane && ./testpasses as a non-root Linux user, including82 passing package groups and the Ignition/Butane documentation checks.
git diff --checkandgofmtpass.Scope
Ignition already runs as root;
as_useris defense in depth rather than asecurity boundary. Its caller writes SSH authorized keys during first boot.
The change clears supplementary groups rather than looking up and installing
the target user's group memberships.
Fixes #2242.