Commit `ca4d0e9517`

ca4d0e9517fe1b43351f0bec9c008b78f1fb28ea

parent: 6b5f1f02e9

Verified · cmc ci/build: success ci/sonar: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 23:12 UTC

runner: a build workspace another user could have created first

The default workdir was <tmp>/gitbay-runner: a fixed name inside a
world-writable directory, created with MkdirAll, which succeeds against a
directory whoever already owns it. On a shared host another local user
could make it — or symlink it — before the runner starts, and this is the
process that clones repositories and exports build secrets into step
environments.

bay1 was never exposed: its unit passes -workdir
/var/lib/gitbay-runner/work. The weakness is the default, which is what
anyone gets running the binary by hand.

The default is now under the user's cache directory, which is not
world-writable and is per-user by construction, and the workspace is
created 0700 rather than 0755.

checkWorkdir then verifies what MkdirAll cannot: being able to create a
directory says nothing about who made it. A symlink or a directory owned
by someone else is refused, since neither is ours to correct and both are
what an attacker leaves behind.

A directory we own that is merely too permissive is tightened instead of
refused. Every runner before this one created its workspace 0755, so
refusing would take the runner down on upgrade over a permission it is
entitled to change — a check that breaks the deploy it ships in is worse
than the hole it closes.

Ref #153

Layout: unified · split

cmd/gitbay-runner/main.go +62 −2

		@@ -20,6 +20,7 @@ import (
20	20	"path/filepath"
21	21	"strings"
22	22	"sync"
	23	"syscall"
23	24	"time"
24	25
25	26	"gitbay.org/gitbay/internal/buildinfo"
		@@ -53,7 +54,7 @@ func main() {
53	54	remote = flag.String("remote", "git@gitbay.org", "ssh destination of the gitbay server")
54	55	sshOpts = flag.String("ssh-opts", "", "extra ssh options, space-separated (also used for git clone)")
55	56	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")
57	58	poll = flag.Duration("poll", 5*time.Second, "idle poll interval")
58	59	timeout = flag.Duration("timeout", 30*time.Minute, "per-build time limit")
59	60	repos = flag.String("repos", "", "only claim builds for these repositories, comma-separated owner/name (default: any)")
		@@ -86,7 +87,13 @@ func main() {
86	87	if r.cloneBase == "" {
87	88	r.cloneBase = "ssh://" + *remote
88	89	}
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 {
90	97	log.Fatal(err)
91	98	}
92	99	n := *jobs
		@@ -327,3 +334,56 @@ func (r *runner) ssh(stdin io.Reader, args ...string) (string, error) {
327	334	}
328	335	return out.String(), nil
329	336	}
	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.
	351	func 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.
	368	func 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 @@
	1	package main
	2
	3	import (
	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).
	15	func 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
	29	func 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.
	43	func 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.
	68	func 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
	86	func 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	}

Commit ca4d0e9517

Commit `ca4d0e9517`