Monorepo for Tangled
knotserver/sandbox: don't set NoSetGroups to true; setgid bit on dirs
Signed-off-by: Anirudh Oppiliappan <anirudh@tangled.org>
Anirudh Oppiliappan
(Jun 29, 2026, 4:31 PM +0530)
0dd3867e
a75e0cb9
+71
-18
+12
-6
knotserver/sandbox/repofs.go
+12
-6
knotserver/sandbox/repofs.go
···
10
10
"syscall"
11
11
)
12
12
13
-
// ChmodRepoTree sets directory modes to 0770 and file modes to 0660 under
14
-
// root, preserving the executable bit on files (hook scripts need it).
15
-
// Symlinks are skipped since their mode is not meaningful.
13
+
// ChmodRepoTree sets directory modes to 2770 (with the setgid bit) and
14
+
// file modes to 0660 under root, preserving the executable bit on files
15
+
// (hook scripts need it). Symlinks are skipped since their mode is not
16
+
// meaningful.
16
17
//
17
18
// The group bits exist so the knot service (running as the git user, which
18
19
// is in the git group that owns the repos) can still read and write the
19
20
// repo via group permissions even though the repo's UID owner is a virtual
20
-
// UID. Sandbox subprocesses run with NoSetGroups: true so they don't gain
21
-
// group access and cross-owner isolation still holds.
21
+
// UID. Sandbox subprocesses drop supplementary groups so cross-owner
22
+
// isolation still holds.
23
+
//
24
+
// The setgid bit on directories makes new files and subdirectories created
25
+
// by sandbox subprocesses inherit the directory's group (the git group)
26
+
// rather than the subprocess's primary group (the virtual UID). Without
27
+
// it, sandbox-created files would be unreadable to the knot service.
22
28
func ChmodRepoTree(root string) error {
23
29
return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
24
30
if err != nil {
···
28
34
return nil
29
35
}
30
36
if d.IsDir() {
31
-
return os.Chmod(path, 0770)
37
+
return os.Chmod(path, 0770|os.ModeSetgid)
32
38
}
33
39
info, err := d.Info()
34
40
if err != nil {
+39
-8
knotserver/sandbox/repofs_test.go
+39
-8
knotserver/sandbox/repofs_test.go
···
29
29
30
30
cases := []struct {
31
31
path string
32
-
wantMode os.FileMode
32
+
wantPerm os.FileMode
33
+
wantDir bool
33
34
}{
34
-
{root, 0770},
35
-
{filepath.Join(root, "file.txt"), 0660},
36
-
{filepath.Join(root, "script.sh"), 0770},
37
-
{filepath.Join(root, "subdir"), 0770},
38
-
{filepath.Join(root, "subdir", "nested.txt"), 0660},
35
+
{root, 0770, true},
36
+
{filepath.Join(root, "file.txt"), 0660, false},
37
+
{filepath.Join(root, "script.sh"), 0770, false},
38
+
{filepath.Join(root, "subdir"), 0770, true},
39
+
{filepath.Join(root, "subdir", "nested.txt"), 0660, false},
39
40
}
40
41
for _, c := range cases {
41
42
info, err := os.Stat(c.path)
···
43
44
t.Errorf("stat %s: %v", c.path, err)
44
45
continue
45
46
}
46
-
if got := info.Mode().Perm(); got != c.wantMode {
47
-
t.Errorf("%s: mode = %o, want %o", c.path, got, c.wantMode)
47
+
if got := info.Mode().Perm(); got != c.wantPerm {
48
+
t.Errorf("%s: perm = %o, want %o", c.path, got, c.wantPerm)
49
+
}
50
+
setgid := info.Mode()&os.ModeSetgid != 0
51
+
if c.wantDir && !setgid {
52
+
t.Errorf("%s: setgid bit not set on directory", c.path)
53
+
}
54
+
if !c.wantDir && setgid {
55
+
t.Errorf("%s: setgid bit set on non-directory", c.path)
56
+
}
57
+
}
58
+
}
59
+
60
+
func TestChmodRepoTree_SetsSetgidOnExistingDirs(t *testing.T) {
61
+
// Directories that already exist without the setgid bit should have it
62
+
// applied by the chmod walk; otherwise, files later created inside them
63
+
// by sandbox subprocesses would not inherit the directory's group.
64
+
root := t.TempDir()
65
+
mustMkdir(t, filepath.Join(root, "objects"), 0755)
66
+
mustMkdir(t, filepath.Join(root, "refs", "heads"), 0755)
67
+
68
+
if err := ChmodRepoTree(root); err != nil {
69
+
t.Fatalf("ChmodRepoTree: %v", err)
70
+
}
71
+
72
+
for _, p := range []string{root, filepath.Join(root, "objects"), filepath.Join(root, "refs"), filepath.Join(root, "refs", "heads")} {
73
+
info, err := os.Stat(p)
74
+
if err != nil {
75
+
t.Fatalf("stat %s: %v", p, err)
76
+
}
77
+
if info.Mode()&os.ModeSetgid == 0 {
78
+
t.Errorf("%s: setgid bit not set", p)
48
79
}
49
80
}
50
81
}
+12
-2
knotserver/sandbox/sandbox_linux.go
+12
-2
knotserver/sandbox/sandbox_linux.go
···
60
60
wrapped.Stderr = cmd.Stderr
61
61
62
62
// drop to the virtual UID if we can resolve one. the kernel handles
63
-
// fork -> setresuid -> chdir -> execve; requires CAP_SETUID/GID on the caller.
63
+
// fork -> setgroups -> setresgid -> setresuid -> chdir -> execve;
64
+
// requires CAP_SETUID/CAP_SETGID on the caller.
64
65
//
65
66
// the primary GID is intentionally set to the virtual UID, NOT the
66
67
// repo's group ownership. repo dirs are owned by virtualUID:gitGroup
67
68
// with mode 0770 so the knot service (in gitGroup) can read them, but
68
69
// sandbox subprocesses must not inherit gitGroup or they would gain
69
70
// group access to every other repo and lose cross-owner isolation.
71
+
//
72
+
// Groups is an empty (non-nil) slice and NoSetGroups is false so the
73
+
// kernel calls setgroups(0, NULL) and clears supplementary groups.
74
+
// NoSetGroups: true would skip setgroups entirely and the subprocess
75
+
// would inherit the parent's supplementary groups (including gitGroup).
70
76
if l.lookup != nil {
71
77
if uid, _, err := l.lookup(paths[0]); err == nil && uid > 0 {
72
78
wrapped.SysProcAttr = &syscall.SysProcAttr{
73
-
Credential: &syscall.Credential{Uid: uid, Gid: uid, NoSetGroups: true},
79
+
Credential: &syscall.Credential{
80
+
Uid: uid,
81
+
Gid: uid,
82
+
Groups: []uint32{},
83
+
},
74
84
}
75
85
}
76
86
}
+8
-2
knotserver/sandbox/sandbox_linux_test.go
+8
-2
knotserver/sandbox/sandbox_linux_test.go
···
202
202
if cred.Gid != 100042 {
203
203
t.Errorf("Credential.Gid = %d, want 100042 (must equal Uid, not lookup's gid 1234)", cred.Gid)
204
204
}
205
-
if !cred.NoSetGroups {
206
-
t.Error("NoSetGroups should be true")
205
+
// NoSetGroups must be false (the default) so the kernel calls
206
+
// setgroups(0, NULL) and clears supplementary groups. NoSetGroups: true
207
+
// would let the subprocess inherit the parent's groups (gitGroup).
208
+
if cred.NoSetGroups {
209
+
t.Error("NoSetGroups must be false so supplementary groups get cleared")
210
+
}
211
+
if len(cred.Groups) != 0 {
212
+
t.Errorf("Groups = %v, want empty (no supplementary groups granted)", cred.Groups)
207
213
}
208
214
}
209
215