From 1f4107c16f20a8e40a35437698f52c72f0998074 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Tue, 27 Oct 2026 11:47:21 -0400 Subject: [PATCH 1/5] feat(privileged-helper): create and serve its own socket with --socket `rshell privileged-helper` only accepted a systemd-activated socket (fd 3 with LISTEN_PID and LISTEN_FDS=1). An embedding application that starts the helper itself had to fake socket activation, moving the descriptor to fd 3 and setting LISTEN_PID through a shell trampoline. With --socket=PATH the helper creates the socket itself, after loading its policy and account and before dropping privileges, so the socket exists only while a ready helper serves it: - owned by --user and its primary group, mode 0600, created under a 0177 umask so nobody can connect before it is handed over; - its directory must be owned by the helper's effective UID (root) and not writable by group or others, since anyone able to write it could replace the socket with their own listener; - an existing socket at PATH is replaced, any other file is not; - it is removed when the helper exits. Without --socket, the systemd-activated path is unchanged. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- cmd/rshell/privileged_helper_linux.go | 85 +++++++++++++++++++++- cmd/rshell/privileged_helper_linux_test.go | 75 +++++++++++++++++++ docs/PRIVILEGED_HELPER.md | 17 ++++- 3 files changed, 170 insertions(+), 7 deletions(-) diff --git a/cmd/rshell/privileged_helper_linux.go b/cmd/rshell/privileged_helper_linux.go index 94429e0eb..53a243e27 100644 --- a/cmd/rshell/privileged_helper_linux.go +++ b/cmd/rshell/privileged_helper_linux.go @@ -19,6 +19,7 @@ import ( "os" "os/exec" "os/user" + "path/filepath" "runtime" "slices" "sort" @@ -45,6 +46,7 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i flags.StringVar(&policyPath, "credential", policyPath, "deprecated alias for --policy") idleTimeout := flags.Duration("idle-timeout", 30*time.Second, "exit after this long without a connection") userName := flags.String("user", "dd-agent", "unprivileged effective user") + socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; its directory must be owned by root and not writable by others") if err := flags.Parse(args); err != nil { return 2 } @@ -71,10 +73,21 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i fmt.Fprintf(stderr, "resolving credentials for %s: %v\n", *userName, err) return 1 } - listener, err := inheritedListener() - if err != nil { - fmt.Fprintf(stderr, "loading systemd socket: %v\n", err) - return 1 + var listener net.Listener + if *socketPath != "" { + // Created last, once the helper is ready to serve, so that the socket + // existing means a working helper is behind it. + listener, err = createListener(*socketPath, credentials.uid, credentials.primaryGID) + if err != nil { + fmt.Fprintf(stderr, "creating socket: %v\n", err) + return 1 + } + } else { + listener, err = inheritedListener() + if err != nil { + fmt.Fprintf(stderr, "loading systemd socket: %v\n", err) + return 1 + } } defer listener.Close() if err := setProcessGroups(credentials.primaryGID, credentials.supplementaryGIDs); err != nil { @@ -252,6 +265,70 @@ func inheritedListener() (net.Listener, error) { return listener, nil } +// createListener creates the helper socket at path, owned by uid:gid with mode +// 0600, while the helper still runs as root. The socket is the only access +// control in front of the helper, so its directory must be owned by the +// helper's own effective UID (root in production) and not writable by group +// or others: anyone who could write it could replace the socket with one they +// listen on. A stale socket is replaced; any other existing file is not. +// The socket is removed when the listener closes. +func createListener(path string, uid, gid int) (*net.UnixListener, error) { + if !filepath.IsAbs(path) { + return nil, fmt.Errorf("socket path %q is not absolute", path) + } + if err := requireTrustedDirectory(filepath.Dir(path)); err != nil { + return nil, err + } + info, err := os.Lstat(path) + switch { + case err == nil && info.Mode()&os.ModeSocket == 0: + return nil, fmt.Errorf("%s exists and is not a socket", path) + case err == nil: + if err := os.Remove(path); err != nil { + return nil, fmt.Errorf("remove stale socket: %w", err) + } + case !errors.Is(err, os.ErrNotExist): + return nil, fmt.Errorf("inspect %s: %w", path, err) + } + // The socket is created owner-only, so nobody can connect before it is + // handed to uid:gid. + oldUmask := syscall.Umask(0o177) + listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + syscall.Umask(oldUmask) + if err != nil { + return nil, fmt.Errorf("listen on %s: %w", path, err) + } + if err := os.Lchown(path, uid, gid); err != nil { + listener.Close() + return nil, fmt.Errorf("chown %s: %w", path, err) + } + listener.SetUnlinkOnClose(true) + return listener, nil +} + +// requireTrustedDirectory reports an error unless dir is a directory owned by +// the current effective UID that group and others cannot write. +func requireTrustedDirectory(dir string) error { + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("inspect socket directory: %w", err) + } + if !info.IsDir() { + return fmt.Errorf("socket directory %s is not a directory", dir) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("socket directory %s has no ownership information", dir) + } + if int(stat.Uid) != os.Geteuid() { + return fmt.Errorf("socket directory %s is owned by uid %d, not %d", dir, stat.Uid, os.Geteuid()) + } + if info.Mode().Perm()&0o022 != 0 { + return fmt.Errorf("socket directory %s is writable by group or others (mode %#o)", dir, info.Mode().Perm()) + } + return nil +} + type workerCommandFactory func(context.Context) (*exec.Cmd, error) type helperExecutor struct { diff --git a/cmd/rshell/privileged_helper_linux_test.go b/cmd/rshell/privileged_helper_linux_test.go index d9f50768f..6ac1fe7aa 100644 --- a/cmd/rshell/privileged_helper_linux_test.go +++ b/cmd/rshell/privileged_helper_linux_test.go @@ -8,8 +8,10 @@ package main import ( + "net" "os" "path/filepath" + "syscall" "testing" "github.com/stretchr/testify/require" @@ -29,3 +31,76 @@ func TestLoadOptionalPolicyEmpty(t *testing.T) { require.NoError(t, err) require.Nil(t, credential) } + +// shortTempDir returns an owner-only temporary directory with a path short +// enough for a Unix socket (about 100 bytes). +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(dir) }) + require.NoError(t, os.Chmod(dir, 0o755)) + return dir +} + +func TestCreateListener(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + + listener, err := createListener(path, os.Getuid(), os.Getgid()) + require.NoError(t, err) + + var stat syscall.Stat_t + require.NoError(t, syscall.Lstat(path, &stat)) + require.Equal(t, uint32(syscall.S_IFSOCK), stat.Mode&syscall.S_IFMT) + require.Equal(t, uint32(0o600), stat.Mode&0o777, "only the owner may connect") + require.Equal(t, uint32(os.Getuid()), stat.Uid) + + conn, err := net.Dial("unix", path) + require.NoError(t, err) + require.NoError(t, conn.Close()) + + require.NoError(t, listener.Close()) + _, err = os.Lstat(path) + require.ErrorIs(t, err, os.ErrNotExist, "the socket is removed when the helper stops") +} + +func TestCreateListenerReplacesStaleSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + stale.SetUnlinkOnClose(false) + require.NoError(t, stale.Close()) + + listener, err := createListener(path, os.Getuid(), os.Getgid()) + require.NoError(t, err) + require.NoError(t, listener.Close()) +} + +func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { + t.Run("relative path", func(t *testing.T) { + _, err := createListener("h.sock", os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "not absolute") + }) + t.Run("existing non-socket file", func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + require.NoError(t, os.WriteFile(path, []byte("keep"), 0o600)) + _, err := createListener(path, os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "is not a socket") + content, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, "keep", string(content), "a non-socket file must not be removed") + }) + t.Run("directory writable by others", func(t *testing.T) { + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o777)) + _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "writable by group or others") + }) + t.Run("directory is a symlink", func(t *testing.T) { + parent := shortTempDir(t) + link := filepath.Join(parent, "link") + require.NoError(t, os.Symlink(shortTempDir(t), link)) + _, err := createListener(filepath.Join(link, "h.sock"), os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "is not a directory") + }) +} diff --git a/docs/PRIVILEGED_HELPER.md b/docs/PRIVILEGED_HELPER.md index 0731a67f4..e2241c553 100644 --- a/docs/PRIVILEGED_HELPER.md +++ b/docs/PRIVILEGED_HELPER.md @@ -1,8 +1,19 @@ # Privileged helper -The `rshell privileged-helper` mode is the Linux-only, socket-activated -execution boundary for selectively elevated Private Action Runner tasks. It is -not enabled by the normal rshell CLI. +The `rshell privileged-helper` mode is the Linux-only execution boundary for +selectively elevated Private Action Runner tasks. It is not enabled by the +normal rshell CLI. + +The helper serves one Unix socket, which only the `dd-agent` account can +connect to. It either inherits it from systemd socket activation (fd 3 with +`LISTEN_PID` and `LISTEN_FDS=1`), or, with `--socket=PATH`, creates it itself +once its policy and account are loaded, before dropping privileges: owned by +`--user` with mode `0600`, and removed when the helper exits. The directory +holding `PATH` must be owned by root and not writable by group or others, and +an existing file at `PATH` is only replaced if it is a socket. With `--socket`, +the socket exists only while a ready helper serves it. Pass +`--idle-timeout=0` to keep it serving until it is stopped, since nothing would +restart it on demand. The helper must start with real UID 0. It loads its trust policy, resolves the `dd-agent` UID and complete group set, permanently switches its real, effective, From 6e02cbc8081fb98dcf42e1753e5bbee93dc04ef0 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Tue, 27 Oct 2026 12:48:56 -0400 Subject: [PATCH 2/5] fix(privileged-helper): address --socket review findings - Do not unlink the socket on close: by then the helper's effective UID is --user, which cannot write the root-owned directory, so the unlink failed silently and the docs promised cleanup that never happened. Removing the socket is the supervisor's job (e.g. RuntimeDirectory=). - With --socket, default --idle-timeout to 0 and reject a non-zero value: nothing restarts a helper that owns its socket on demand. - Check every ancestor directory, not only the parent: each must be owned by root (the effective UID) and not group/other writable, or sticky above the socket's own directory. Otherwise anyone able to rename an entry on the way could swap in their own socket. - Refuse to replace a socket that still accepts connections; only an ECONNREFUSED socket is stale. - Add a root integration test that runs the helper with --socket and --user=nobody: socket owner and mode, the effective UID after the drop, refusal of a served socket, and replacement of a stale one. Add flag-handling and ancestor tests, and correct the docs. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- cmd/rshell/privileged_helper_linux.go | 110 +++++++++++++----- cmd/rshell/privileged_helper_linux_test.go | 56 ++++++++- ...privileged_helper_root_integration_test.go | 80 +++++++++++++ docs/PRIVILEGED_HELPER.md | 22 +++- 4 files changed, 228 insertions(+), 40 deletions(-) diff --git a/cmd/rshell/privileged_helper_linux.go b/cmd/rshell/privileged_helper_linux.go index 53a243e27..a93656975 100644 --- a/cmd/rshell/privileged_helper_linux.go +++ b/cmd/rshell/privileged_helper_linux.go @@ -46,7 +46,7 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i flags.StringVar(&policyPath, "credential", policyPath, "deprecated alias for --policy") idleTimeout := flags.Duration("idle-timeout", 30*time.Second, "exit after this long without a connection") userName := flags.String("user", "dd-agent", "unprivileged effective user") - socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; its directory must be owned by root and not writable by others") + socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; every directory above it must be owned by root and not writable by others unless sticky; implies --idle-timeout=0") if err := flags.Parse(args); err != nil { return 2 } @@ -54,6 +54,18 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i fmt.Fprintln(stderr, "privileged-helper does not accept positional arguments") return 2 } + if *socketPath != "" { + // Nothing restarts a helper that owns its socket on the next + // connection, as systemd socket activation does, so it must not exit + // when idle. + idleTimeoutSet := false + flags.Visit(func(f *flag.Flag) { idleTimeoutSet = idleTimeoutSet || f.Name == "idle-timeout" }) + if idleTimeoutSet && *idleTimeout != 0 { + fmt.Fprintln(stderr, "privileged-helper --socket requires --idle-timeout=0") + return 2 + } + *idleTimeout = 0 + } if os.Getuid() != 0 { fmt.Fprintln(stderr, "privileged-helper requires real uid 0") return 1 @@ -267,28 +279,24 @@ func inheritedListener() (net.Listener, error) { // createListener creates the helper socket at path, owned by uid:gid with mode // 0600, while the helper still runs as root. The socket is the only access -// control in front of the helper, so its directory must be owned by the -// helper's own effective UID (root in production) and not writable by group -// or others: anyone who could write it could replace the socket with one they -// listen on. A stale socket is replaced; any other existing file is not. -// The socket is removed when the listener closes. +// control in front of the helper, so every directory above it must be one +// that only root (the helper's effective UID) can change: anyone else able to +// rename an entry on the way could swap in a socket they listen on. A stale +// socket is replaced; a socket that is still being served, or any other file, +// is not. +// +// The socket is not removed when the helper exits: by then the helper runs as +// --user, which cannot write the directory. Removing it is the supervisor's +// job, for example with systemd's RuntimeDirectory=. func createListener(path string, uid, gid int) (*net.UnixListener, error) { if !filepath.IsAbs(path) { return nil, fmt.Errorf("socket path %q is not absolute", path) } - if err := requireTrustedDirectory(filepath.Dir(path)); err != nil { + if err := requireTrustedAncestors(filepath.Dir(filepath.Clean(path))); err != nil { return nil, err } - info, err := os.Lstat(path) - switch { - case err == nil && info.Mode()&os.ModeSocket == 0: - return nil, fmt.Errorf("%s exists and is not a socket", path) - case err == nil: - if err := os.Remove(path); err != nil { - return nil, fmt.Errorf("remove stale socket: %w", err) - } - case !errors.Is(err, os.ErrNotExist): - return nil, fmt.Errorf("inspect %s: %w", path, err) + if err := removeStaleSocket(path); err != nil { + return nil, err } // The socket is created owner-only, so nobody can connect before it is // handed to uid:gid. @@ -298,37 +306,75 @@ func createListener(path string, uid, gid int) (*net.UnixListener, error) { if err != nil { return nil, fmt.Errorf("listen on %s: %w", path, err) } + listener.SetUnlinkOnClose(false) if err := os.Lchown(path, uid, gid); err != nil { listener.Close() return nil, fmt.Errorf("chown %s: %w", path, err) } - listener.SetUnlinkOnClose(true) return listener, nil } -// requireTrustedDirectory reports an error unless dir is a directory owned by -// the current effective UID that group and others cannot write. -func requireTrustedDirectory(dir string) error { - info, err := os.Lstat(dir) +// removeStaleSocket removes a socket at path that nothing serves anymore. It +// refuses to replace a socket that accepts connections, or anything that is +// not a socket. +func removeStaleSocket(path string) error { + info, err := os.Lstat(path) + if errors.Is(err, os.ErrNotExist) { + return nil + } if err != nil { - return fmt.Errorf("inspect socket directory: %w", err) + return fmt.Errorf("inspect %s: %w", path, err) } - if !info.IsDir() { - return fmt.Errorf("socket directory %s is not a directory", dir) + if info.Mode()&os.ModeSocket == 0 { + return fmt.Errorf("%s exists and is not a socket", path) } - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return fmt.Errorf("socket directory %s has no ownership information", dir) + conn, err := net.DialTimeout("unix", path, time.Second) + if err == nil { + conn.Close() + return fmt.Errorf("%s is already being served", path) } - if int(stat.Uid) != os.Geteuid() { - return fmt.Errorf("socket directory %s is owned by uid %d, not %d", dir, stat.Uid, os.Geteuid()) + if !errors.Is(err, syscall.ECONNREFUSED) { + return fmt.Errorf("check whether %s is being served: %w", path, err) } - if info.Mode().Perm()&0o022 != 0 { - return fmt.Errorf("socket directory %s is writable by group or others (mode %#o)", dir, info.Mode().Perm()) + if err := os.Remove(path); err != nil { + return fmt.Errorf("remove stale socket: %w", err) } return nil } +// requireTrustedAncestors reports an error unless dir and every directory +// above it is owned by root or the current effective UID and not writable by +// group or others. Directories above dir may instead be sticky, like /tmp: a +// sticky directory only lets owners rename or remove their own entries, and +// the directory below it is itself checked. dir cannot be sticky: the socket +// is owned by --user, who could then remove and replace it. +func requireTrustedAncestors(dir string) error { + for socketDir := true; ; socketDir = false { + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("inspect socket directory %s: %w", dir, err) + } + if !info.IsDir() { + return fmt.Errorf("socket directory %s is not a directory", dir) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("socket directory %s has no ownership information", dir) + } + if stat.Uid != 0 && int(stat.Uid) != os.Geteuid() { + return fmt.Errorf("socket directory %s is owned by uid %d, not root", dir, stat.Uid) + } + if info.Mode().Perm()&0o022 != 0 && (socketDir || info.Mode()&os.ModeSticky == 0) { + return fmt.Errorf("socket directory %s is writable by group or others (mode %#o)", dir, info.Mode().Perm()) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil + } + dir = parent + } +} + type workerCommandFactory func(context.Context) (*exec.Cmd, error) type helperExecutor struct { diff --git a/cmd/rshell/privileged_helper_linux_test.go b/cmd/rshell/privileged_helper_linux_test.go index 6ac1fe7aa..415b7044d 100644 --- a/cmd/rshell/privileged_helper_linux_test.go +++ b/cmd/rshell/privileged_helper_linux_test.go @@ -8,6 +8,8 @@ package main import ( + "bytes" + "context" "net" "os" "path/filepath" @@ -59,9 +61,11 @@ func TestCreateListener(t *testing.T) { require.NoError(t, err) require.NoError(t, conn.Close()) + // After the privilege drop the helper cannot unlink the socket, so it + // does not try: the supervisor removes it. require.NoError(t, listener.Close()) _, err = os.Lstat(path) - require.ErrorIs(t, err, os.ErrNotExist, "the socket is removed when the helper stops") + require.NoError(t, err) } func TestCreateListenerReplacesStaleSocket(t *testing.T) { @@ -76,6 +80,19 @@ func TestCreateListenerReplacesStaleSocket(t *testing.T) { require.NoError(t, listener.Close()) } +func TestCreateListenerRefusesServedSocket(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + served, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + t.Cleanup(func() { served.Close() }) + + _, err = createListener(path, os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "already being served") + conn, err := net.Dial("unix", path) + require.NoError(t, err, "the served socket must stay reachable") + require.NoError(t, conn.Close()) +} + func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { t.Run("relative path", func(t *testing.T) { _, err := createListener("h.sock", os.Getuid(), os.Getgid()) @@ -90,12 +107,27 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { require.NoError(t, err) require.Equal(t, "keep", string(content), "a non-socket file must not be removed") }) - t.Run("directory writable by others", func(t *testing.T) { + t.Run("socket directory writable by others", func(t *testing.T) { dir := shortTempDir(t) require.NoError(t, os.Chmod(dir, 0o777)) _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) require.ErrorContains(t, err, "writable by group or others") }) + t.Run("sticky socket directory", func(t *testing.T) { + // --user owns the socket and could replace it in a sticky directory. + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky)) + _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "writable by group or others") + }) + t.Run("ancestor writable by others", func(t *testing.T) { + ancestor := shortTempDir(t) + dir := filepath.Join(ancestor, "d") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(ancestor, 0o777)) + _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, ancestor+" is writable by group or others") + }) t.Run("directory is a symlink", func(t *testing.T) { parent := shortTempDir(t) link := filepath.Join(parent, "link") @@ -104,3 +136,23 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { require.ErrorContains(t, err, "is not a directory") }) } + +func TestRunPrivilegedHelperSocketFlags(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("checks the flag handling that runs before the root requirement") + } + socket := filepath.Join(shortTempDir(t), "h.sock") + var stderr bytes.Buffer + code := runPrivilegedHelper(context.Background(), []string{"--socket=" + socket, "--idle-timeout=5s"}, &stderr) + require.Equal(t, 2, code) + require.Contains(t, stderr.String(), "--socket requires --idle-timeout=0") + + for _, args := range [][]string{{"--socket=" + socket}, {"--socket=" + socket, "--idle-timeout=0"}} { + stderr.Reset() + code = runPrivilegedHelper(context.Background(), args, &stderr) + require.Equal(t, 1, code, "%v: the flags are accepted", args) + require.Contains(t, stderr.String(), "requires real uid 0") + } + _, err := os.Lstat(socket) + require.ErrorIs(t, err, os.ErrNotExist, "no socket is created before the helper is ready") +} diff --git a/cmd/rshell/privileged_helper_root_integration_test.go b/cmd/rshell/privileged_helper_root_integration_test.go index 91b36c77f..216f59765 100644 --- a/cmd/rshell/privileged_helper_root_integration_test.go +++ b/cmd/rshell/privileged_helper_root_integration_test.go @@ -8,6 +8,7 @@ package main import ( + "bytes" "context" "crypto/ed25519" "crypto/rand" @@ -19,7 +20,10 @@ import ( "net" "os" "os/exec" + "os/user" "path/filepath" + "strconv" + "syscall" "testing" "time" @@ -194,3 +198,79 @@ func signedIntegrationRequestForAction(t *testing.T, privateKey ed25519.PrivateK }}, }} } + +// TestPrivilegedHelperOwnSocketRootIntegration runs the helper with --socket +// as root, so the socket is created for another user and the helper really +// drops privileges, which unit tests cannot do. +func TestPrivilegedHelperOwnSocketRootIntegration(t *testing.T) { + if os.Getenv("RSHELL_ROOT_INTEGRATION") != "1" { + t.Skip("set RSHELL_ROOT_INTEGRATION=1 to run") + } + if os.Getuid() != 0 { + t.Fatal("root integration test must run with real uid 0") + } + binary := os.Getenv("RSHELL_BINARY") + if binary == "" { + t.Fatal("RSHELL_BINARY is required") + } + nobody, err := user.Lookup("nobody") + require.NoError(t, err) + + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, os.RemoveAll(dir)) }) + require.NoError(t, os.Chmod(dir, 0o755)) + socket := filepath.Join(dir, "h.sock") + + start := func() (*exec.Cmd, *bytes.Buffer) { + var stderr bytes.Buffer + command := exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) + command.Stderr = &stderr + return command, &stderr + } + waitServing := func(command *exec.Cmd, stderr *bytes.Buffer) { + require.Eventually(t, func() bool { + conn, err := net.Dial("unix", socket) + if err != nil { + return false + } + conn.Close() + return true + }, 10*time.Second, 50*time.Millisecond, "helper did not serve its socket: %s", stderr) + } + stop := func(command *exec.Cmd) { + _ = command.Process.Kill() + _ = command.Wait() + } + + first, firstStderr := start() + require.NoError(t, first.Start()) + t.Cleanup(func() { stop(first) }) + waitServing(first, firstStderr) + + var stat syscall.Stat_t + require.NoError(t, syscall.Lstat(socket, &stat)) + require.Equal(t, uint32(syscall.S_IFSOCK), stat.Mode&syscall.S_IFMT) + require.Equal(t, uint32(0o600), stat.Mode&0o777) + require.Equal(t, nobody.Uid, strconv.Itoa(int(stat.Uid)), "the socket belongs to --user") + status, err := os.ReadFile(fmt.Sprintf("/proc/%d/status", first.Process.Pid)) + require.NoError(t, err) + require.Regexp(t, `(?m)^Uid:\s+0\s+`+nobody.Uid+`\s`, string(status), "the helper keeps real uid 0 and serves as --user") + + // A second helper must not take over a socket that is still served. + second, secondStderr := start() + err = second.Run() + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr) + require.Contains(t, secondStderr.String(), "already being served") + + // The helper cannot unlink the socket after dropping privileges, so it is + // left for the supervisor; the next start replaces it. + stop(first) + _, err = os.Lstat(socket) + require.NoError(t, err) + third, thirdStderr := start() + require.NoError(t, third.Start()) + t.Cleanup(func() { stop(third) }) + waitServing(third, thirdStderr) +} diff --git a/docs/PRIVILEGED_HELPER.md b/docs/PRIVILEGED_HELPER.md index e2241c553..c3d401c02 100644 --- a/docs/PRIVILEGED_HELPER.md +++ b/docs/PRIVILEGED_HELPER.md @@ -8,12 +8,22 @@ The helper serves one Unix socket, which only the `dd-agent` account can connect to. It either inherits it from systemd socket activation (fd 3 with `LISTEN_PID` and `LISTEN_FDS=1`), or, with `--socket=PATH`, creates it itself once its policy and account are loaded, before dropping privileges: owned by -`--user` with mode `0600`, and removed when the helper exits. The directory -holding `PATH` must be owned by root and not writable by group or others, and -an existing file at `PATH` is only replaced if it is a socket. With `--socket`, -the socket exists only while a ready helper serves it. Pass -`--idle-timeout=0` to keep it serving until it is stopped, since nothing would -restart it on demand. +`--user` and its primary group, with mode `0600`. + +With `--socket`: + +- Every directory above `PATH` must be owned by root and not writable by group + or others, so nobody else can swap in a socket of their own. Directories + further up may instead be sticky, like `/tmp`; the socket's own directory may + not, since `--user` owns the socket. +- An existing socket at `PATH` is replaced only if nothing accepts connections + on it. A socket that is still being served, or any other file, makes the + helper refuse to start. +- The helper does not remove the socket when it exits: by then it runs as + `--user`, which cannot write the directory. Removing it is the supervisor's + job, for example by placing it in a systemd `RuntimeDirectory=`. +- `--idle-timeout` defaults to `0`, and a non-zero value is rejected: nothing + would start the helper again on the next connection. The helper must start with real UID 0. It loads its trust policy, resolves the `dd-agent` UID and complete group set, permanently switches its real, effective, From d57cc35c5040d8cc8e096551a39b4cf0d5c7eeb0 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Tue, 27 Oct 2026 13:03:24 -0400 Subject: [PATCH 3/5] fix(privileged-helper): reject unclean --socket paths, run the new root test in CI - Reject a --socket path that is not already clean. The ancestor check examined filepath.Clean(path) while the bind used the raw path, and the kernel resolves ".." after following a symlink, so link/../h.sock could bind in a directory the check never saw. - Run TestPrivilegedHelperOwnSocketRootIntegration in the root CI job and require it to pass; it was skipped everywhere in CI. - Say the helper does not, rather than cannot, remove its socket: only its effective UID drops, and it deliberately never regains root for it. Document that the path must not go through symlinks such as /var/run. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 5 +++-- cmd/rshell/privileged_helper_linux.go | 17 ++++++++++++----- cmd/rshell/privileged_helper_linux_test.go | 9 +++++++++ docs/PRIVILEGED_HELPER.md | 10 +++++++--- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 71813fdee..abeabb158 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -66,12 +66,13 @@ jobs: | tee -a "$RUNNER_TEMP/root-tests.log" sudo env RSHELL_ROOT_INTEGRATION=1 RSHELL_BINARY="$helper_dir/rshell" \ "$RUNNER_TEMP/rshell.test" -test.v -test.timeout=2m \ - -test.run='^TestPrivilegedHelperRootIntegration$' \ + -test.run='^TestPrivilegedHelper(OwnSocket)?RootIntegration$' \ | tee -a "$RUNNER_TEMP/root-tests.log" for test in TestRestrictEnforcesReadAndWritePolicy \ TestRestrictUsesValidatedDescriptorAfterPathReplacement \ TestDeniedSyscallRemainsDeniedAfterSelectiveElevation \ - TestPrivilegedHelperRootIntegration; do + TestPrivilegedHelperRootIntegration \ + TestPrivilegedHelperOwnSocketRootIntegration; do if ! grep -Eq "^--- PASS: $test \\(" "$RUNNER_TEMP/root-tests.log"; then echo "Required root test did not pass: $test" >&2 exit 1 diff --git a/cmd/rshell/privileged_helper_linux.go b/cmd/rshell/privileged_helper_linux.go index a93656975..fe44f62e8 100644 --- a/cmd/rshell/privileged_helper_linux.go +++ b/cmd/rshell/privileged_helper_linux.go @@ -46,7 +46,7 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i flags.StringVar(&policyPath, "credential", policyPath, "deprecated alias for --policy") idleTimeout := flags.Duration("idle-timeout", 30*time.Second, "exit after this long without a connection") userName := flags.String("user", "dd-agent", "unprivileged effective user") - socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; every directory above it must be owned by root and not writable by others unless sticky; implies --idle-timeout=0") + socketPath := flags.String("socket", "", "create and serve this Unix socket, owned by --user with mode 0600, instead of a systemd-activated one; the path must be clean and must not go through symlinks (such as /var/run), and every directory above it must be owned by root and not writable by others unless sticky; implies --idle-timeout=0") if err := flags.Parse(args); err != nil { return 2 } @@ -285,14 +285,21 @@ func inheritedListener() (net.Listener, error) { // socket is replaced; a socket that is still being served, or any other file, // is not. // -// The socket is not removed when the helper exits: by then the helper runs as -// --user, which cannot write the directory. Removing it is the supervisor's -// job, for example with systemd's RuntimeDirectory=. +// The helper does not remove the socket when it exits, so that the +// socket-facing process, which serves as --user, never regains root for it. +// Removing it is the supervisor's job, for example with systemd's +// RuntimeDirectory=. func createListener(path string, uid, gid int) (*net.UnixListener, error) { if !filepath.IsAbs(path) { return nil, fmt.Errorf("socket path %q is not absolute", path) } - if err := requireTrustedAncestors(filepath.Dir(filepath.Clean(path))); err != nil { + // The checks below must examine the directories the kernel resolves. A + // path that is not clean could differ: the kernel resolves ".." after + // following a symlink, which filepath.Clean cannot see. + if path != filepath.Clean(path) { + return nil, fmt.Errorf("socket path %q is not clean", path) + } + if err := requireTrustedAncestors(filepath.Dir(path)); err != nil { return nil, err } if err := removeStaleSocket(path); err != nil { diff --git a/cmd/rshell/privileged_helper_linux_test.go b/cmd/rshell/privileged_helper_linux_test.go index 415b7044d..4dee723c8 100644 --- a/cmd/rshell/privileged_helper_linux_test.go +++ b/cmd/rshell/privileged_helper_linux_test.go @@ -128,6 +128,15 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) require.ErrorContains(t, err, ancestor+" is writable by group or others") }) + t.Run("path is not clean", func(t *testing.T) { + // The kernel resolves ".." after following link, so a check on the + // cleaned path would examine the wrong directory. + parent := shortTempDir(t) + link := filepath.Join(parent, "link") + require.NoError(t, os.Symlink(filepath.Join(shortTempDir(t), "sub"), link)) + _, err := createListener(link+"/../h.sock", os.Getuid(), os.Getgid()) + require.ErrorContains(t, err, "is not clean") + }) t.Run("directory is a symlink", func(t *testing.T) { parent := shortTempDir(t) link := filepath.Join(parent, "link") diff --git a/docs/PRIVILEGED_HELPER.md b/docs/PRIVILEGED_HELPER.md index c3d401c02..9c6b0ecce 100644 --- a/docs/PRIVILEGED_HELPER.md +++ b/docs/PRIVILEGED_HELPER.md @@ -12,6 +12,9 @@ once its policy and account are loaded, before dropping privileges: owned by With `--socket`: +- `PATH` must be absolute and clean (no `.`, `..` or repeated slashes), and + must not go through symlinks: under `/var/run`, which is usually a symlink to + `/run`, use `/run` directly. - Every directory above `PATH` must be owned by root and not writable by group or others, so nobody else can swap in a socket of their own. Directories further up may instead be sticky, like `/tmp`; the socket's own directory may @@ -19,9 +22,10 @@ With `--socket`: - An existing socket at `PATH` is replaced only if nothing accepts connections on it. A socket that is still being served, or any other file, makes the helper refuse to start. -- The helper does not remove the socket when it exits: by then it runs as - `--user`, which cannot write the directory. Removing it is the supervisor's - job, for example by placing it in a systemd `RuntimeDirectory=`. +- The helper does not remove the socket when it exits, so that the + socket-facing process, which serves as `--user`, never regains root for it. + Removing it is the supervisor's job, for example by placing it in a systemd + `RuntimeDirectory=`. - `--idle-timeout` defaults to `0`, and a non-zero value is rejected: nothing would start the helper again on the next connection. From b63183659b815156bc7628f6fd45536eac3d6f79 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Tue, 27 Oct 2026 15:10:57 -0400 Subject: [PATCH 4/5] fix(privileged-helper): serialize --socket startup, clean up a failed chown, check traversal Runtime QA findings on Ubuntu 24.04 and RHEL 9: - Concurrent starts could leave several live helpers: two could both find a stale socket stale, and the later one unlinked the socket the earlier one had just bound. The helper now holds an exclusive, non-blocking flock on a root-owned PATH.lock for its lifetime, taken before the stale check and held through bind and chown; a second helper refuses to start. The kernel releases it however the helper exits. - A failed Lchown left a root-owned socket behind, because unlink on close was disabled before the chown. It is now disabled only once the chown succeeds, so a failed start, still running as root, removes it. - The socket's directories must now be searchable by --user or one of its groups, so the helper cannot run healthy behind a socket its client gets EACCES on. A root integration test now starts 20 helpers at once over a stale socket, 60 times, and requires exactly one to serve; with the lock disabled it fails within 25 iterations. It runs in the root CI job. Unit tests cover the lock, the failed-chown cleanup and the traversal check. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 5 +- cmd/rshell/privileged_helper_linux.go | 131 +++++++++++++++--- cmd/rshell/privileged_helper_linux_test.go | 70 ++++++++-- ...privileged_helper_root_integration_test.go | 79 ++++++++++- docs/PRIVILEGED_HELPER.md | 12 +- 5 files changed, 259 insertions(+), 38 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index abeabb158..2cc9ec0af 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -66,13 +66,14 @@ jobs: | tee -a "$RUNNER_TEMP/root-tests.log" sudo env RSHELL_ROOT_INTEGRATION=1 RSHELL_BINARY="$helper_dir/rshell" \ "$RUNNER_TEMP/rshell.test" -test.v -test.timeout=2m \ - -test.run='^TestPrivilegedHelper(OwnSocket)?RootIntegration$' \ + -test.run='^TestPrivilegedHelper(OwnSocket|ConcurrentSocketStarts)?RootIntegration$' \ | tee -a "$RUNNER_TEMP/root-tests.log" for test in TestRestrictEnforcesReadAndWritePolicy \ TestRestrictUsesValidatedDescriptorAfterPathReplacement \ TestDeniedSyscallRemainsDeniedAfterSelectiveElevation \ TestPrivilegedHelperRootIntegration \ - TestPrivilegedHelperOwnSocketRootIntegration; do + TestPrivilegedHelperOwnSocketRootIntegration \ + TestPrivilegedHelperConcurrentSocketStartsRootIntegration; do if ! grep -Eq "^--- PASS: $test \\(" "$RUNNER_TEMP/root-tests.log"; then echo "Required root test did not pass: $test" >&2 exit 1 diff --git a/cmd/rshell/privileged_helper_linux.go b/cmd/rshell/privileged_helper_linux.go index fe44f62e8..761706fe6 100644 --- a/cmd/rshell/privileged_helper_linux.go +++ b/cmd/rshell/privileged_helper_linux.go @@ -89,11 +89,15 @@ func runPrivilegedHelper(ctx context.Context, args []string, stderr io.Writer) i if *socketPath != "" { // Created last, once the helper is ready to serve, so that the socket // existing means a working helper is behind it. - listener, err = createListener(*socketPath, credentials.uid, credentials.primaryGID) + var lock *os.File + groups := append([]int{credentials.primaryGID}, credentials.supplementaryGIDs...) + listener, lock, err = createListener(*socketPath, credentials.uid, groups) if err != nil { fmt.Fprintf(stderr, "creating socket: %v\n", err) return 1 } + // Held until the helper exits, so no other helper starts on this path. + defer lock.Close() } else { listener, err = inheritedListener() if err != nil { @@ -277,48 +281,131 @@ func inheritedListener() (net.Listener, error) { return listener, nil } -// createListener creates the helper socket at path, owned by uid:gid with mode -// 0600, while the helper still runs as root. The socket is the only access -// control in front of the helper, so every directory above it must be one -// that only root (the helper's effective UID) can change: anyone else able to -// rename an entry on the way could swap in a socket they listen on. A stale -// socket is replaced; a socket that is still being served, or any other file, -// is not. +// createListener creates the helper socket at path, owned by uid and the +// first of groups with mode 0600, while the helper still runs as root, and +// returns it with the lock that keeps other helpers off path. The caller holds +// the lock for as long as it serves. +// +// The socket is the only access control in front of the helper, so every +// directory above it must be one that only root (the helper's effective UID) +// can change: anyone else able to rename an entry on the way could swap in a +// socket they listen on. Every one of them must also let uid, or one of +// groups, through, so the helper's client can reach the socket. A stale socket +// is replaced; a socket that is still being served, or any other file, is not. // // The helper does not remove the socket when it exits, so that the // socket-facing process, which serves as --user, never regains root for it. -// Removing it is the supervisor's job, for example with systemd's -// RuntimeDirectory=. -func createListener(path string, uid, gid int) (*net.UnixListener, error) { +// Removing it, and the lock file, is the supervisor's job, for example with +// systemd's RuntimeDirectory=. +func createListener(path string, uid int, groups []int) (*net.UnixListener, *os.File, error) { if !filepath.IsAbs(path) { - return nil, fmt.Errorf("socket path %q is not absolute", path) + return nil, nil, fmt.Errorf("socket path %q is not absolute", path) } // The checks below must examine the directories the kernel resolves. A // path that is not clean could differ: the kernel resolves ".." after // following a symlink, which filepath.Clean cannot see. if path != filepath.Clean(path) { - return nil, fmt.Errorf("socket path %q is not clean", path) + return nil, nil, fmt.Errorf("socket path %q is not clean", path) } - if err := requireTrustedAncestors(filepath.Dir(path)); err != nil { - return nil, err + dir := filepath.Dir(path) + if err := requireTrustedAncestors(dir); err != nil { + return nil, nil, err + } + if err := requireTraversable(dir, uid, groups); err != nil { + return nil, nil, err + } + // Serializes the stale check, the bind and the chown across helpers: + // otherwise two of them can both find the old socket stale, and the + // second unlinks the socket the first just bound. + lock, err := lockSocketPath(path) + if err != nil { + return nil, nil, err } if err := removeStaleSocket(path); err != nil { - return nil, err + lock.Close() + return nil, nil, err } // The socket is created owner-only, so nobody can connect before it is - // handed to uid:gid. + // handed to uid. oldUmask := syscall.Umask(0o177) listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) syscall.Umask(oldUmask) if err != nil { - return nil, fmt.Errorf("listen on %s: %w", path, err) + lock.Close() + return nil, nil, fmt.Errorf("listen on %s: %w", path, err) } - listener.SetUnlinkOnClose(false) - if err := os.Lchown(path, uid, gid); err != nil { + if err := os.Lchown(path, uid, groups[0]); err != nil { + // Still root here, so closing the listener unlinks the socket. listener.Close() - return nil, fmt.Errorf("chown %s: %w", path, err) + lock.Close() + return nil, nil, fmt.Errorf("chown %s: %w", path, err) + } + listener.SetUnlinkOnClose(false) + return listener, lock, nil +} + +// lockSocketPath takes an exclusive lock on path + ".lock", a root-owned +// file next to the socket, and fails at once if another helper holds it. The +// kernel releases the lock when its holder exits, however it exits. +func lockSocketPath(path string) (*os.File, error) { + lockPath := path + ".lock" + lock, err := os.OpenFile(lockPath, os.O_RDWR|os.O_CREATE|syscall.O_NOFOLLOW|syscall.O_CLOEXEC, 0o600) + if err != nil { + return nil, fmt.Errorf("open %s: %w", lockPath, err) + } + info, err := lock.Stat() + if err != nil { + lock.Close() + return nil, fmt.Errorf("inspect %s: %w", lockPath, err) + } + if stat, ok := info.Sys().(*syscall.Stat_t); !ok || !info.Mode().IsRegular() || int(stat.Uid) != os.Geteuid() { + lock.Close() + return nil, fmt.Errorf("%s is not a regular file owned by uid %d", lockPath, os.Geteuid()) + } + if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil { + lock.Close() + if errors.Is(err, syscall.EWOULDBLOCK) { + return nil, fmt.Errorf("another helper is serving %s", path) + } + return nil, fmt.Errorf("lock %s: %w", lockPath, err) + } + return lock, nil +} + +// requireTraversable reports an error unless uid, or one of groups, may +// search dir and every directory above it, as the kernel decides for a +// process with those credentials. +func requireTraversable(dir string, uid int, groups []int) error { + for { + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("inspect socket directory %s: %w", dir, err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return fmt.Errorf("socket directory %s has no ownership information", dir) + } + perm := info.Mode().Perm() + var searchable bool + switch { + case uid == 0: + searchable = perm&0o111 != 0 + case int(stat.Uid) == uid: + searchable = perm&0o100 != 0 + case slices.Contains(groups, int(stat.Gid)): + searchable = perm&0o010 != 0 + default: + searchable = perm&0o001 != 0 + } + if !searchable { + return fmt.Errorf("socket directory %s cannot be searched by uid %d (mode %#o)", dir, uid, perm) + } + parent := filepath.Dir(dir) + if parent == dir { + return nil + } + dir = parent } - return listener, nil } // removeStaleSocket removes a socket at path that nothing serves anymore. It diff --git a/cmd/rshell/privileged_helper_linux_test.go b/cmd/rshell/privileged_helper_linux_test.go index 4dee723c8..2a538ad75 100644 --- a/cmd/rshell/privileged_helper_linux_test.go +++ b/cmd/rshell/privileged_helper_linux_test.go @@ -48,8 +48,9 @@ func shortTempDir(t *testing.T) string { func TestCreateListener(t *testing.T) { path := filepath.Join(shortTempDir(t), "h.sock") - listener, err := createListener(path, os.Getuid(), os.Getgid()) + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) require.NoError(t, err) + t.Cleanup(func() { lock.Close() }) var stat syscall.Stat_t require.NoError(t, syscall.Lstat(path, &stat)) @@ -75,8 +76,9 @@ func TestCreateListenerReplacesStaleSocket(t *testing.T) { stale.SetUnlinkOnClose(false) require.NoError(t, stale.Close()) - listener, err := createListener(path, os.Getuid(), os.Getgid()) + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) require.NoError(t, err) + t.Cleanup(func() { lock.Close() }) require.NoError(t, listener.Close()) } @@ -86,7 +88,7 @@ func TestCreateListenerRefusesServedSocket(t *testing.T) { require.NoError(t, err) t.Cleanup(func() { served.Close() }) - _, err = createListener(path, os.Getuid(), os.Getgid()) + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "already being served") conn, err := net.Dial("unix", path) require.NoError(t, err, "the served socket must stay reachable") @@ -95,13 +97,13 @@ func TestCreateListenerRefusesServedSocket(t *testing.T) { func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { t.Run("relative path", func(t *testing.T) { - _, err := createListener("h.sock", os.Getuid(), os.Getgid()) + _, _, err := createListener("h.sock", os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "not absolute") }) t.Run("existing non-socket file", func(t *testing.T) { path := filepath.Join(shortTempDir(t), "h.sock") require.NoError(t, os.WriteFile(path, []byte("keep"), 0o600)) - _, err := createListener(path, os.Getuid(), os.Getgid()) + _, _, err := createListener(path, os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "is not a socket") content, err := os.ReadFile(path) require.NoError(t, err) @@ -110,14 +112,14 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { t.Run("socket directory writable by others", func(t *testing.T) { dir := shortTempDir(t) require.NoError(t, os.Chmod(dir, 0o777)) - _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "writable by group or others") }) t.Run("sticky socket directory", func(t *testing.T) { // --user owns the socket and could replace it in a sticky directory. dir := shortTempDir(t) require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky)) - _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "writable by group or others") }) t.Run("ancestor writable by others", func(t *testing.T) { @@ -125,7 +127,7 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { dir := filepath.Join(ancestor, "d") require.NoError(t, os.Mkdir(dir, 0o755)) require.NoError(t, os.Chmod(ancestor, 0o777)) - _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), os.Getgid()) + _, _, err := createListener(filepath.Join(dir, "h.sock"), os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, ancestor+" is writable by group or others") }) t.Run("path is not clean", func(t *testing.T) { @@ -134,18 +136,66 @@ func TestCreateListenerRejectsUnsafeTargets(t *testing.T) { parent := shortTempDir(t) link := filepath.Join(parent, "link") require.NoError(t, os.Symlink(filepath.Join(shortTempDir(t), "sub"), link)) - _, err := createListener(link+"/../h.sock", os.Getuid(), os.Getgid()) + _, _, err := createListener(link+"/../h.sock", os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "is not clean") }) t.Run("directory is a symlink", func(t *testing.T) { parent := shortTempDir(t) link := filepath.Join(parent, "link") require.NoError(t, os.Symlink(shortTempDir(t), link)) - _, err := createListener(filepath.Join(link, "h.sock"), os.Getuid(), os.Getgid()) + _, _, err := createListener(filepath.Join(link, "h.sock"), os.Getuid(), []int{os.Getgid()}) require.ErrorContains(t, err, "is not a directory") }) } +func TestCreateListenerLocksThePath(t *testing.T) { + path := filepath.Join(shortTempDir(t), "h.sock") + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err) + t.Cleanup(func() { listener.Close() }) + + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "another helper is serving") + + // The lock outlives the socket: a helper whose socket was removed by + // someone else still keeps others off the path until it exits. + require.NoError(t, os.Remove(path)) + _, _, err = createListener(path, os.Getuid(), []int{os.Getgid()}) + require.ErrorContains(t, err, "another helper is serving") + + require.NoError(t, lock.Close()) + listener2, lock2, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err, "the lock is released when its holder closes it") + require.NoError(t, lock2.Close()) + require.NoError(t, listener2.Close()) +} + +func TestCreateListenerCleansUpFailedChown(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root can chown to any account") + } + path := filepath.Join(shortTempDir(t), "h.sock") + _, _, err := createListener(path, 65534, []int{65534}) + require.ErrorContains(t, err, "chown") + _, err = os.Lstat(path) + require.ErrorIs(t, err, os.ErrNotExist, "a socket that was never handed over must not be left behind") + + listener, lock, err := createListener(path, os.Getuid(), []int{os.Getgid()}) + require.NoError(t, err, "a failed start must release the lock") + require.NoError(t, lock.Close()) + require.NoError(t, listener.Close()) +} + +func TestCreateListenerRequiresTraversableDirectories(t *testing.T) { + dir := shortTempDir(t) + require.NoError(t, os.Chmod(dir, 0o700)) + _, _, err := createListener(filepath.Join(dir, "h.sock"), 65534, []int{65534}) + require.ErrorContains(t, err, "cannot be searched by uid 65534") + + require.NoError(t, os.Chmod(dir, 0o711)) + require.NoError(t, requireTraversable(dir, 65534, []int{65534})) +} + func TestRunPrivilegedHelperSocketFlags(t *testing.T) { if os.Getuid() == 0 { t.Skip("checks the flag handling that runs before the root requirement") diff --git a/cmd/rshell/privileged_helper_root_integration_test.go b/cmd/rshell/privileged_helper_root_integration_test.go index 216f59765..10cb19906 100644 --- a/cmd/rshell/privileged_helper_root_integration_test.go +++ b/cmd/rshell/privileged_helper_root_integration_test.go @@ -23,6 +23,7 @@ import ( "os/user" "path/filepath" "strconv" + "sync" "syscall" "testing" "time" @@ -262,7 +263,7 @@ func TestPrivilegedHelperOwnSocketRootIntegration(t *testing.T) { err = second.Run() var exitErr *exec.ExitError require.ErrorAs(t, err, &exitErr) - require.Contains(t, secondStderr.String(), "already being served") + require.Regexp(t, "another helper is serving|already being served", secondStderr.String()) // The helper cannot unlink the socket after dropping privileges, so it is // left for the supervisor; the next start replaces it. @@ -274,3 +275,79 @@ func TestPrivilegedHelperOwnSocketRootIntegration(t *testing.T) { t.Cleanup(func() { stop(third) }) waitServing(third, thirdStderr) } + +// TestPrivilegedHelperConcurrentSocketStartsRootIntegration starts many +// helpers on the same --socket path at once, over a stale socket. Exactly one +// may serve: without the startup lock, two can both find the stale socket +// stale, and the later one unlinks the socket the earlier one just bound. +func TestPrivilegedHelperConcurrentSocketStartsRootIntegration(t *testing.T) { + if os.Getenv("RSHELL_ROOT_INTEGRATION") != "1" { + t.Skip("set RSHELL_ROOT_INTEGRATION=1 to run") + } + if os.Getuid() != 0 { + t.Fatal("root integration test must run with real uid 0") + } + binary := os.Getenv("RSHELL_BINARY") + if binary == "" { + t.Fatal("RSHELL_BINARY is required") + } + const starters = 20 + for iteration := 0; iteration < 60; iteration++ { + dir, err := os.MkdirTemp("", "rsh") + require.NoError(t, err) + require.NoError(t, os.Chmod(dir, 0o755)) + socket := filepath.Join(dir, "h.sock") + // Start from a stale socket, as after a crash: every starter then + // finds it stale, which is what makes them race to replace it. + stale, err := net.ListenUnix("unix", &net.UnixAddr{Name: socket, Net: "unix"}) + require.NoError(t, err) + stale.SetUnlinkOnClose(false) + require.NoError(t, stale.Close()) + + commands := make([]*exec.Cmd, starters) + exited := make(chan int, starters) + release := make(chan struct{}) + var ready sync.WaitGroup + for i := range commands { + commands[i] = exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) + ready.Add(1) + go func(i int) { + ready.Done() + <-release + if err := commands[i].Start(); err != nil { + exited <- i + return + } + _ = commands[i].Wait() + exited <- i + }(i) + } + ready.Wait() + close(release) + + // Every starter but one exits; the survivor keeps serving. + for i := 0; i < starters-1; i++ { + select { + case <-exited: + case <-time.After(20 * time.Second): + t.Fatalf("iteration %d: only %d of %d starters exited", iteration, i, starters-1) + } + } + select { + case <-exited: + t.Fatalf("iteration %d: every starter exited, none serves", iteration) + case <-time.After(500 * time.Millisecond): + } + conn, err := net.Dial("unix", socket) + require.NoError(t, err, "iteration %d: the surviving helper's socket must be reachable", iteration) + conn.Close() + + for _, command := range commands { + if command.Process != nil { + _ = command.Process.Kill() + } + } + <-exited + require.NoError(t, os.RemoveAll(dir)) + } +} diff --git a/docs/PRIVILEGED_HELPER.md b/docs/PRIVILEGED_HELPER.md index 9c6b0ecce..09e13c562 100644 --- a/docs/PRIVILEGED_HELPER.md +++ b/docs/PRIVILEGED_HELPER.md @@ -16,16 +16,22 @@ With `--socket`: must not go through symlinks: under `/var/run`, which is usually a symlink to `/run`, use `/run` directly. - Every directory above `PATH` must be owned by root and not writable by group - or others, so nobody else can swap in a socket of their own. Directories + or others, so nobody else can swap in a socket of their own, and must let + `--user` or one of its groups search it, so the client can reach the socket. Directories further up may instead be sticky, like `/tmp`; the socket's own directory may not, since `--user` owns the socket. +- The helper holds an exclusive lock on `PATH.lock`, a root-owned file next to + the socket, for as long as it runs, so only one helper serves `PATH`; a + second one refuses to start. The kernel releases the lock however the helper + exits. - An existing socket at `PATH` is replaced only if nothing accepts connections on it. A socket that is still being served, or any other file, makes the helper refuse to start. - The helper does not remove the socket when it exits, so that the socket-facing process, which serves as `--user`, never regains root for it. - Removing it is the supervisor's job, for example by placing it in a systemd - `RuntimeDirectory=`. + Removing it, and the lock file, is the supervisor's job, for example by + placing them in a systemd `RuntimeDirectory=`. A socket that fails to start + is removed before the helper exits. - `--idle-timeout` defaults to `0`, and a non-zero value is rejected: nothing would start the helper again on the next connection. From c6e6e1af0f9fc1d22c9b5321fdf752ac410ef906 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Tue, 27 Oct 2026 16:02:48 -0400 Subject: [PATCH 5/5] test(privileged-helper): fix a data race in the concurrent-start root test The root CI job builds the tests with -race, which flagged the cleanup loop reading each exec.Cmd's Process while the starter goroutines were still writing it in Start. Record started processes under a mutex. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- ...privileged_helper_root_integration_test.go | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/cmd/rshell/privileged_helper_root_integration_test.go b/cmd/rshell/privileged_helper_root_integration_test.go index 10cb19906..b9497c3e9 100644 --- a/cmd/rshell/privileged_helper_root_integration_test.go +++ b/cmd/rshell/privileged_helper_root_integration_test.go @@ -304,23 +304,29 @@ func TestPrivilegedHelperConcurrentSocketStartsRootIntegration(t *testing.T) { stale.SetUnlinkOnClose(false) require.NoError(t, stale.Close()) - commands := make([]*exec.Cmd, starters) - exited := make(chan int, starters) + var ( + mu sync.Mutex + processes []*os.Process + ) + exited := make(chan struct{}, starters) release := make(chan struct{}) var ready sync.WaitGroup - for i := range commands { - commands[i] = exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) + for range starters { + command := exec.Command(binary, "privileged-helper", "--user=nobody", "--socket="+socket) ready.Add(1) - go func(i int) { + go func() { ready.Done() <-release - if err := commands[i].Start(); err != nil { - exited <- i + if err := command.Start(); err != nil { + exited <- struct{}{} return } - _ = commands[i].Wait() - exited <- i - }(i) + mu.Lock() + processes = append(processes, command.Process) + mu.Unlock() + _ = command.Wait() + exited <- struct{}{} + }() } ready.Wait() close(release) @@ -342,11 +348,11 @@ func TestPrivilegedHelperConcurrentSocketStartsRootIntegration(t *testing.T) { require.NoError(t, err, "iteration %d: the surviving helper's socket must be reachable", iteration) conn.Close() - for _, command := range commands { - if command.Process != nil { - _ = command.Process.Kill() - } + mu.Lock() + for _, process := range processes { + _ = process.Kill() } + mu.Unlock() <-exited require.NoError(t, os.RemoveAll(dir)) }