krz/gitbay

runner: a build workspace another user could have created first !252

merged merged by cmc on 2026-09-05 00:02 UTC · krz/gitbay:sonar-runner-workdir into main

2 files changed, +156 −2

Layout: unified · split

cmd/gitbay-runner/main.go +62 −2
@@ -20,6 +20,7 @@ import (
2020 "path/filepath"
2121 "strings"
2222 "sync"
23 "syscall"
2324 "time"
2425
2526 "gitbay.org/gitbay/internal/buildinfo"
@@ -53,7 +54,7 @@ func main() {
5354 remote = flag.String("remote", "git@gitbay.org", "ssh destination of the gitbay server")
5455 sshOpts = flag.String("ssh-opts", "", "extra ssh options, space-separated (also used for git clone)")
5556 cloneBase = flag.String("clone-base", "", "clone URL prefix (default ssh://<remote>)")
56 workdir = flag.String("workdir", filepath.Join(os.TempDir(), "gitbay-runner"), "build workspace root")
57 workdir = flag.String("workdir", defaultWorkdir(), "build workspace root")
5758 poll = flag.Duration("poll", 5*time.Second, "idle poll interval")
5859 timeout = flag.Duration("timeout", 30*time.Minute, "per-build time limit")
5960 repos = flag.String("repos", "", "only claim builds for these repositories, comma-separated owner/name (default: any)")
@@ -86,7 +87,13 @@ func main() {
8687 if r.cloneBase == "" {
8788 r.cloneBase = "ssh://" + *remote
8889 }
89 if err := os.MkdirAll(r.workdir, 0o755); err != nil {
90 // 0o700, not 0o755: a build's checkout and its secrets-bearing
91 // environment are this user's business alone, and the default sits
92 // beside other users' data on a shared host.
93 if err := os.MkdirAll(r.workdir, 0o700); err != nil {
94 log.Fatal(err)
95 }
96 if err := checkWorkdir(r.workdir); err != nil {
9097 log.Fatal(err)
9198 }
9299 n := *jobs
@@ -327,3 +334,56 @@ func (r *runner) ssh(stdin io.Reader, args ...string) (string, error) {
327334 }
328335 return out.String(), nil
329336}
337
338// defaultWorkdir picks a build workspace that another local user cannot
339// have created first.
340//
341// The default used to be <tmp>/gitbay-runner: a fixed name inside a
342// world-writable directory, created with MkdirAll, which succeeds against
343// an existing directory whoever owns it. On a shared host another user
344// could have made it — or symlinked it — before the runner started, and
345// this is the process that clones repositories and exports build secrets
346// into step environments (go:S5445, #153).
347//
348// The user's cache directory is not world-writable and is per-user by
349// construction. Falling back to tmp keeps a runner working where HOME is
350// unset, and checkWorkdir refuses the unsafe cases there.
351func defaultWorkdir() string {
352 if cache, err := os.UserCacheDir(); err == nil && cache != "" {
353 return filepath.Join(cache, "gitbay-runner")
354 }
355 return filepath.Join(os.TempDir(), "gitbay-runner")
356}
357
358// checkWorkdir makes sure the workspace is a directory this user owns
359// privately. MkdirAll is happy with one that already exists, so being
360// able to create it proves nothing about who made it.
361//
362// A directory we own that is merely too permissive is tightened rather
363// than refused: every runner before this one created its workspace 0755,
364// so refusing would take the runner down on upgrade to fix a permission
365// we are entitled to change. What cannot be repaired — a symlink, or
366// something owned by someone else — is refused, because those are what an
367// attacker leaves behind and neither is ours to correct.
368func checkWorkdir(dir string) error {
369 fi, err := os.Lstat(dir)
370 if err != nil {
371 return err
372 }
373 if fi.Mode()&os.ModeSymlink != 0 {
374 return fmt.Errorf("workdir %s is a symlink; point -workdir at a real directory", dir)
375 }
376 if !fi.IsDir() {
377 return fmt.Errorf("workdir %s is not a directory", dir)
378 }
379 if st, ok := fi.Sys().(*syscall.Stat_t); ok && int(st.Uid) != os.Getuid() {
380 return fmt.Errorf("workdir %s is owned by uid %d, not this process's %d", dir, st.Uid, os.Getuid())
381 }
382 if perm := fi.Mode().Perm(); perm&0o077 != 0 {
383 log.Printf("workdir %s was mode %04o; tightening to 0700 (builds and their secrets are this user's alone)", dir, perm)
384 if err := os.Chmod(dir, 0o700); err != nil {
385 return fmt.Errorf("tightening workdir %s: %w", dir, err)
386 }
387 }
388 return nil
389}
cmd/gitbay-runner/workdir_test.go added +94
@@ -0,0 +1,94 @@
1package main
2
3import (
4 "fmt"
5 "os"
6 "path/filepath"
7 "strings"
8 "testing"
9)
10
11// The default must not be a fixed name inside a world-writable directory:
12// another local user could create it first, and MkdirAll would accept
13// theirs. This is the process that clones repositories and exports build
14// secrets into step environments (go:S5445, #153).
15func TestDefaultWorkdirIsNotInSharedTmp(t *testing.T) {
16 got := defaultWorkdir()
17 cache, err := os.UserCacheDir()
18 if err != nil || cache == "" {
19 t.Skip("no user cache directory on this machine; the tmp fallback is the tested path")
20 }
21 if !strings.HasPrefix(got, cache) {
22 t.Errorf("default workdir %q is not under the user cache %q", got, cache)
23 }
24 if strings.HasPrefix(got, os.TempDir()+string(filepath.Separator)) {
25 t.Errorf("default workdir %q is still inside the shared temp directory", got)
26 }
27}
28
29func TestCheckWorkdirAcceptsAPrivateDirectory(t *testing.T) {
30 dir := filepath.Join(t.TempDir(), "work")
31 if err := os.MkdirAll(dir, 0o700); err != nil {
32 t.Fatal(err)
33 }
34 if err := checkWorkdir(dir); err != nil {
35 t.Fatalf("a private directory this user owns was refused: %v", err)
36 }
37}
38
39// A workspace we own that is merely too permissive is tightened, not
40// refused: every runner before this one made its workspace 0755, and
41// refusing would take the runner down on upgrade over a permission it is
42// entitled to change.
43func TestCheckWorkdirTightensOurOwnDirectory(t *testing.T) {
44 base := t.TempDir()
45 for _, mode := range []os.FileMode{0o755, 0o770, 0o777} {
46 dir := filepath.Join(base, fmt.Sprintf("mode-%o", mode))
47 if err := os.MkdirAll(dir, mode); err != nil {
48 t.Fatal(err)
49 }
50 if err := os.Chmod(dir, mode); err != nil { // MkdirAll applies umask
51 t.Fatal(err)
52 }
53 if err := checkWorkdir(dir); err != nil {
54 t.Fatalf("mode %04o: refused a directory we own: %v", mode, err)
55 }
56 fi, err := os.Stat(dir)
57 if err != nil {
58 t.Fatal(err)
59 }
60 if got := fi.Mode().Perm(); got != 0o700 {
61 t.Errorf("mode %04o was left at %04o, want 0700", mode, got)
62 }
63 }
64}
65
66// What cannot be repaired is refused: a symlink is not ours to correct,
67// and it is what an attacker leaves behind.
68func TestCheckWorkdirRefusesUnsafeDirectories(t *testing.T) {
69 base := t.TempDir()
70
71 target := filepath.Join(base, "elsewhere")
72 if err := os.MkdirAll(target, 0o700); err != nil {
73 t.Fatal(err)
74 }
75 link := filepath.Join(base, "link")
76 if err := os.Symlink(target, link); err != nil {
77 t.Skipf("symlinks unavailable: %v", err)
78 }
79 if err := checkWorkdir(link); err == nil {
80 t.Error("a symlinked workdir was accepted")
81 } else if !strings.Contains(err.Error(), "symlink") {
82 t.Errorf("symlink refusal does not say why: %v", err)
83 }
84}
85
86func TestCheckWorkdirRefusesAFile(t *testing.T) {
87 f := filepath.Join(t.TempDir(), "notadir")
88 if err := os.WriteFile(f, []byte("x"), 0o600); err != nil {
89 t.Fatal(err)
90 }
91 if err := checkWorkdir(f); err == nil {
92 t.Error("a regular file was accepted as a workdir")
93 }
94}