diff options
| author | James O'Doherty <james@theodohertyfamily.com> | 2026-05-29 19:41:28 -0400 |
|---|---|---|
| committer | James O'Doherty <james@theodohertyfamily.com> | 2026-05-29 19:41:28 -0400 |
| commit | c6a1240e469ff8170cf31b39a01c1cb08fdb86f4 (patch) | |
| tree | 9e7f8750e43996496eef2fabbebb6ee9386dc020 /internal/namespace | |
| parent | edf4e0f0380b6662ba88cfa00d2d2ff5a43032de (diff) | |
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.
Diffstat (limited to 'internal/namespace')
| -rw-r--r-- | internal/namespace/launcher_src/launcher.c | 2 | ||||
| -rw-r--r-- | internal/namespace/lock_linux.go | 40 | ||||
| -rw-r--r-- | internal/namespace/lock_stub.go | 18 | ||||
| -rw-r--r-- | internal/namespace/namespace.go | 26 | ||||
| -rw-r--r-- | internal/namespace/pinning.go | 5 |
5 files changed, 84 insertions, 7 deletions
diff --git a/internal/namespace/launcher_src/launcher.c b/internal/namespace/launcher_src/launcher.c index e108da6..7bbbce7 100644 --- a/internal/namespace/launcher_src/launcher.c +++ b/internal/namespace/launcher_src/launcher.c @@ -9,7 +9,7 @@ int main(int argc, char **argv) { if (argc < 1) { - fprintf(stderr, "Usage: %s <command> [args...]\n", argv[0]); + fprintf(stderr, "Usage: launcher <command> [args...]\n"); return 1; } diff --git a/internal/namespace/lock_linux.go b/internal/namespace/lock_linux.go new file mode 100644 index 0000000..8da98f6 --- /dev/null +++ b/internal/namespace/lock_linux.go @@ -0,0 +1,40 @@ +//go:build linux + +package namespace + +import ( + "fmt" + "os" + "path/filepath" + + "git.theodohertyfamily.com/tools/wg-wrap/internal/paths" + "golang.org/x/sys/unix" +) + +// AcquireProfileLock locks the profile to prevent concurrent startup races. +func AcquireProfileLock(pm *paths.PathManager, profile string) (*os.File, error) { + lockPath := filepath.Join(pm.RuntimeBaseDir(), "profiles", profile+".lock") + if err := os.MkdirAll(filepath.Dir(lockPath), 0755); err != nil { + return nil, fmt.Errorf("failed to create lock directory: %w", err) + } + + file, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0600) + if err != nil { + return nil, fmt.Errorf("failed to open lock file: %w", err) + } + + if err := unix.Flock(int(file.Fd()), unix.LOCK_EX); err != nil { + _ = file.Close() + return nil, fmt.Errorf("failed to lock profile: %w", err) + } + + return file, nil +} + +// ReleaseProfileLock unlocks the profile. +func ReleaseProfileLock(file *os.File) { + if file != nil { + _ = unix.Flock(int(file.Fd()), unix.LOCK_UN) + _ = file.Close() + } +} diff --git a/internal/namespace/lock_stub.go b/internal/namespace/lock_stub.go new file mode 100644 index 0000000..2282852 --- /dev/null +++ b/internal/namespace/lock_stub.go @@ -0,0 +1,18 @@ +//go:build !linux + +package namespace + +import ( + "os" + + "git.theodohertyfamily.com/tools/wg-wrap/internal/paths" +) + +// AcquireProfileLock is a stub for non-Linux platforms. +func AcquireProfileLock(pm *paths.PathManager, profile string) (*os.File, error) { + return nil, nil +} + +// ReleaseProfileLock is a stub for non-Linux platforms. +func ReleaseProfileLock(file *os.File) { +} diff --git a/internal/namespace/namespace.go b/internal/namespace/namespace.go index e2ef2f1..ab3797d 100644 --- a/internal/namespace/namespace.go +++ b/internal/namespace/namespace.go @@ -69,11 +69,20 @@ func VerifyArguments(args []string) error { // Bootstrap ensures the process is running in an isolated user and network namespace. // It writes the embedded C launcher to a temporary file and replaces the current process. -func Bootstrap() error { +func Bootstrap() (err error) { if IsIsolated() { return nil } + var fdsToClose []int + defer func() { + if err != nil { + for _, fd := range fdsToClose { + _ = syscall.Close(fd) + } + } + }() + // 0. Validate current arguments for null bytes before proceeding. // If any argument contains a null byte, syscall.Exec will fail with 'invalid argument'. for i, arg := range os.Args { @@ -118,6 +127,7 @@ func Bootstrap() error { _ = os.Remove(launcherPath) return fmt.Errorf("failed to open launcher for exec: %w", err) } + fdsToClose = append(fdsToClose, execFd) // Close the write file descriptor (to avoid ETXTBSY) _ = tmpFile.Close() @@ -152,6 +162,8 @@ func Bootstrap() error { if err != nil { return fmt.Errorf("failed to open host netns: %w", err) } + fdsToClose = append(fdsToClose, hostNetFd) + // Clear close-on-exec so it remains open across syscall.Exec if flags, err := unix.FcntlInt(uintptr(hostNetFd), unix.F_GETFD, 0); err == nil { _, _ = unix.FcntlInt(uintptr(hostNetFd), unix.F_SETFD, flags&^unix.FD_CLOEXEC) @@ -160,15 +172,17 @@ func Bootstrap() error { env := append(os.Environ(), fmt.Sprintf("WG_WRAP_HOST_NETNS_FD=%d", hostNetFd)) // Open a host UDP socket on 0.0.0.0:0 before unsharing network namespace. - laddr, err := net.ResolveUDPAddr("udp", "0.0.0.0:0") - if err == nil { - if conn, err := net.ListenUDP("udp", laddr); err == nil { - if file, err := conn.File(); err == nil { + laddr, errAddr := net.ResolveUDPAddr("udp", "0.0.0.0:0") + if errAddr == nil { + if conn, errConn := net.ListenUDP("udp", laddr); errConn == nil { + if file, errFile := conn.File(); errFile == nil { hostSocketFd := file.Fd() - if flags, err := unix.FcntlInt(hostSocketFd, unix.F_GETFD, 0); err == nil { + if flags, fcntlErr := unix.FcntlInt(hostSocketFd, unix.F_GETFD, 0); fcntlErr == nil { _, _ = unix.FcntlInt(hostSocketFd, unix.F_SETFD, flags&^unix.FD_CLOEXEC) } env = append(env, fmt.Sprintf("WG_WRAP_HOST_SOCKET_FD=%d", hostSocketFd)) + fdsToClose = append(fdsToClose, int(hostSocketFd)) + _ = conn.Close() } } } 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) |
