From c6a1240e469ff8170cf31b39a01c1cb08fdb86f4 Mon Sep 17 00:00:00 2001 From: James O'Doherty Date: Fri, 29 May 2026 19:41:28 -0400 Subject: security, refactor: resolve critical namespace escapes, path traversal, concurrency races, and resource leaks This commit addresses several security vulnerabilities, undefined behaviors, race conditions, and resource leaks across the application: 1. Path Traversal & Arbitrary File/Directory Actions: - Implemented `IsValidProfileName` in `internal/cli/cli.go` to restrict profile names to alphanumeric characters, dashes, and underscores. - Applied validation to all CLI paths (`--profile`, `import`, `configure`, `delete`, `stop`) to prevent directory traversal and unauthorized directory or file creations/deletions. - Added `TestIsValidProfileName` in `internal/cli/cli_test.go`. 2. Network Namespace Escape via Compromised Thread recycling: - Fixed `HostBind.Open` in `internal/wireguard/wireguard.go` to panic immediately instead of returning an error if restoring the isolated namespace fails. This prevents Go from returning the compromised thread (still in host namespace) to the runtime pool. 3. Concurrency Race Conditions & Thread Migration: - Added `runtime.LockOSThread()` in `JoinExistingNamespace` (`internal/namespace/pinning.go`) to ensure the goroutine stays on the modified namespace thread before executing the command. - Implemented profile locking using advisory file locks (`unix.Flock`) on a `.lock` file in the user's runtime directory (with platform stubs in `internal/namespace/lock_linux.go` and `internal/namespace/lock_stub.go`). - Integrated locking during `App.Run` and `App.ExecuteCommand`, releasing the lock right before spawning the wrapped process. 4. File Descriptor Leaks on Bootstrap Failures: - Refactored `Bootstrap()` in `internal/namespace/namespace.go` to use named return values and a deferred cleanup loop that closes `execFd`, `hostNetFd`, and the duplicated `hostSocketFd` if `syscall.Exec` fails. - Added an explicit `conn.Close()` on the original socket connection after duplication. 5. Glibc Undefined Behavior / Crash on argc == 0: - Corrected `internal/namespace/launcher_src/launcher.c` to not reference `argv[0]` when `argc < 1`. Recompiled `internal/namespace/launcher.bin`. 6. DNS Fallback Usability & Import Safety: - Added validation in `ExecuteCommand` to issue a warning when falling back to `1.1.1.1` if the configuration does not route all traffic (`0.0.0.0/0` or `::/0`). - Prevented silent overwrites in `handleProfileImport` if the destination profile already exists, and added a corresponding unit test verifying failure. --- internal/namespace/pinning.go | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'internal/namespace/pinning.go') diff --git a/internal/namespace/pinning.go b/internal/namespace/pinning.go index eb0a376..2433203 100644 --- a/internal/namespace/pinning.go +++ b/internal/namespace/pinning.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strconv" "syscall" @@ -84,6 +85,10 @@ func JoinExistingNamespace(pm *paths.PathManager, profile string) (bool, error) return false, nil } + // Lock the OS thread before joining namespaces to ensure this goroutine stays on the modified thread, + // and that the thread is not reused for other goroutines (since we never unlock it). + runtime.LockOSThread() + // Join User Namespace first userNsPath := fmt.Sprintf("/proc/%d/ns/user", activePid) userFd, err := os.Open(userNsPath) -- cgit v1.2.3