diff options
| -rw-r--r-- | AGENTS.md | 5 | ||||
| -rw-r--r-- | Makefile | 5 | ||||
| -rw-r--r-- | internal/cli/cli.go | 57 | ||||
| -rw-r--r-- | internal/wireguard/wireguard.go | 58 | ||||
| -rw-r--r-- | tests/e2e/race_test.go | 94 | ||||
| -rw-r--r-- | tests/e2e/vulnerability_test.go | 109 |
6 files changed, 300 insertions, 28 deletions
@@ -35,7 +35,10 @@ No piece of code is considered "done" until it has passed the full verification 1. **Formatting**: `go fmt ./...` 2. **Static Analysis**: `go vet ./...` 3. **Linting**: `golangci-lint run` -4. **Verification**: `go test -timeout 30s ./...` +4. **Verification**: Run the test suite via the Makefile: + - **All tests**: `make test` + - **Specific tests**: `make test TEST_ARGS="-run TestName"` (e.g., `make test TEST_ARGS="-run TestDNSLeak"`) + - **Custom flags**: `make test TEST_ARGS="-count=10"` If any of these tools report an error or warning, the code must be corrected before the task is marked as complete. @@ -27,10 +27,13 @@ $(BINARY): $(LAUNCHER_BIN) $(LAUNCHER_BIN): $(LAUNCHER_SRC) $(CC) $(CFLAGS) $(LAUNCHER_SRC) -o $(LAUNCHER_BIN) +# Test arguments (can be overridden from CLI: make test TEST_ARGS="-run TestName") +TEST_ARGS ?= -timeout 30s + # Run tests test: $(BINARY) @echo "Running tests with WG_WRAP_BIN=$(shell pwd)/$(BINARY)" - WG_WRAP_BIN=$(shell pwd)/$(BINARY) go test -v -race ./... + WG_WRAP_BIN=$(shell pwd)/$(BINARY) go test -v -race $(TEST_ARGS) ./... # Run fuzzing tests fuzz: clean diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 85b9ae3..9b3409e 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -159,25 +159,39 @@ func (a *App) ExecuteCommand(cfg *config.Config) error { // Acquire execution lock during configuration and startup inside the namespace lockFile, lockErr := namespace.AcquireProfileLock(pm, cfg.Profile) + if lockErr == nil { + defer namespace.ReleaseProfileLock(lockFile) + } if err := namespace.PruneStalePids(pm, cfg.Profile); err != nil { fmt.Printf("failed to prune stale pids: %v\n", err) } if err := namespace.RegisterProcess(pm, cfg.Profile); err != nil { - if lockErr == nil { - namespace.ReleaseProfileLock(lockFile) - } return fmt.Errorf("failed to register process: %w", err) } defer func() { - if err := namespace.UnregisterProcess(pm, cfg.Profile); err != nil { - fmt.Printf("failed to unregister process: %v\n", err) - } - if last, err := namespace.IsLastProcess(pm, cfg.Profile); err == nil && last { - fmt.Printf("Last process exiting. Cleaning up profile %s...\n", cfg.Profile) - if err := namespace.UnpinNamespace(pm, cfg.Profile); err != nil { - fmt.Printf("failed to unpin namespace: %v\n", err) + // Re-acquire lock for the entire cleanup sequence to ensure atomic unregister and unpin + cleanupLock, cleanupErr := namespace.AcquireProfileLock(pm, cfg.Profile) + if cleanupErr == nil { + // Check if we are the last active process before unregistering + last, lastErr := namespace.IsLastProcess(pm, cfg.Profile) + + if err := namespace.UnregisterProcess(pm, cfg.Profile); err != nil { + fmt.Printf("failed to unregister process: %v\n", err) + } + + if lastErr == nil && last { + fmt.Printf("Last process exiting. Cleaning up profile %s...\n", cfg.Profile) + if err := namespace.UnpinNamespace(pm, cfg.Profile); err != nil { + fmt.Printf("failed to unpin namespace: %v\n", err) + } + } + namespace.ReleaseProfileLock(cleanupLock) + } else { + // Fallback if lock fails to ensure we still unregister + if err := namespace.UnregisterProcess(pm, cfg.Profile); err != nil { + fmt.Printf("failed to unregister process: %v\n", err) } } }() @@ -192,9 +206,6 @@ func (a *App) ExecuteCommand(cfg *config.Config) error { if _, err := os.Stat(profilePath); err == nil { wgCfg, err := wgconf.Parse(profilePath) if err != nil { - if lockErr == nil { - namespace.ReleaseProfileLock(lockFile) - } return fmt.Errorf("failed to parse profile %s: %w", cfg.Profile, err) } @@ -225,9 +236,6 @@ func (a *App) ExecuteCommand(cfg *config.Config) error { tunnel, err := wireguard.StartTunnel(wgCfg, dnsServer) if err != nil { - if lockErr == nil { - namespace.ReleaseProfileLock(lockFile) - } return fmt.Errorf("failed to start WireGuard tunnel: %w", err) } defer tunnel.Close() @@ -239,18 +247,13 @@ func (a *App) ExecuteCommand(cfg *config.Config) error { } else { // If profile is not default or it was explicitly requested but doesn't exist, we error if cfg.Profile != "default" { - if lockErr == nil { - namespace.ReleaseProfileLock(lockFile) - } return fmt.Errorf("profile %s not found: %w", cfg.Profile, err) } fmt.Printf("warning: default profile configuration not found. Executing command in bare isolation.\n") } - // Setup and initialization are complete. We can now safely release the startup lock! - if lockErr == nil { - namespace.ReleaseProfileLock(lockFile) - } + // We can now release the startup lock and execute the command + namespace.ReleaseProfileLock(lockFile) cmd := exec.Command(cfg.Command[0], cfg.Command[1:]...) cmd.Stdin = os.Stdin @@ -327,9 +330,15 @@ func (a *App) handleProfileConfigure(name string) error { editor = "vi" // Sensible fallback } + // Split editor string into command and arguments (e.g., "vim -R" -> ["vim", "-R"]) + editorArgs := strings.Fields(editor) + if len(editorArgs) == 0 { + editorArgs = []string{"vi"} + } + fmt.Printf("Opening profile %s in default editor (%s)...\n", name, editor) - cmd := exec.Command(editor, profilePath) + cmd := exec.Command(editorArgs[0], append(editorArgs[1:], profilePath)...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr diff --git a/internal/wireguard/wireguard.go b/internal/wireguard/wireguard.go index 48bd562..3f17392 100644 --- a/internal/wireguard/wireguard.go +++ b/internal/wireguard/wireguard.go @@ -42,6 +42,11 @@ func StartTunnel(cfg *wgconf.Config, dnsServer string) (*Tunnel, error) { fmt.Printf("warning: failed to make mount namespace private: %v\n", err) } + // Block host services (D-Bus, nscd) to prevent name resolution leak bypasses + if err := BlockHostServices(); err != nil { + fmt.Printf("warning: failed to block host services: %v\n", err) + } + tunDev, err := tun.CreateTUN(tunName, mtu) if err != nil { return nil, fmt.Errorf("failed to create TUN device %s: %w", tunName, err) @@ -254,13 +259,62 @@ func ConfigureResolvConf(dns string) error { // 2. Make the mount private to ensure it doesn't propagate back to the host // and to satisfy kernel requirements for mount transitions in some environments. - if err := unix.Mount("/etc/resolv.conf", "/etc/resolv.conf", "", unix.MS_REMOUNT|unix.MS_BIND|unix.MS_PRIVATE, ""); err != nil { - return fmt.Errorf("failed to make /etc/resolv.conf mount private: %w", err) + // We do this by applying MS_PRIVATE in a separate mount call. + if err := unix.Mount("", "/etc/resolv.conf", "", unix.MS_PRIVATE, ""); err != nil { + // If MS_PRIVATE fails, we can log a warning but proceed since / is already private + fmt.Printf("warning: failed to make /etc/resolv.conf mount private: %v\n", err) } return nil } +// BlockHostServices blocks local D-Bus and name service cache daemon (nscd) sockets +// inside the mount namespace. This prevents glibc from bypassing the network namespace +// isolation via host services (e.g. systemd-resolved via D-Bus). +func BlockHostServices() error { + tmpDir, err := os.MkdirTemp("", "wg-wrap-block-") + if err != nil { + return fmt.Errorf("failed to create temp dir: %w", err) + } + defer func() { _ = os.Remove(tmpDir) }() + + tmpFile, err := os.CreateTemp("", "wg-wrap-block-file-") + if err != nil { + return fmt.Errorf("failed to create temp file: %w", err) + } + tmpFileName := tmpFile.Name() + _ = tmpFile.Close() + defer func() { _ = os.Remove(tmpFileName) }() + + // Specific socket files and directories to block + pathsToBlock := []string{ + "/run/dbus/system_bus_socket", + "/run/systemd/resolve/io.systemd.Resolve", + "/run/systemd/resolve/io.systemd.Resolve.Monitor", + "/run/nscd/socket", + "/var/run/dbus/system_bus_socket", + "/var/run/systemd/resolve/io.systemd.Resolve", + "/var/run/systemd/resolve/io.systemd.Resolve.Monitor", + "/var/run/nscd/socket", + } + + for _, p := range pathsToBlock { + stat, err := os.Stat(p) + if err == nil { + source := tmpFileName + if stat.IsDir() { + source = tmpDir + } + if err := unix.Mount(source, p, "", unix.MS_BIND, ""); err != nil { + fmt.Printf("warning: failed to bind-mount block over %s: %v\n", p, err) + } else { + _ = unix.Mount("", p, "", unix.MS_PRIVATE, "") + } + } + } + return nil +} + // HostBind wraps a standard conn.Bind so that its socket creation (Open) // is forced to execute within a host network namespace. type HostBind struct { diff --git a/tests/e2e/race_test.go b/tests/e2e/race_test.go new file mode 100644 index 0000000..3f5ecfe --- /dev/null +++ b/tests/e2e/race_test.go @@ -0,0 +1,94 @@ +package e2e + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// TestLifecycleRace proves that a new process joining an existing namespace +// can have that namespace unpinned if an exiting process incorrectly +// thinks it's the last one out. +func TestLifecycleRace(t *testing.T) { + binaryPath, err := GetBinaryPath() + if err != nil { + t.Skipf("Skipping test: %v", err) + } + + tmpRuntimeDir := t.TempDir() + profile := "race-test" + pidsDir := filepath.Join(tmpRuntimeDir, "profiles", profile, "pids") + + // Setup a valid profile config to ensure tunneling starts + tmpConfigDir := t.TempDir() + profilesDir := filepath.Join(tmpConfigDir, "wg-wrap", "profiles") + if err := os.MkdirAll(profilesDir, 0755); err != nil { + t.Fatal(err) + } + profileConfPath := filepath.Join(profilesDir, profile+".conf") + conf := `[Interface] +Address = 10.0.0.2/24 +PrivateKey = 0000000000000000000000000000000000000000000000000000000000000000 +DNS = 1.1.1.1 + +[Peer] +PublicKey = 0000000000000000000000000000000000000000000000000000000000000000 +AllowedIPs = 0.0.0.0/0 +Endpoint = 1.1.1.1:51820 +` + if err := os.WriteFile(profileConfPath, []byte(conf), 0644); err != nil { + t.Fatal(err) + } + + // We use a a long-running sleep for Process A to keep the namespace active. + // Process A is the "Victim". + cmdA := exec.Command(binaryPath, "--profile", profile, "--", "sleep", "10") + cmdA.Env = append(os.Environ(), + fmt.Sprintf("XDG_RUNTIME_DIR=%s", tmpRuntimeDir), + fmt.Sprintf("XDG_CONFIG_HOME=%s", tmpConfigDir), + ) + if err := cmdA.Start(); err != nil { + t.Fatalf("Failed to start Process A: %v", err) + } + defer func() { _ = cmdA.Process.Kill() }() + + // Wait for Process A to establish the namespace and register PID + waitForPids(t, pidsDir, 1) + + // Process B is the "Saboteur". It will join and then exit. + // We will loop this to increase the chance of hitting the race window. + for i := 0; i < 5; i++ { + cmdB := exec.Command(binaryPath, "--profile", profile, "--", "sleep", "0.1") + cmdB.Env = append(os.Environ(), + fmt.Sprintf("XDG_RUNTIME_DIR=%s", tmpRuntimeDir), + fmt.Sprintf("XDG_CONFIG_HOME=%s", tmpConfigDir), + ) + if err := cmdB.Start(); err != nil { + t.Fatalf("Failed to start Process B (iteration %d): %v", i, err) + } + + // Wait for Process B to register + waitForPids(t, pidsDir, 2) + + // Let Process B exit. The defer block in ExecuteCommand will: + // 1. Unregister Process B + // 2. Check IsLastProcess() -> might return true if Process A's PID is stale or miscounted + // 3. UnpinNamespace() + if err := cmdB.Wait(); err != nil { + t.Fatalf("Process B failed: %v", err) + } + + // After B exits, Process A should still be the only remaining process. + // We check if the namespace pin file was accidentally deleted by B. + nsPath := filepath.Join(tmpRuntimeDir, "profiles", profile+".ns") + if _, err := os.Stat(nsPath); os.IsNotExist(err) { + t.Errorf("BUG: Namespace pin file was deleted by exiting Process B, despite Process A still running!") + return + } + + // Wait for the PID count to drop back to 1 + waitForPids(t, pidsDir, 1) + } +} diff --git a/tests/e2e/vulnerability_test.go b/tests/e2e/vulnerability_test.go new file mode 100644 index 0000000..a8c2dfb --- /dev/null +++ b/tests/e2e/vulnerability_test.go @@ -0,0 +1,109 @@ +package e2e + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "git.theodohertyfamily.com/tools/wg-wrap/internal/cli" +) + +// TestEditorArgumentSplitting verifies that the editor is correctly split into command and arguments. +func TestEditorArgumentSplitting(t *testing.T) { + // Create a dummy profile file so the check passes + tmpDir := t.TempDir() + profileName := "editor-test" + profilePath := tmpDir + "/" + profileName + ".conf" + err := os.WriteFile(profilePath, []byte("[Interface]\nPrivateKey = 00000000000000000000000000000000"), 0644) + if err != nil { + t.Fatal(err) + } + + app := cli.NewApp([]string{"wg-wrap", "profile", "configure", profileName}) + app.ConfigDir = tmpDir + + // Set EDITOR to something that includes an argument. + // We use a non-existent command that clearly contains a space. + if err := os.Setenv("EDITOR", "nonexistent-editor -v"); err != nil { + t.Fatalf("failed to set EDITOR: %v", err) + } + defer func() { + _ = os.Unsetenv("EDITOR") + }() + + err = app.Route() + + if err == nil { + t.Fatal("Expected error when calling nonexistent editor, but got nil") + } + + // If it fails because it can't find "nonexistent-editor -v" (the whole string), it's the bug. + // If it fails because it can't find "nonexistent-editor" (split), it's correct. + if strings.Contains(err.Error(), "nonexistent-editor -v") { + t.Errorf("BUG: Editor was not split into arguments. Error contains the full string: %v", err) + } else { + t.Logf("Success: Editor was likely split correctly. Error: %v", err) + } +} + +// TestDNSLeak proves that the child process leaks to the host DNS if the +// isolation fails or if /etc/resolv.conf is not correctly bound. +func TestDNSLeak(t *testing.T) { + if testing.Short() { + t.Skip("Skipping DNS leak test in short mode") + } + + binaryPath, err := GetBinaryPath() + if err != nil { + t.Skipf("Skipping test: %v", err) + } + + // 1. Setup a profile with a fake, unreachable DNS server. + tmpConfigDir := t.TempDir() + tmpRuntimeDir := t.TempDir() + profileName := "dns-leak-test" + + profilesDir := filepath.Join(tmpConfigDir, "wg-wrap", "profiles") + if err := os.MkdirAll(profilesDir, 0755); err != nil { + t.Fatal(err) + } + profilePath := filepath.Join(profilesDir, profileName+".conf") + + // Use a fake DNS server (10.0.0.53) and a valid-looking config. + conf := `[Interface] +Address = 10.0.0.2/24 +PrivateKey = 0000000000000000000000000000000000000000000000000000000000000000 +DNS = 10.0.0.53 + +[Peer] +PublicKey = 0000000000000000000000000000000000000000000000000000000000000000 +AllowedIPs = 0.0.0.0/0 +Endpoint = 1.1.1.1:51820 +` + err = os.WriteFile(profilePath, []byte(conf), 0644) + if err != nil { + t.Fatal(err) + } + + // 2. Run a command that performs a DNS lookup using exec.Command on the wg-wrap binary. + // We use 'timeout 3 getent hosts google.com' to ensure it fails quickly instead of waiting on timeouts. + cmd := exec.Command(binaryPath, "--profile", profileName, "--", "timeout", "3", "getent", "hosts", "google.com") + cmd.Env = append(os.Environ(), + fmt.Sprintf("XDG_CONFIG_HOME=%s", tmpConfigDir), + fmt.Sprintf("XDG_RUNTIME_DIR=%s", tmpRuntimeDir), + ) + + // If the DNS isolation works, it should fail to resolve because 10.0.0.53 is fake. + // If it leaks, it will resolve using the host's DNS. + out, err := cmd.CombinedOutput() + + // If err is nil, it means getent returned 0, which means it RESOLVED the host -> LEAK! + if err == nil { + t.Errorf("BUG: DNS Leak detected! The command resolved the hostname using the host's DNS instead of failing via the fake VPN DNS. Output: %s", string(out)) + } else { + t.Logf("Success: Command failed as expected (DNS isolated). Error: %v, Output: %s", err, string(out)) + } +} |