From 22351f62abd4a238799708a965381ba3afb3460f Mon Sep 17 00:00:00 2001 From: Adrian Bienkowski Date: Mon, 28 Sep 2026 17:08:34 -0400 Subject: [PATCH] fix: set the listening socket mode instead of inheriting the umask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bind(2) on an AF_UNIX socket creates the inode with 0777 & ~umask, and none of the three implementations touched it afterwards. connect(2) requires write permission on the socket, so at the usual umask 022 the socket came out 0755 and the grant the README documents — put the caller in the socket's group — silently did not work; only the owning uid could connect. Under umask 0 it came out 0777 and any local uid could drive the full Docker API through it. With the TCP listener gone this mode is the entire access-control boundary. Adds --listen-socket-mode (default 0660) and --listen-socket-group, taking either a group name or a gid. The umask is narrowed to 0177 around the bind so the socket is created at 0600, then chown'd and chmod'd before the first accept. Setting the mode after bind instead would leave a window in which the socket is already listening at the ambient mode. A world-writable mode is refused at startup with exit 2. There is deliberately no opt-out: it removes the boundary entirely. Both flags are ignored for fd://3, where systemd owns the socket and SocketMode/SocketGroup in the .socket unit are the right controls. Group name lookup differs by runtime: Go uses os/user.LookupGroup and Rust uses getgrnam_r, both of which consult NSS. Node has no equivalent, so TypeScript reads /etc/group — enough for the container case, and the error points at passing a numeric gid otherwise. TypeScript's bind is extracted to ts/src/listen.ts so the mode can be tested at all; it was top-level module code with no export. Verified natively under both umasks, all three: mode 660 either way, and serving. Each new test was checked to fail without the fix (it reports 755 under umask 022 and 777 under umask 0, exactly as reported). Closes #40 --- README.md | 30 ++++- deploy/docker-compose.sock.yml | 6 + deploy/test-sock.sh | 31 +++++ go/main.go | 106 ++++++++++++++++- go/main_test.go | 130 +++++++++++++++++++-- rs/Cargo.lock | 1 + rs/Cargo.toml | 3 + rs/src/main.rs | 201 +++++++++++++++++++++++++++++++-- ts/src/flags.test.ts | 81 ++++++++++++- ts/src/flags.ts | 81 +++++++++++++ ts/src/index.ts | 44 +++++++- ts/src/listen.test.ts | 117 +++++++++++++++++++ ts/src/listen.ts | 56 +++++++++ 13 files changed, 857 insertions(+), 30 deletions(-) create mode 100644 ts/src/listen.test.ts create mode 100644 ts/src/listen.ts diff --git a/README.md b/README.md index 50a4bd3..dd1d706 100644 --- a/README.md +++ b/README.md @@ -125,11 +125,16 @@ make validate ```bash ./docker-socket-policy \ --listen-socket=/var/run/docker-socket-policy.sock \ + --listen-socket-group=builders \ --docker-host=/var/run/docker.sock \ --config-dir=./config \ --log-file=/tmp/docker-socket-policy.log ``` +The socket is created `0660` owned by `--listen-socket-group`, so members of +that group can connect and nobody else can. Omit the flag and only the proxy's +own user can reach it. + ### Configure a Service Create a YAML policy in the config directory: @@ -217,6 +222,8 @@ docker pull attacker/malware:latest # denied: image not in allowlist | `--config-dir` | `/etc/docker-socket-policy/services` | Policy config directory | | `--log-file` | `/var/log/docker-socket-policy.log` | Audit log path | | `--readonly` | `false` | Enable read-only mode | +| `--listen-socket-mode` | `0660` | Octal mode for the listening socket (ignored for `fd://3`) | +| `--listen-socket-group` | *(none)* | Group name or gid owning the listening socket (ignored for `fd://3`) | > **Unix socket security boundary**: the proxy listens on a Unix socket only, > in all three implementations. Access control is the file permissions and Unix @@ -228,10 +235,25 @@ docker pull attacker/malware:latest # denied: image not in allowlist > Docker daemon over Unix sockets exclusively and reject `tcp://` and `http://` > schemes for `--docker-host`. > -> To grant access, place the caller's container user in the group that owns the -> listening socket and bind-mount that socket in; to revoke it, remove the group -> membership. If the proxy cannot reach the daemon socket because of its own -> group permissions, requests surface as `403`. +> To grant access, set `--listen-socket-group` to a group, place the caller's +> container user in that group, and bind-mount the socket in; to revoke it, +> remove the group membership. If the proxy cannot reach the daemon socket +> because of its own group permissions, requests surface as `403`. +> +> The socket is created at `--listen-socket-mode` (default `0660`) regardless of +> the ambient umask. This matters: `bind(2)` applies `0777 & ~umask`, so left to +> a default umask the socket would be `0755`, and `connect(2)` on a Unix socket +> requires **write** permission — the group grant above would silently not work. +> Under `umask 0` it would be `0777`, reachable by every local uid. A +> world-writable mode is rejected at startup and there is no opt-out. +> +> Without `--listen-socket-group` the socket is `0660` owned by the proxy's own +> user and group, so only that user can connect. The group is what makes the +> mode useful. +> +> Under `fd://3` the socket belongs to systemd: use `SocketMode=` and +> `SocketGroup=` in the `.socket` unit instead, as in the example below. Both +> flags are ignored in that mode. > > **What the socket does not give you is per-service isolation.** The proxy > performs no caller authentication: it selects a policy from the `Image` field diff --git a/deploy/docker-compose.sock.yml b/deploy/docker-compose.sock.yml index 6bb64c9..48dcec9 100644 --- a/deploy/docker-compose.sock.yml +++ b/deploy/docker-compose.sock.yml @@ -59,6 +59,10 @@ services: command: - --docker-host=/sock/docker.sock - --listen-socket=/sock/granted.sock + # Exercises the #40 flags: the socket must come out 0660 owned by this + # group regardless of the image's umask. + - --listen-socket-mode=0660 + - --listen-socket-group=2001 - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log @@ -77,6 +81,8 @@ services: command: - --docker-host=/sock/docker.sock - --listen-socket=/sock/denied.sock + - --listen-socket-mode=0660 + - --listen-socket-group=3001 - --config-dir=/etc/docker-socket-policy/services - --log-file=/tmp/docker-socket-policy.log diff --git a/deploy/test-sock.sh b/deploy/test-sock.sh index aed5989..474f8d6 100755 --- a/deploy/test-sock.sh +++ b/deploy/test-sock.sh @@ -115,6 +115,37 @@ if [ $i -eq 15 ]; then fi echo "" +# ─── Listening socket permissions ───────────────────── + +# Regression guard for #40. The listening socket's mode used to be whatever the +# ambient umask left behind — 0755 by default, 0777 under umask 0. connect(2) +# on a Unix socket needs write permission, so at 0755 the documented group +# grant did not work, and at 0777 every local uid could drive the Docker API. +echo "--- listening socket permissions ---" + +MODE=$(stat -c '%a' "$GRANTED_SOCK" 2>/dev/null || echo "?") +check "granted.sock mode is 660, not the umask default" "660" "$MODE" + +GROUP=$(stat -c '%g' "$GRANTED_SOCK" 2>/dev/null || echo "?") +check "granted.sock is owned by --listen-socket-group 2001" "2001" "$GROUP" + +# The specific failure mode that removes the boundary entirely. +case "$MODE" in + *[2367]) + echo " FAIL: granted.sock is world-writable (mode $MODE) — any local uid could connect" + FAIL=$((FAIL+1)) + ;; + *) + echo " PASS: granted.sock is not world-writable" + PASS=$((PASS+1)) + ;; +esac + +MODE=$(stat -c '%a' "$DENIED_SOCK" 2>/dev/null || echo "?") +check "denied.sock mode is 660" "660" "$MODE" + +echo "" + # ─── proxy-granted: should work ─────────────────────── echo "--- proxy-granted (GID 2001, has group access) ---" diff --git a/go/main.go b/go/main.go index 1118659..8cbc6ea 100644 --- a/go/main.go +++ b/go/main.go @@ -11,6 +11,8 @@ import ( "net/http" "os" "os/signal" + "os/user" + "strconv" "strings" "syscall" "time" @@ -34,6 +36,10 @@ func main() { "Audit log file (JSON)") readonly := flag.Bool("readonly", false, "Enable read-only mode (deny all POST/PUT/DELETE)") + listenSocketMode := flag.String("listen-socket-mode", "0660", + "Octal mode for the listening socket (ignored for fd://3)") + listenSocketGroup := flag.String("listen-socket-group", "", + "Group name or gid to own the listening socket (ignored for fd://3)") flag.Parse() if err := validateListenSocket(*listenSocket); err != nil { @@ -44,6 +50,16 @@ func main() { slog.Error(err.Error()) os.Exit(2) } + socketMode, err := parseSocketMode(*listenSocketMode) + if err != nil { + slog.Error(err.Error()) + os.Exit(2) + } + socketGID, err := resolveGroup(*listenSocketGroup) + if err != nil { + slog.Error(err.Error()) + os.Exit(2) + } ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer cancel() @@ -67,7 +83,7 @@ func main() { transport := proxy.NewTransport(*dockerHost) handler := proxy.NewHandler(router, chain, auditLog, transport) - listener, err := unixListener(*listenSocket) + listener, err := unixListener(*listenSocket, socketMode, socketGID) if err != nil { slog.Error("failed to start listener", "addr", *listenSocket, "error", err) os.Exit(1) @@ -154,11 +170,73 @@ func listenerFromFile(f *os.File) (net.Listener, error) { return l, nil } +// defaultListenSocketMode is the mode applied to the listening socket when +// --listen-socket-mode is not given. connect(2) on an AF_UNIX socket requires +// write permission, so 0660 is what actually grants the owning group access. +const defaultListenSocketMode = 0o660 + +// bindUmask is set around bind(2) so the socket is created at 0600 and is never +// briefly reachable by group or world. bind() applies 0777 &^ umask, and +// 0777 &^ 0177 == 0600. Correcting with chmod after the fact would leave a +// window in which the socket is already listening at the ambient mode. +const bindUmask = 0o177 + +// resolveGroup maps --listen-socket-group to a gid. A numeric value is used +// as-is so deployments without the group in /etc/group (or NSS) still work. +func resolveGroup(group string) (int, error) { + if group == "" { + return -1, nil + } + if gid, err := strconv.Atoi(group); err == nil { + if gid < 0 { + return -1, fmt.Errorf("--listen-socket-group %q: negative gid", group) + } + return gid, nil + } + g, err := user.LookupGroup(group) + if err != nil { + return -1, fmt.Errorf("--listen-socket-group %q: %w", group, err) + } + gid, err := strconv.Atoi(g.Gid) + if err != nil { + return -1, fmt.Errorf("--listen-socket-group %q: gid %q is not numeric", group, g.Gid) + } + return gid, nil +} + +// parseSocketMode accepts an octal mode and rejects anything world-writable. +// A world-writable socket is connectable by every local uid, which removes the +// boundary entirely, so there is deliberately no opt-out. +func parseSocketMode(s string) (os.FileMode, error) { + if s == "" { + return 0, fmt.Errorf("--listen-socket-mode must not be empty") + } + m, err := strconv.ParseUint(s, 8, 32) + if err != nil { + return 0, fmt.Errorf("--listen-socket-mode %q: not an octal mode", s) + } + if m > 0o777 { + return 0, fmt.Errorf("--listen-socket-mode %q: must be within 0777", s) + } + if m&0o002 != 0 { + return 0, fmt.Errorf("--listen-socket-mode %q is world-writable: every local user "+ + "could connect to the proxy, which disables the access-control boundary", s) + } + return os.FileMode(m), nil +} + // unixListener binds the proxy's only listening socket. Listening is Unix-socket // only by design: filesystem ownership on the socket is the access-control // boundary, and a TCP listener would have none. -func unixListener(addr string) (net.Listener, error) { +// +// The mode is set explicitly rather than inherited from the ambient umask. +// Left to the umask the socket is 0755 by default — connect(2) needs write, so +// the documented "add the caller to the socket's group" grant does not work — +// and 0777 under umask 0, which lets any local uid drive the Docker API. +func unixListener(addr string, mode os.FileMode, gid int) (net.Listener, error) { if addr == fmt.Sprintf("fd://%d", systemdSocketFD) { + // Under socket activation systemd owns the socket and applies its own + // SocketMode/SocketGroup. Re-chmod'ing it here would fight the unit. return listenerFromFile(os.NewFile(systemdSocketFD, "socket")) } @@ -176,7 +254,29 @@ func unixListener(addr string) (net.Listener, error) { return nil, fmt.Errorf("checking %s: %w", addr, err) } - return net.Listen("unix", addr) + // umask is process-global and not thread-safe. This runs during startup, + // before any request handling, so nothing else is creating files. + old := syscall.Umask(bindUmask) + l, err := net.Listen("unix", addr) + syscall.Umask(old) + if err != nil { + return nil, err + } + + // Widen from 0600 to the configured mode only after ownership is right, + // so the socket is never group-reachable by the wrong group. + if gid >= 0 { + if err := os.Chown(addr, -1, gid); err != nil { + l.Close() + return nil, fmt.Errorf("setting group on %s: %w", addr, err) + } + } + if err := os.Chmod(addr, mode); err != nil { + l.Close() + return nil, fmt.Errorf("setting mode on %s: %w", addr, err) + } + + return l, nil } // shutdownTimeout bounds how long in-flight requests are given to finish once diff --git a/go/main_test.go b/go/main_test.go index b2c7ec6..6a9662b 100644 --- a/go/main_test.go +++ b/go/main_test.go @@ -2,11 +2,14 @@ package main import ( "context" + "fmt" "net" "net/http" "os" + "os/user" "path/filepath" "strings" + "syscall" "testing" "time" ) @@ -107,7 +110,7 @@ func shortTempDir(t *testing.T) string { func TestUnixListenerBindsFreshPath(t *testing.T) { path := filepath.Join(shortTempDir(t), "fresh.sock") - l, err := unixListener(path) + l, err := unixListener(path, defaultListenSocketMode, -1) if err != nil { t.Fatalf("unixListener(%q) = %v, want nil", path, err) } @@ -139,7 +142,7 @@ func TestUnixListenerReplacesStaleSocket(t *testing.T) { } _ = stale - l, err := unixListener(path) + l, err := unixListener(path, defaultListenSocketMode, -1) if err != nil { t.Fatalf("unixListener over stale socket = %v, want nil", err) } @@ -155,7 +158,7 @@ func TestUnixListenerRefusesToDeleteNonSocket(t *testing.T) { t.Fatal(err) } - if _, err := unixListener(path); err == nil { + if _, err := unixListener(path, defaultListenSocketMode, -1); err == nil { t.Fatal("unixListener over a regular file = nil, want error") } else if !strings.Contains(err.Error(), "not a socket") { t.Fatalf("error = %q, want it to mention 'not a socket'", err) @@ -174,7 +177,7 @@ func TestUnixListenerRefusesToDeleteNonSocket(t *testing.T) { t.Fatal(err) } - if _, err := unixListener(path); err == nil { + if _, err := unixListener(path, defaultListenSocketMode, -1); err == nil { t.Fatal("unixListener over a directory = nil, want error") } if _, err := os.Stat(path); err != nil { @@ -257,7 +260,7 @@ func unixClient(path string) *http.Client { // suites assert HTTP status codes and cannot observe process lifecycle. func TestServeReturnsPromptlyWhenIdle(t *testing.T) { path := filepath.Join(shortTempDir(t), "idle.sock") - l, err := unixListener(path) + l, err := unixListener(path, defaultListenSocketMode, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -280,7 +283,7 @@ func TestServeReturnsPromptlyWhenIdle(t *testing.T) { // signal arrives must be allowed to finish. func TestServeWaitsForInFlightRequest(t *testing.T) { path := filepath.Join(shortTempDir(t), "inflight.sock") - l, err := unixListener(path) + l, err := unixListener(path, defaultListenSocketMode, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -339,7 +342,7 @@ func TestServeWaitsForInFlightRequest(t *testing.T) { // closed, not merely ignored: a connection attempt after shutdown must fail. func TestServeStopsAcceptingAfterShutdown(t *testing.T) { path := filepath.Join(shortTempDir(t), "closed.sock") - l, err := unixListener(path) + l, err := unixListener(path, defaultListenSocketMode, -1) if err != nil { t.Fatalf("unixListener: %v", err) } @@ -353,3 +356,116 @@ func TestServeStopsAcceptingAfterShutdown(t *testing.T) { t.Fatal("connected after shutdown, want the listener closed") } } + +func TestParseSocketMode(t *testing.T) { + tests := []struct { + name string + in string + want os.FileMode + wantErr string + }{ + {"default", "0660", 0o660, ""}, + {"no leading zero", "660", 0o660, ""}, + {"owner only", "0600", 0o600, ""}, + {"group read only", "0640", 0o640, ""}, + {"empty", "", 0, "must not be empty"}, + {"not octal", "0x1ff", 0, "not an octal mode"}, + {"decimal 8 is invalid octal", "668", 0, "not an octal mode"}, + {"too wide", "1777", 0, "within 0777"}, + // connect(2) needs write, so o+w means every local uid can connect. + {"world writable", "0666", 0, "world-writable"}, + {"world writable 0777", "0777", 0, "world-writable"}, + {"world writable 0602", "0602", 0, "world-writable"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseSocketMode(tt.in) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("parseSocketMode(%q) = %v, want nil", tt.in, err) + } + if got != tt.want { + t.Fatalf("parseSocketMode(%q) = %o, want %o", tt.in, got, tt.want) + } + return + } + if err == nil { + t.Fatalf("parseSocketMode(%q) = nil error, want %q", tt.in, tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("parseSocketMode(%q) error = %q, want it to contain %q", tt.in, err, tt.wantErr) + } + }) + } +} + +// TestUnixListenerAppliesMode is the regression test for #40: the mode used to +// be whatever the umask left behind, which is 0755 by default. connect(2) +// requires write permission, so the group grant the README documents silently +// did not work, and under umask 0 the socket was 0777 to every local uid. +func TestUnixListenerAppliesMode(t *testing.T) { + for _, mode := range []os.FileMode{0o660, 0o600, 0o640} { + t.Run(fmt.Sprintf("%o", mode), func(t *testing.T) { + path := filepath.Join(shortTempDir(t), "mode.sock") + l, err := unixListener(path, mode, -1) + if err != nil { + t.Fatalf("unixListener: %v", err) + } + defer l.Close() + + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != mode { + t.Fatalf("socket mode = %o, want %o", got, mode) + } + }) + } +} + +// The ambient umask must not influence the result: that was the whole bug. +func TestUnixListenerIgnoresAmbientUmask(t *testing.T) { + // umask 0 is the dangerous case — it used to produce a 0777 socket. + old := syscall.Umask(0) + defer syscall.Umask(old) + + path := filepath.Join(shortTempDir(t), "umask.sock") + l, err := unixListener(path, 0o660, -1) + if err != nil { + t.Fatalf("unixListener: %v", err) + } + defer l.Close() + + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o660 { + t.Fatalf("socket mode = %o under umask 0, want 0660", got) + } + if info.Mode().Perm()&0o002 != 0 { + t.Fatal("socket is world-writable: any local uid could connect") + } +} + +func TestResolveGroup(t *testing.T) { + if gid, err := resolveGroup(""); err != nil || gid != -1 { + t.Fatalf("resolveGroup(\"\") = %d, %v; want -1, nil", gid, err) + } + // A numeric value is taken as a gid without consulting /etc/group, so a + // container without the group defined can still be configured. + if gid, err := resolveGroup("2001"); err != nil || gid != 2001 { + t.Fatalf("resolveGroup(\"2001\") = %d, %v; want 2001, nil", gid, err) + } + if _, err := resolveGroup("definitely-no-such-group-xyz"); err == nil { + t.Fatal("resolveGroup on an unknown group = nil error, want a failure") + } + // Every Unix has gid 0 under some name; resolve it by name and check it + // round-trips to a number. + if g, err := user.LookupGroupId("0"); err == nil { + if gid, err := resolveGroup(g.Name); err != nil || gid != 0 { + t.Fatalf("resolveGroup(%q) = %d, %v; want 0, nil", g.Name, gid, err) + } + } +} diff --git a/rs/Cargo.lock b/rs/Cargo.lock index d111244..e6d0538 100644 --- a/rs/Cargo.lock +++ b/rs/Cargo.lock @@ -204,6 +204,7 @@ dependencies = [ "http-body-util", "hyper", "hyper-util", + "libc", "rand", "regex", "serde", diff --git a/rs/Cargo.toml b/rs/Cargo.toml index b3b6357..13dfae4 100644 --- a/rs/Cargo.toml +++ b/rs/Cargo.toml @@ -20,3 +20,6 @@ regex = "1" rand = "0.8" chrono = "0.4" async-trait = "0.1" +# Already present transitively via tokio; needed directly for umask(2) and +# getgrnam_r(3), neither of which std exposes. +libc = "0.2" diff --git a/rs/src/main.rs b/rs/src/main.rs index 6b6d8c9..e361cfc 100644 --- a/rs/src/main.rs +++ b/rs/src/main.rs @@ -14,7 +14,7 @@ use hyper::body::Incoming as IncomingBody; use hyper::Request; use hyper_util::rt::TokioIo; use std::io; -use std::os::unix::fs::FileTypeExt; +use std::os::unix::fs::{FileTypeExt, PermissionsExt}; use std::os::unix::io::{FromRawFd, RawFd}; use std::os::unix::net::UnixListener as StdUnixListener; use std::sync::Arc; @@ -31,6 +31,12 @@ const SYSTEMD_SOCKET_FD: RawFd = 3; /// (fd exhaustion, non-listening fd) don't spin the loop at 100% CPU. const ACCEPT_ERROR_BACKOFF: std::time::Duration = std::time::Duration::from_millis(100); +/// Set around bind(2) so the socket is created at 0600 and is never briefly +/// reachable by group or world. bind() applies 0777 & !umask, and +/// 0777 & !0177 == 0600. A chmod after the fact would leave a window in which +/// the socket is already listening at the ambient mode. +const BIND_UMASK: libc::mode_t = 0o177; + /// Upper bound on how long in-flight requests are given to finish after a /// shutdown signal. This is a cap, not a delay: shutdown returns as soon as /// the last connection closes. Matches the Go and TypeScript implementations. @@ -53,6 +59,14 @@ struct Cli { #[arg(long, default_value_t = false)] readonly: bool, + + /// Octal mode for the listening socket (ignored for fd://3). + #[arg(long, default_value = "0660")] + listen_socket_mode: String, + + /// Group name or gid to own the listening socket (ignored for fd://3). + #[arg(long)] + listen_socket_group: Option, } #[tokio::main] @@ -71,6 +85,20 @@ async fn main() -> Result<(), Box> { tracing::error!("{}", msg); std::process::exit(2); } + let socket_mode = match parse_socket_mode(&cli.listen_socket_mode) { + Ok(m) => m, + Err(msg) => { + tracing::error!("{}", msg); + std::process::exit(2); + } + }; + let socket_gid = match cli.listen_socket_group.as_deref().map(resolve_group).transpose() { + Ok(g) => g, + Err(msg) => { + tracing::error!("{}", msg); + std::process::exit(2); + } + }; let policy_manager = policy::Manager::new(&cli.config_dir)?; tracing::info!("loaded {} policies", policy_manager.list().len()); @@ -116,7 +144,7 @@ async fn main() -> Result<(), Box> { // Bind before spawning so a bind failure is fatal: the Unix socket is the // process's only listener, so a running-but-unbound proxy is never useful. - let listener = bind_unix_listener(&cli.listen_socket).map_err(|e| { + let listener = bind_unix_listener(&cli.listen_socket, socket_mode, socket_gid).map_err(|e| { tracing::error!("failed to bind unix socket {}: {}", cli.listen_socket, e); e })?; @@ -144,8 +172,10 @@ async fn main() -> Result<(), Box> { /// and listening; we just adopt the fd). Any other value is treated as a /// filesystem path: a stale socket left over from a previous run is removed /// before binding, matching the Go implementation. -fn bind_unix_listener(addr: &str) -> io::Result { +fn bind_unix_listener(addr: &str, mode: u32, gid: Option) -> io::Result { if addr == "fd://3" { + // Under socket activation systemd owns the socket and applies its own + // SocketMode/SocketGroup; re-chmod'ing here would fight the unit. return unix_listener_from_raw_fd(SYSTEMD_SOCKET_FD); } @@ -165,7 +195,87 @@ fn bind_unix_listener(addr: &str) -> io::Result { Err(e) if e.kind() == io::ErrorKind::NotFound => {} Err(e) => return Err(e), } - tokio::net::UnixListener::bind(addr) + + // umask is process-global and not thread-safe. This runs during startup, + // before any connection is served, so nothing else is creating files. + // SAFETY: umask(2) cannot fail and has no preconditions. + let previous = unsafe { libc::umask(BIND_UMASK) }; + let listener = tokio::net::UnixListener::bind(addr); + // SAFETY: as above; restores the caller's umask. + unsafe { libc::umask(previous) }; + let listener = listener?; + + // Widen from 0600 to the configured mode only once ownership is correct, + // so the socket is never reachable by the wrong group. + if let Some(gid) = gid { + std::os::unix::fs::chown(addr, None, Some(gid)).map_err(|e| { + io::Error::new(e.kind(), format!("setting group on {}: {}", addr, e)) + })?; + } + std::fs::set_permissions(addr, std::fs::Permissions::from_mode(mode)) + .map_err(|e| io::Error::new(e.kind(), format!("setting mode on {}: {}", addr, e)))?; + + Ok(listener) +} + +/// Maps `--listen-socket-group` to a gid. A numeric value is used as-is so a +/// deployment without the group in /etc/group (or NSS) can still be configured. +fn resolve_group(group: &str) -> Result { + if let Ok(gid) = group.parse::() { + return Ok(gid); + } + let name = std::ffi::CString::new(group) + .map_err(|_| format!("--listen-socket-group {:?}: contains a NUL byte", group))?; + + // getgrnam_r is the reentrant form; the non-_r variant returns a pointer + // into a shared static buffer. + let mut grp: libc::group = unsafe { std::mem::zeroed() }; + let mut buf = vec![0_i8; 4096]; + let mut result: *mut libc::group = std::ptr::null_mut(); + // SAFETY: all pointers are valid for the duration of the call and the + // buffer length matches the allocation. + let rc = unsafe { + libc::getgrnam_r( + name.as_ptr(), + &mut grp, + buf.as_mut_ptr() as *mut libc::c_char, + buf.len(), + &mut result, + ) + }; + if rc != 0 { + return Err(format!( + "--listen-socket-group {:?}: lookup failed: {}", + group, + io::Error::from_raw_os_error(rc) + )); + } + if result.is_null() { + return Err(format!("--listen-socket-group {:?}: no such group", group)); + } + Ok(grp.gr_gid) +} + +/// Parses an octal mode and rejects anything world-writable. connect(2) on an +/// AF_UNIX socket requires write permission, so a world-writable socket is +/// reachable by every local uid — there is deliberately no opt-out. +fn parse_socket_mode(s: &str) -> Result { + if s.is_empty() { + return Err("--listen-socket-mode must not be empty".to_string()); + } + let mode = u32::from_str_radix(s, 8) + .map_err(|_| format!("--listen-socket-mode {:?}: not an octal mode", s))?; + if mode > 0o777 { + return Err(format!("--listen-socket-mode {:?}: must be within 0777", s)); + } + if mode & 0o002 != 0 { + return Err(format!( + "--listen-socket-mode {:?} is world-writable: every local user could connect \ + to the proxy, which disables the access-control boundary", + s + )); + } + Ok(mode) } /// Rejects `--listen-socket` values that would not produce a filesystem-visible @@ -392,7 +502,7 @@ mod tests { drop(stale); assert!(path.exists(), "precondition: stale socket should still be on disk"); - let result = bind_unix_listener(path.to_str().unwrap()); + let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); assert!( result.is_ok(), "expected stale socket to be removed and bind to succeed: {:?}", @@ -408,7 +518,7 @@ mod tests { let path = unique_socket_path(); std::fs::write(&path, b"important data").unwrap(); - let result = bind_unix_listener(path.to_str().unwrap()); + let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); assert!(result.is_err(), "expected a regular file to be refused, not deleted"); assert!( result.unwrap_err().to_string().contains("not a socket"), @@ -477,7 +587,7 @@ mod tests { async fn test_bind_unix_listener_binds_fresh_path() { let path = unique_socket_path(); - let result = bind_unix_listener(path.to_str().unwrap()); + let result = bind_unix_listener(path.to_str().unwrap(), 0o660, None); assert!(result.is_ok(), "expected bind to a fresh path to succeed: {:?}", result.err()); assert!(path.exists(), "expected socket file to be created"); @@ -566,7 +676,7 @@ mod tests { #[tokio::test] async fn test_listener_shuts_down_promptly_when_idle() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap()).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -592,7 +702,7 @@ mod tests { #[tokio::test] async fn test_listener_drains_in_flight_request() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap()).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -628,12 +738,81 @@ mod tests { std::fs::remove_file(&path).ok(); } + #[test] + fn test_parse_socket_mode() { + for (input, want) in [("0660", 0o660), ("660", 0o660), ("0600", 0o600), ("0640", 0o640)] { + assert_eq!(parse_socket_mode(input), Ok(want), "{} should parse", input); + } + for (input, want) in [ + ("", "must not be empty"), + ("0x1ff", "not an octal mode"), + ("668", "not an octal mode"), + ("1777", "within 0777"), + // connect(2) needs write, so o+w means every local uid can connect. + ("0666", "world-writable"), + ("0777", "world-writable"), + ("0602", "world-writable"), + ] { + let err = parse_socket_mode(input) + .expect_err(&format!("{:?} should be rejected", input)); + assert!(err.contains(want), "error for {:?} was {:?}", input, err); + } + } + + /// Regression test for #40: the mode used to be whatever the umask left + /// behind, which is 0755 by default. connect(2) requires write permission, + /// so the documented group grant silently did not work, and under umask 0 + /// the socket was 0777 to every local uid. + #[tokio::test] + async fn test_bind_applies_socket_mode() { + for mode in [0o660_u32, 0o600, 0o640] { + let path = unique_socket_path(); + let listener = bind_unix_listener(path.to_str().unwrap(), mode, None).unwrap(); + + let got = std::fs::symlink_metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(got, mode, "socket mode was {:o}, want {:o}", got, mode); + + drop(listener); + std::fs::remove_file(&path).ok(); + } + } + + /// The ambient umask must not influence the result: that was the bug. + #[tokio::test] + async fn test_bind_ignores_ambient_umask() { + // SAFETY: umask(2) cannot fail. Restored below. + let previous = unsafe { libc::umask(0) }; + let path = unique_socket_path(); + let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); + // SAFETY: as above. + unsafe { libc::umask(previous) }; + + let got = std::fs::symlink_metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(got, 0o660, "socket mode was {:o} under umask 0, want 0660", got); + assert_eq!(got & 0o002, 0, "socket is world-writable: any local uid could connect"); + + drop(listener); + std::fs::remove_file(&path).ok(); + } + + #[test] + fn test_resolve_group() { + // A numeric value is taken as a gid without consulting /etc/group. + assert_eq!(resolve_group("2001"), Ok(2001)); + assert!(resolve_group("definitely-no-such-group-xyz").is_err()); + // Whatever gid 0 is called on this platform must round-trip. + let root_group = if cfg!(target_os = "macos") { "wheel" } else { "root" }; + if let Ok(gid) = resolve_group(root_group) { + assert_eq!(gid, 0, "{} should be gid 0", root_group); + } + } + /// A clean shutdown must not leave the socket file on disk, matching Go /// and TypeScript. tokio does not unlink on drop, so this is explicit. #[tokio::test] async fn test_listener_unlinks_socket_on_shutdown() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap()).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( @@ -677,7 +856,7 @@ mod tests { #[tokio::test] async fn test_listener_releases_idle_keepalive_connection() { let path = unique_socket_path(); - let listener = bind_unix_listener(path.to_str().unwrap()).unwrap(); + let listener = bind_unix_listener(path.to_str().unwrap(), 0o660, None).unwrap(); let (tx, rx) = broadcast::channel::<()>(1); let handle = spawn_unix_listener( diff --git a/ts/src/flags.test.ts b/ts/src/flags.test.ts index 9b2b5c9..673c40a 100644 --- a/ts/src/flags.test.ts +++ b/ts/src/flags.test.ts @@ -1,10 +1,15 @@ import { describe, it } from "node:test"; import assert from "node:assert/strict"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { getFlag, hasFlag, parseListenSocket, + parseSocketMode, parseSocketPath, + resolveGroup, validateFlags, } from "./flags.js"; @@ -188,4 +193,78 @@ describe("flags", () => { assert.match(parseSocketPath("") ?? "", /must not be empty/); }); }); -}); \ No newline at end of file +}); + +describe("parseSocketMode", () => { + it("accepts octal modes with and without a leading zero", () => { + for (const [input, want] of [ + ["0660", 0o660], + ["660", 0o660], + ["0600", 0o600], + ["0640", 0o640], + ] as const) { + const got = parseSocketMode(input); + assert.deepEqual(got, { mode: want }, `${input} should parse`); + } + }); + + // connect(2) needs write permission, so o+w means every local uid can + // connect. There is deliberately no opt-out for this. + it("rejects world-writable modes", () => { + for (const input of ["0666", "0777", "0602"]) { + const got = parseSocketMode(input); + assert.ok("error" in got, `${input} should be rejected`); + assert.match(got.error, /world-writable/); + } + }); + + it("rejects malformed and out-of-range modes", () => { + for (const [input, want] of [ + ["", /must not be empty/], + ["0x1ff", /not an octal mode/], + ["668", /not an octal mode/], + ["1777", /within 0777/], + ] as const) { + const got = parseSocketMode(input); + assert.ok("error" in got, `${input} should be rejected`); + assert.match(got.error, want); + } + }); +}); + +describe("resolveGroup", () => { + function groupFile(contents: string): string { + const p = join(mkdtempSync(join(tmpdir(), "grp-")), "group"); + writeFileSync(p, contents); + return p; + } + + it("takes a numeric value as a gid without consulting the group file", () => { + assert.deepEqual(resolveGroup("2001", "/nonexistent"), { gid: 2001 }); + }); + + it("resolves a name from the group file", () => { + const f = groupFile("root:x:0:\ndocker:x:999:alice,bob\n"); + assert.deepEqual(resolveGroup("docker", f), { gid: 999 }); + }); + + it("reports an unknown group", () => { + const f = groupFile("root:x:0:\n"); + const got = resolveGroup("nope", f); + assert.ok("error" in got); + assert.match(got.error, /no such group/); + }); + + it("reports a non-numeric gid field", () => { + const f = groupFile("broken:x:notanumber:\n"); + const got = resolveGroup("broken", f); + assert.ok("error" in got); + assert.match(got.error, /not numeric/); + }); + + it("reports an unreadable group file", () => { + const got = resolveGroup("docker", "/nonexistent-group-file"); + assert.ok("error" in got); + assert.match(got.error, /cannot read/); + }); +}); diff --git a/ts/src/flags.ts b/ts/src/flags.ts index f3338ca..03e8595 100644 --- a/ts/src/flags.ts +++ b/ts/src/flags.ts @@ -1,6 +1,8 @@ // Minimal CLI flag parser supporting both "--name value" and "--name=value" // forms, matching Go's flag package and Rust's clap behavior. +import { readFileSync } from "node:fs"; + export function getFlag( args: string[], name: string, @@ -110,6 +112,85 @@ export function parseListenSocket(input: string): ListenTarget { return { kind: "path", path: input }; } +// Mode applied to the listening socket when --listen-socket-mode is omitted. +// connect(2) on an AF_UNIX socket requires write permission, so 0o660 is what +// actually grants the owning group access. +export const DEFAULT_LISTEN_SOCKET_MODE = 0o660; + +// Set around bind(2) so the socket is created at 0600 and is never briefly +// reachable by group or world. bind() applies 0777 & ~umask, and +// 0777 & ~0177 === 0600. Correcting with chmod afterwards would leave a window +// in which the socket is already listening at the ambient mode. +export const BIND_UMASK = 0o177; + +export type SocketMode = { mode: number } | { error: string }; + +// Parses an octal mode and rejects anything world-writable. A world-writable +// socket is connectable by every local uid, which removes the boundary +// entirely, so there is deliberately no opt-out. +export function parseSocketMode(input: string): SocketMode { + if (input.length === 0) { + return { error: "--listen-socket-mode must not be empty" }; + } + if (!/^[0-7]+$/.test(input)) { + return { error: `--listen-socket-mode ${JSON.stringify(input)}: not an octal mode` }; + } + const mode = parseInt(input, 8); + if (mode > 0o777) { + return { error: `--listen-socket-mode ${JSON.stringify(input)}: must be within 0777` }; + } + if (mode & 0o002) { + return { + error: + `--listen-socket-mode ${JSON.stringify(input)} is world-writable: every local user ` + + `could connect to the proxy, which disables the access-control boundary`, + }; + } + return { mode }; +} + +export type GroupId = { gid: number } | { error: string }; + +// Maps --listen-socket-group to a gid. A numeric value is used as-is so a +// deployment without the group defined can still be configured. +// +// Node exposes no getgrnam equivalent, so a name is resolved by reading +// /etc/group. That covers the container case this proxy targets but not +// NSS-backed directories (LDAP, SSSD) — pass a numeric gid for those. Go uses +// os/user.LookupGroup and Rust uses getgrnam_r, both of which do consult NSS. +export function resolveGroup(input: string, groupFile = "/etc/group"): GroupId { + if (/^\d+$/.test(input)) { + return { gid: parseInt(input, 10) }; + } + let contents: string; + try { + contents = readFileSync(groupFile, "utf8"); + } catch (err) { + const e = err as NodeJS.ErrnoException; + return { + error: `--listen-socket-group ${JSON.stringify(input)}: cannot read ${groupFile}: ${e.message}`, + }; + } + for (const line of contents.split("\n")) { + // name:password:gid:members + const parts = line.split(":"); + if (parts.length >= 3 && parts[0] === input) { + const gid = parseInt(parts[2], 10); + if (Number.isNaN(gid)) { + return { + error: `--listen-socket-group ${JSON.stringify(input)}: gid ${JSON.stringify(parts[2])} is not numeric`, + }; + } + return { gid }; + } + } + return { + error: + `--listen-socket-group ${JSON.stringify(input)}: no such group in ${groupFile} ` + + `(Node cannot query NSS; pass a numeric gid if the group is not in ${groupFile})`, + }; +} + // Validates a Docker daemon address supplied via --docker-host. Only Unix // socket paths are accepted: connecting to the daemon over TCP would bypass // the Linux user/group ownership on the socket, which is the security model diff --git a/ts/src/index.ts b/ts/src/index.ts index d7a3415..4e2455e 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -7,17 +7,27 @@ import { Router } from "./proxy.js"; import { Handler } from "./handler.js"; import { Transport } from "./transport.js"; import { createShutdown } from "./shutdown.js"; +import { listenOnSocket } from "./listen.js"; import { getFlag, hasFlag, parseListenSocket, + parseSocketMode, parseSocketPath, + resolveGroup, validateFlags, } from "./flags.js"; const args = process.argv.slice(2); -const VALUE_FLAGS = ["--listen-socket", "--docker-host", "--config-dir", "--log-file"]; +const VALUE_FLAGS = [ + "--listen-socket", + "--docker-host", + "--config-dir", + "--log-file", + "--listen-socket-mode", + "--listen-socket-group", +]; const BOOL_FLAGS = ["--readonly"]; const flagError = validateFlags(args, VALUE_FLAGS, BOOL_FLAGS); @@ -38,6 +48,24 @@ if (listenTarget.kind === "error") { console.error(listenTarget.message); process.exit(2); } +const parsedMode = parseSocketMode(getFlag(args, "--listen-socket-mode", "0660")); +if ("error" in parsedMode) { + console.error(parsedMode.error); + process.exit(2); +} +const socketMode = parsedMode.mode; + +const groupFlag = getFlag(args, "--listen-socket-group", ""); +let socketGid: number | undefined; +if (groupFlag !== "") { + const resolved = resolveGroup(groupFlag); + if ("error" in resolved) { + console.error(resolved.error); + process.exit(2); + } + socketGid = resolved.gid; +} + const configDir = getFlag(args, "--config-dir", "/etc/docker-socket-policy/services"); const logFile = getFlag(args, "--log-file", "/var/log/docker-socket-policy.log"); const readonly = hasFlag(args, "--readonly"); @@ -111,9 +139,17 @@ if (listenTarget.kind === "fd") { process.exit(1); } } - server.listen(path, () => { - console.log(`listening on unix socket ${path}`); - }); + listenOnSocket(server, path, socketMode, socketGid).then( + () => { + console.log( + `listening on unix socket ${path} (mode ${socketMode.toString(8).padStart(4, "0")})`, + ); + }, + (err: NodeJS.ErrnoException) => { + console.error(`failed to listen on ${path}: ${err.message}`); + process.exit(1); + }, + ); } const shutdown = createShutdown(server); diff --git a/ts/src/listen.test.ts b/ts/src/listen.test.ts new file mode 100644 index 0000000..7b859d5 --- /dev/null +++ b/ts/src/listen.test.ts @@ -0,0 +1,117 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { createServer } from "node:http"; +import { mkdtempSync, rmSync, statSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { listenOnSocket } from "./listen.js"; + +// sun_path is capped at 104 bytes on macOS, so keep the path short. +function tempSocket(): { path: string; cleanup: () => void } { + const dir = mkdtempSync(join(tmpdir(), "lsn-")); + return { + path: join(dir, "s.sock"), + cleanup: () => rmSync(dir, { recursive: true, force: true }), + }; +} + +function modeOf(path: string): number { + return statSync(path).mode & 0o777; +} + +describe("listenOnSocket", () => { + // Regression test for #40: the mode used to be whatever the umask left + // behind, which is 0755 by default. connect(2) requires write permission, so + // the documented group grant silently did not work. + it("applies the requested mode", async () => { + for (const mode of [0o660, 0o600, 0o640]) { + const { path, cleanup } = tempSocket(); + const server = createServer(() => {}); + await listenOnSocket(server, path, mode); + + assert.equal( + modeOf(path), + mode, + `socket mode was ${modeOf(path).toString(8)}, want ${mode.toString(8)}`, + ); + + await new Promise((r) => server.close(r)); + cleanup(); + } + }); + + // The ambient umask must not influence the result: that was the whole bug. + it("ignores the ambient umask", async () => { + const previous = process.umask(0); + const { path, cleanup } = tempSocket(); + const server = createServer(() => {}); + try { + await listenOnSocket(server, path, 0o660); + } finally { + process.umask(previous); + } + + assert.equal( + modeOf(path), + 0o660, + `socket mode was ${modeOf(path).toString(8)} under umask 0, want 660`, + ); + assert.equal( + modeOf(path) & 0o002, + 0, + "socket is world-writable: any local uid could connect", + ); + + await new Promise((r) => server.close(r)); + cleanup(); + }); + + it("restores the previous umask", async () => { + const { path, cleanup } = tempSocket(); + const before = process.umask(); + const server = createServer(() => {}); + await listenOnSocket(server, path, 0o660); + + assert.equal(process.umask(), before, "umask was not restored after bind"); + + await new Promise((r) => server.close(r)); + cleanup(); + }); + + it("rejects, and restores the umask, when the bind fails", async () => { + const before = process.umask(); + const server = createServer(() => {}); + + await assert.rejects( + () => listenOnSocket(server, "/nonexistent-dir-xyz/s.sock", 0o660), + /ENOENT|EACCES/, + ); + assert.equal(process.umask(), before, "umask was not restored after a failed bind"); + }); + + it("is actually connectable at the mode it sets", async () => { + const { path, cleanup } = tempSocket(); + const server = createServer((_req, res) => res.end("ok")); + await listenOnSocket(server, path, 0o660); + + const { connect } = await import("node:net"); + const reply = await new Promise((resolve, reject) => { + const sock = connect(path, () => { + sock.write("GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"); + }); + let buf = ""; + sock.on("data", (d) => { + buf += d.toString(); + }); + sock.on("error", reject); + setTimeout(() => { + sock.destroy(); + resolve(buf); + }, 300).unref(); + }); + assert.match(reply, /200 OK/); + + await new Promise((r) => server.close(r)); + cleanup(); + }); +}); diff --git a/ts/src/listen.ts b/ts/src/listen.ts new file mode 100644 index 0000000..defe57c --- /dev/null +++ b/ts/src/listen.ts @@ -0,0 +1,56 @@ +import type { Server } from "node:http"; +import { chmodSync, chownSync } from "node:fs"; +import { BIND_UMASK } from "./flags.js"; + +/** + * Binds `server` to a Unix socket path with an explicit mode and group. + * + * Extracted from index.ts so it can be tested: the mode of the listening + * socket is invisible to the integration suite, which only observes HTTP + * status codes. + * + * The mode is set rather than inherited from the ambient umask. Left to the + * umask the socket is 0755 by default — connect(2) needs write permission, so + * the documented "add the caller to the socket's group" grant does not work — + * and 0777 under umask 0, which lets any local uid drive the Docker API. + * + * The umask is narrowed around bind(2) rather than the mode being corrected + * afterwards, because a chmod after bind leaves a window in which the socket is + * already listening at the ambient mode. The window here is at 0600 instead, + * which is more restrictive than any mode we would set. + */ +export function listenOnSocket( + server: Server, + path: string, + mode: number, + gid?: number, +): Promise { + return new Promise((resolve, reject) => { + // umask is process-global. This runs during startup, before any request is + // served, so nothing else is creating files in the window. + const previousUmask = process.umask(BIND_UMASK); + + const onError = (err: Error) => { + process.umask(previousUmask); + reject(err); + }; + server.once("error", onError); + + server.listen(path, () => { + process.umask(previousUmask); + server.off("error", onError); + try { + // Set the group before widening the mode, so the socket is never + // reachable by the wrong group. + if (gid !== undefined) { + chownSync(path, -1, gid); + } + chmodSync(path, mode); + } catch (err) { + reject(err); + return; + } + resolve(); + }); + }); +}