From 838740f7962ca07f2749b9b5e080063c89f38420 Mon Sep 17 00:00:00 2001 From: Jason Colapietro <55137770+JasonColapietro@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:51:10 -0400 Subject: [PATCH 1/3] internal/as_user: drop supplementary groups 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 #2242 Signed-off-by: Jason Colapietro <55137770+JasonColapietro@users.noreply.github.com> --- docs/release-notes.md | 1 + internal/as_user/as_user.c | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/release-notes.md b/docs/release-notes.md index 95d3ad5a66..8a2280f00d 100644 --- a/docs/release-notes.md +++ b/docs/release-notes.md @@ -15,6 +15,7 @@ nav_order: 9 ### Bug fixes - Resolve intermediate symlinks in relabel paths, fixing SELinux relabeling failures for users with `home_dir` on OSTree platforms after policycoreutils 3.11 ([#2316](https://github.com/coreos/ignition/pull/2316)) +- Drop supplementary groups when dropping privileges to write files as a user ([#2242](https://github.com/coreos/ignition/issues/2242)) ## Ignition 2.27.0 (2026-08-26) diff --git a/internal/as_user/as_user.c b/internal/as_user/as_user.c index f84b464803..386f998ba7 100644 --- a/internal/as_user/as_user.c +++ b/internal/as_user/as_user.c @@ -15,6 +15,7 @@ #define _GNU_SOURCE #include #include +#include #include #include #include @@ -54,7 +55,9 @@ typedef struct au_thread_ctxt { int err; } au_thread_ctxt_t; -/* set_eids() sets the effective gid and uid of the calling thread to ids */ +/* set_eids() drops the supplementary groups and sets the effective gid and + * uid of the calling thread to ids + */ static int set_eids(au_ids_t *ids) { uid_t cu; gid_t cg; @@ -64,6 +67,14 @@ static int set_eids(au_ids_t *ids) { cu = geteuid(); cg = getegid(); + /* Drop the supplementary groups inherited from the caller before + * relinquishing the gid and uid, while we still have the privilege + * required to do so. Otherwise the thread would keep root's group + * memberships while acting as the target user. + */ + if(setgroups(0, NULL) == -1) + return -1; + if(cg != ids->gid && setregid(-1, ids->gid) == -1) return -1; From 8e404ef00b1a0aade5f6ee18643d33c9c81d3562 Mon Sep 17 00:00:00 2001 From: Jason Colapietro Date: Sun, 20 Sep 2026 04:49:42 -0400 Subject: [PATCH 2/3] internal/as_user: test supplementary group drop 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 --- internal/as_user/as_user_test.go | 109 +++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 internal/as_user/as_user_test.go diff --git a/internal/as_user/as_user_test.go b/internal/as_user/as_user_test.go new file mode 100644 index 0000000000..453fddd3c7 --- /dev/null +++ b/internal/as_user/as_user_test.go @@ -0,0 +1,109 @@ +// Copyright 2026 Jason Colapietro +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build linux && cgo + +package as_user_test + +import ( + "errors" + "os" + "os/exec" + "os/user" + "path/filepath" + "slices" + "syscall" + "testing" + + "github.com/coreos/ignition/v2/internal/as_user" +) + +func TestOpenFileDropsSupplementaryGroups(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("test requires root") + } + + const helperEnv = "IGNITION_TEST_SUPPLEMENTARY_GROUPS" + if os.Getenv(helperEnv) != "1" { + // Set the supplementary groups only in a subprocess so the test + // runner's credentials cannot be changed by this test. + cmd := exec.Command(os.Args[0], "-test.run=^TestOpenFileDropsSupplementaryGroups$", "-test.v") + cmd.Env = append(os.Environ(), helperEnv+"=1") + cmd.SysProcAttr = &syscall.SysProcAttr{ + Credential: &syscall.Credential{Uid: 0, Gid: 0, Groups: []uint32{4242}}, + } + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("subprocess failed: %v\n%s", err, output) + } + return + } + + // Unlike t.TempDir's parent directory, this directory can be traversed + // by the unprivileged user after its permissions are changed. + dir, err := os.MkdirTemp("", "ignition-as-user-") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := os.RemoveAll(dir); err != nil { + t.Error(err) + } + }) + if err := os.Chmod(dir, 0755); err != nil { + t.Fatal(err) + } + + u := &user.User{Uid: "65534", Gid: "65534"} + for _, test := range []struct { + name string + uid int + gid int + mode os.FileMode + want error + }{ + {"supplementary group only", 0, 4242, 0070, syscall.EACCES}, + {"target user owned", 65534, 65534, 0700, nil}, + } { + t.Run(test.name, func(t *testing.T) { + path := filepath.Join(dir, test.name) + if err := os.Mkdir(path, 0700); err != nil { + t.Fatal(err) + } + if err := os.Chown(path, test.uid, test.gid); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, test.mode); err != nil { + t.Fatal(err) + } + + f, err := as_user.OpenFile(u, filepath.Join(path, "file"), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0600) + if f != nil { + if err := f.Close(); err != nil { + t.Error(err) + } + } + if !errors.Is(err, test.want) { + t.Fatalf("OpenFile returned %v, want %v", err, test.want) + } + }) + } + + groups, err := syscall.Getgroups() + if err != nil { + t.Fatal(err) + } + if !slices.Equal(groups, []int{4242}) { + t.Fatalf("OpenFile changed the caller's supplementary groups: %v", groups) + } +} From 9c50dbfc58ab10435175e6b31378726e17b9df94 Mon Sep 17 00:00:00 2001 From: Jason Colapietro <55137770+JasonColapietro@users.noreply.github.com> Date: Sun, 20 Sep 2026 17:03:51 -0400 Subject: [PATCH 3/3] internal/as_user: document group regression test Describe the access and caller-group guarantees of the root-gated test. Signed-off-by: Jason Colapietro --- internal/as_user/as_user_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/as_user/as_user_test.go b/internal/as_user/as_user_test.go index 453fddd3c7..08319f7c18 100644 --- a/internal/as_user/as_user_test.go +++ b/internal/as_user/as_user_test.go @@ -29,6 +29,9 @@ import ( "github.com/coreos/ignition/v2/internal/as_user" ) +// TestOpenFileDropsSupplementaryGroups verifies that OpenFile drops inherited +// supplementary groups, preserves target-user access, and leaves the caller's +// groups unchanged. It requires root and isolates credentials in a subprocess. func TestOpenFileDropsSupplementaryGroups(t *testing.T) { if os.Geteuid() != 0 { t.Skip("test requires root")