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(); + }); + }); +}