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; diff --git a/internal/as_user/as_user_test.go b/internal/as_user/as_user_test.go new file mode 100644 index 0000000000..08319f7c18 --- /dev/null +++ b/internal/as_user/as_user_test.go @@ -0,0 +1,112 @@ +// 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" +) + +// 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") + } + + 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) + } +}