From 0dd3867ec92b699e928d7b14ead4e74613492bda Mon Sep 17 00:00:00 2001 From: Anirudh Oppiliappan Date: Mon, 29 Jun 2026 10:58:29 +0000 Subject: [PATCH] knotserver/sandbox: don't set NoSetGroups to true; setgid bit on dirs Signed-off-by: Anirudh Oppiliappan --- knotserver/sandbox/repofs.go | 18 ++++++++++++------ knotserver/sandbox/repofs_test.go | 47 +++++++++++++++++++++++++++++++++++++++-------- knotserver/sandbox/sandbox_linux.go | 14 ++++++++++++-- knotserver/sandbox/sandbox_linux_test.go | 10 ++++++++-- 4 file(s) changed, 71 insertion(s)(+), 18 deletion(s)(-) diff --git a/knotserver/sandbox/repofs.go b/knotserver/sandbox/repofs.go --- a/knotserver/sandbox/repofs.go +++ b/knotserver/sandbox/repofs.go @@ -10,15 +10,21 @@ "syscall" ) -// ChmodRepoTree sets directory modes to 0770 and file modes to 0660 under -// root, preserving the executable bit on files (hook scripts need it). -// Symlinks are skipped since their mode is not meaningful. +// ChmodRepoTree sets directory modes to 2770 (with the setgid bit) and +// file modes to 0660 under root, preserving the executable bit on files +// (hook scripts need it). Symlinks are skipped since their mode is not +// meaningful. // // The group bits exist so the knot service (running as the git user, which // is in the git group that owns the repos) can still read and write the // repo via group permissions even though the repo's UID owner is a virtual -// UID. Sandbox subprocesses run with NoSetGroups: true so they don't gain -// group access and cross-owner isolation still holds. +// UID. Sandbox subprocesses drop supplementary groups so cross-owner +// isolation still holds. +// +// The setgid bit on directories makes new files and subdirectories created +// by sandbox subprocesses inherit the directory's group (the git group) +// rather than the subprocess's primary group (the virtual UID). Without +// it, sandbox-created files would be unreadable to the knot service. func ChmodRepoTree(root string) error { return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { if err != nil { @@ -28,7 +34,7 @@ return nil } if d.IsDir() { - return os.Chmod(path, 0770) + return os.Chmod(path, 0770|os.ModeSetgid) } info, err := d.Info() if err != nil { diff --git a/knotserver/sandbox/repofs_test.go b/knotserver/sandbox/repofs_test.go --- a/knotserver/sandbox/repofs_test.go +++ b/knotserver/sandbox/repofs_test.go @@ -29,13 +29,14 @@ cases := []struct { path string - wantMode os.FileMode + wantPerm os.FileMode + wantDir bool }{ - {root, 0770}, - {filepath.Join(root, "file.txt"), 0660}, - {filepath.Join(root, "script.sh"), 0770}, - {filepath.Join(root, "subdir"), 0770}, - {filepath.Join(root, "subdir", "nested.txt"), 0660}, + {root, 0770, true}, + {filepath.Join(root, "file.txt"), 0660, false}, + {filepath.Join(root, "script.sh"), 0770, false}, + {filepath.Join(root, "subdir"), 0770, true}, + {filepath.Join(root, "subdir", "nested.txt"), 0660, false}, } for _, c := range cases { info, err := os.Stat(c.path) @@ -43,8 +44,38 @@ t.Errorf("stat %s: %v", c.path, err) continue } - if got := info.Mode().Perm(); got != c.wantMode { - t.Errorf("%s: mode = %o, want %o", c.path, got, c.wantMode) + if got := info.Mode().Perm(); got != c.wantPerm { + t.Errorf("%s: perm = %o, want %o", c.path, got, c.wantPerm) + } + setgid := info.Mode()&os.ModeSetgid != 0 + if c.wantDir && !setgid { + t.Errorf("%s: setgid bit not set on directory", c.path) + } + if !c.wantDir && setgid { + t.Errorf("%s: setgid bit set on non-directory", c.path) + } + } +} + +func TestChmodRepoTree_SetsSetgidOnExistingDirs(t *testing.T) { + // Directories that already exist without the setgid bit should have it + // applied by the chmod walk; otherwise, files later created inside them + // by sandbox subprocesses would not inherit the directory's group. + root := t.TempDir() + mustMkdir(t, filepath.Join(root, "objects"), 0755) + mustMkdir(t, filepath.Join(root, "refs", "heads"), 0755) + + if err := ChmodRepoTree(root); err != nil { + t.Fatalf("ChmodRepoTree: %v", err) + } + + for _, p := range []string{root, filepath.Join(root, "objects"), filepath.Join(root, "refs"), filepath.Join(root, "refs", "heads")} { + info, err := os.Stat(p) + if err != nil { + t.Fatalf("stat %s: %v", p, err) + } + if info.Mode()&os.ModeSetgid == 0 { + t.Errorf("%s: setgid bit not set", p) } } } diff --git a/knotserver/sandbox/sandbox_linux.go b/knotserver/sandbox/sandbox_linux.go --- a/knotserver/sandbox/sandbox_linux.go +++ b/knotserver/sandbox/sandbox_linux.go @@ -60,17 +60,27 @@ wrapped.Stderr = cmd.Stderr // drop to the virtual UID if we can resolve one. the kernel handles - // fork -> setresuid -> chdir -> execve; requires CAP_SETUID/GID on the caller. + // fork -> setgroups -> setresgid -> setresuid -> chdir -> execve; + // requires CAP_SETUID/CAP_SETGID on the caller. // // the primary GID is intentionally set to the virtual UID, NOT the // repo's group ownership. repo dirs are owned by virtualUID:gitGroup // with mode 0770 so the knot service (in gitGroup) can read them, but // sandbox subprocesses must not inherit gitGroup or they would gain // group access to every other repo and lose cross-owner isolation. + // + // Groups is an empty (non-nil) slice and NoSetGroups is false so the + // kernel calls setgroups(0, NULL) and clears supplementary groups. + // NoSetGroups: true would skip setgroups entirely and the subprocess + // would inherit the parent's supplementary groups (including gitGroup). if l.lookup != nil { if uid, _, err := l.lookup(paths[0]); err == nil && uid > 0 { wrapped.SysProcAttr = &syscall.SysProcAttr{ - Credential: &syscall.Credential{Uid: uid, Gid: uid, NoSetGroups: true}, + Credential: &syscall.Credential{ + Uid: uid, + Gid: uid, + Groups: []uint32{}, + }, } } } diff --git a/knotserver/sandbox/sandbox_linux_test.go b/knotserver/sandbox/sandbox_linux_test.go --- a/knotserver/sandbox/sandbox_linux_test.go +++ b/knotserver/sandbox/sandbox_linux_test.go @@ -202,8 +202,14 @@ if cred.Gid != 100042 { t.Errorf("Credential.Gid = %d, want 100042 (must equal Uid, not lookup's gid 1234)", cred.Gid) } - if !cred.NoSetGroups { - t.Error("NoSetGroups should be true") + // NoSetGroups must be false (the default) so the kernel calls + // setgroups(0, NULL) and clears supplementary groups. NoSetGroups: true + // would let the subprocess inherit the parent's groups (gitGroup). + if cred.NoSetGroups { + t.Error("NoSetGroups must be false so supplementary groups get cleared") + } + if len(cred.Groups) != 0 { + t.Errorf("Groups = %v, want empty (no supplementary groups granted)", cred.Groups) } } -- tangled.sh