diff --git a/client/cmd/ssh_sftp_unix.go b/client/cmd/ssh_sftp_unix.go index 7723165cf..c06aab017 100644 --- a/client/cmd/ssh_sftp_unix.go +++ b/client/cmd/ssh_sftp_unix.go @@ -77,12 +77,6 @@ func sftpMain(cmd *cobra.Command, _ []string) error { os.Exit(sshserver.ExitCodeShellExecFail) } - defer func() { - if err := sftpServer.Close(); err != nil { - cmd.PrintErrf("SFTP server close error: %v\n", err) - } - }() - log.Tracef("starting SFTP server with dropped privileges") if err := sftpServer.Serve(); err != nil && !errors.Is(err, io.EOF) { cmd.PrintErrf("SFTP server error: %v\n", err) diff --git a/client/ssh/server/command_execution_unix.go b/client/ssh/server/command_execution_unix.go index 187d5ecfd..09ed8c71f 100644 --- a/client/ssh/server/command_execution_unix.go +++ b/client/ssh/server/command_execution_unix.go @@ -98,7 +98,7 @@ func (s *Server) handlePty(logger *log.Entry, session ssh.Session, privilegeResu if err != nil { logger.Errorf("Pty command creation failed: %v", err) errorMsg := "User switching failed - login command not available\r\n" - if _, writeErr := fmt.Fprintf(session.Stderr(), errorMsg); writeErr != nil { + if _, writeErr := fmt.Fprint(session.Stderr(), errorMsg); writeErr != nil { logger.Debugf(errWriteSession, writeErr) } if err := session.Exit(1); err != nil { diff --git a/client/ssh/server/command_execution_windows.go b/client/ssh/server/command_execution_windows.go index 3d2606c49..ae55b3eab 100644 --- a/client/ssh/server/command_execution_windows.go +++ b/client/ssh/server/command_execution_windows.go @@ -401,3 +401,11 @@ func (s *Server) killProcessGroup(cmd *exec.Cmd) { logger.Debugf("kill process failed: %v", err) } } + +// buildShellArgs builds shell arguments for executing commands +func buildShellArgs(shell, command string) []string { + if command != "" { + return []string{shell, "-Command", command} + } + return []string{shell} +} diff --git a/client/ssh/server/executor_unix_test.go b/client/ssh/server/executor_unix_test.go new file mode 100644 index 000000000..98fad4a76 --- /dev/null +++ b/client/ssh/server/executor_unix_test.go @@ -0,0 +1,221 @@ +//go:build unix + +package server + +import ( + "context" + "os" + "os/exec" + "os/user" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPrivilegeDropper_ValidatePrivileges(t *testing.T) { + pd := NewPrivilegeDropper() + + tests := []struct { + name string + uid uint32 + gid uint32 + wantErr bool + }{ + { + name: "valid non-root user", + uid: 1000, + gid: 1000, + wantErr: false, + }, + { + name: "root UID should be rejected", + uid: 0, + gid: 1000, + wantErr: true, + }, + { + name: "root GID should be rejected", + uid: 1000, + gid: 0, + wantErr: true, + }, + { + name: "both root should be rejected", + uid: 0, + gid: 0, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := pd.validatePrivileges(tt.uid, tt.gid) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestPrivilegeDropper_CreateExecutorCommand(t *testing.T) { + pd := NewPrivilegeDropper() + + config := ExecutorConfig{ + UID: 1000, + GID: 1000, + Groups: []uint32{1000, 1001}, + WorkingDir: "/home/testuser", + Shell: "/bin/bash", + Command: "ls -la", + } + + cmd, err := pd.CreateExecutorCommand(context.Background(), config) + require.NoError(t, err) + require.NotNil(t, cmd) + + // Verify the command is calling netbird ssh exec + assert.Contains(t, cmd.Args, "ssh") + assert.Contains(t, cmd.Args, "exec") + assert.Contains(t, cmd.Args, "--uid") + assert.Contains(t, cmd.Args, "1000") + assert.Contains(t, cmd.Args, "--gid") + assert.Contains(t, cmd.Args, "1000") + assert.Contains(t, cmd.Args, "--groups") + assert.Contains(t, cmd.Args, "1000") + assert.Contains(t, cmd.Args, "1001") + assert.Contains(t, cmd.Args, "--working-dir") + assert.Contains(t, cmd.Args, "/home/testuser") + assert.Contains(t, cmd.Args, "--shell") + assert.Contains(t, cmd.Args, "/bin/bash") + assert.Contains(t, cmd.Args, "--cmd") + assert.Contains(t, cmd.Args, "ls -la") +} + +func TestPrivilegeDropper_CreateExecutorCommandInteractive(t *testing.T) { + pd := NewPrivilegeDropper() + + config := ExecutorConfig{ + UID: 1000, + GID: 1000, + Groups: []uint32{1000}, + WorkingDir: "/home/testuser", + Shell: "/bin/bash", + Command: "", + } + + cmd, err := pd.CreateExecutorCommand(context.Background(), config) + require.NoError(t, err) + require.NotNil(t, cmd) + + // Verify no command mode (command is empty so no --cmd flag) + assert.NotContains(t, cmd.Args, "--cmd") + assert.NotContains(t, cmd.Args, "--interactive") +} + +// TestPrivilegeDropper_ActualPrivilegeDrop tests actual privilege dropping +// This test requires root privileges and will be skipped if not running as root +func TestPrivilegeDropper_ActualPrivilegeDrop(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("This test requires root privileges") + } + + // Find a non-root user to test with + testUser, err := user.Lookup("nobody") + if err != nil { + // Try to find any non-root user + testUser, err = findNonRootUser() + if err != nil { + t.Skip("No suitable non-root user found for testing") + } + } + + uid64, err := strconv.ParseUint(testUser.Uid, 10, 32) + require.NoError(t, err) + targetUID := uint32(uid64) + + gid64, err := strconv.ParseUint(testUser.Gid, 10, 32) + require.NoError(t, err) + targetGID := uint32(gid64) + + // Test in a child process to avoid affecting the test runner + if os.Getenv("TEST_PRIVILEGE_DROP") == "1" { + pd := NewPrivilegeDropper() + + // This should succeed + err := pd.DropPrivileges(targetUID, targetGID, []uint32{targetGID}) + require.NoError(t, err) + + // Verify we are now running as the target user + currentUID := uint32(os.Geteuid()) + currentGID := uint32(os.Getegid()) + + assert.Equal(t, targetUID, currentUID, "UID should match target") + assert.Equal(t, targetGID, currentGID, "GID should match target") + assert.NotEqual(t, uint32(0), currentUID, "Should not be running as root") + assert.NotEqual(t, uint32(0), currentGID, "Should not be running as root group") + + return + } + + // Fork a child process to test privilege dropping + cmd := os.Args[0] + args := []string{"-test.run=TestPrivilegeDropper_ActualPrivilegeDrop"} + + env := append(os.Environ(), "TEST_PRIVILEGE_DROP=1") + + execCmd := exec.Command(cmd, args...) + execCmd.Env = env + + err = execCmd.Run() + require.NoError(t, err, "Child process should succeed") +} + +// findNonRootUser finds any non-root user on the system for testing +func findNonRootUser() (*user.User, error) { + // Try common non-root users + commonUsers := []string{"nobody", "daemon", "bin", "sys", "sync", "games", "man", "lp", "mail", "news", "uucp", "proxy", "www-data", "backup", "list", "irc"} + + for _, username := range commonUsers { + if u, err := user.Lookup(username); err == nil { + uid64, err := strconv.ParseUint(u.Uid, 10, 32) + if err != nil { + continue + } + if uid64 != 0 { // Not root + return u, nil + } + } + } + + // If no common users found, create a minimal user info for testing + // This won't actually work for privilege dropping but allows the test structure + return &user.User{ + Uid: "65534", // Standard nobody UID + Gid: "65534", // Standard nobody GID + Username: "nobody", + Name: "nobody", + HomeDir: "/nonexistent", + }, nil +} + +func TestPrivilegeDropper_ExecuteWithPrivilegeDrop_Validation(t *testing.T) { + pd := NewPrivilegeDropper() + + // Test validation of root privileges - this should be caught in CreateExecutorCommand + config := ExecutorConfig{ + UID: 0, // Root UID should be rejected + GID: 1000, + Groups: []uint32{1000}, + WorkingDir: "/tmp", + Shell: "/bin/sh", + Command: "echo test", + } + + _, err := pd.CreateExecutorCommand(context.Background(), config) + assert.Error(t, err) + assert.Contains(t, err.Error(), "root user") +} diff --git a/client/ssh/server/session_handlers.go b/client/ssh/server/session_handlers.go index 06d4e5a07..402ff8bfb 100644 --- a/client/ssh/server/session_handlers.go +++ b/client/ssh/server/session_handlers.go @@ -117,7 +117,7 @@ func (s *Server) unregisterSession(sessionKey SessionKey, _ ssh.Session) { func (s *Server) handlePrivError(logger *log.Entry, session ssh.Session, err error) { errorMsg := s.buildUserLookupErrorMessage(err) - if _, writeErr := fmt.Fprintf(session, errorMsg); writeErr != nil { + if _, writeErr := fmt.Fprint(session, errorMsg); writeErr != nil { logger.Debugf(errWriteSession, writeErr) } if exitErr := session.Exit(1); exitErr != nil { diff --git a/client/ssh/server/socket_filter_nonlinux_test.go b/client/ssh/server/socket_filter_nonlinux_test.go deleted file mode 100644 index 5f29b220b..000000000 --- a/client/ssh/server/socket_filter_nonlinux_test.go +++ /dev/null @@ -1,48 +0,0 @@ -//go:build !linux - -package server - -import ( - "net" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestAttachSocketFilter_NonLinux(t *testing.T) { - // Create a test TCP listener - tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0") - require.NoError(t, err, "Should resolve TCP address") - - tcpListener, err := net.ListenTCP("tcp", tcpAddr) - require.NoError(t, err, "Should create TCP listener") - defer func() { - if closeErr := tcpListener.Close(); closeErr != nil { - t.Logf("TCP listener close error: %v", closeErr) - } - }() - - // Test that socket filter attachment returns an error on non-Linux platforms - err = attachSocketFilter(tcpListener, 1) - require.Error(t, err, "Should return error on non-Linux platforms") - require.Contains(t, err.Error(), "only supported on Linux", "Error should indicate platform limitation") -} - -func TestDetachSocketFilter_NonLinux(t *testing.T) { - // Create a test TCP listener - tcpAddr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0") - require.NoError(t, err, "Should resolve TCP address") - - tcpListener, err := net.ListenTCP("tcp", tcpAddr) - require.NoError(t, err, "Should create TCP listener") - defer func() { - if closeErr := tcpListener.Close(); closeErr != nil { - t.Logf("TCP listener close error: %v", closeErr) - } - }() - - // Test that socket filter detachment returns an error on non-Linux platforms - err = detachSocketFilter(tcpListener) - require.Error(t, err, "Should return error on non-Linux platforms") - require.Contains(t, err.Error(), "only supported on Linux", "Error should indicate platform limitation") -} diff --git a/client/ssh/server/user_utils.go b/client/ssh/server/user_utils.go index b82aa6b8a..6215db190 100644 --- a/client/ssh/server/user_utils.go +++ b/client/ssh/server/user_utils.go @@ -378,14 +378,6 @@ func isWindowsPrivilegedSID(sid string) bool { return false } -// buildShellArgs builds shell arguments for executing commands -func buildShellArgs(shell, command string) []string { - if command != "" { - return []string{shell, "-Command", command} - } - return []string{shell} -} - // isCurrentProcessPrivileged checks if the current process is running with elevated privileges. // On Unix systems, this means running as root (UID 0). // On Windows, this means running as Administrator or SYSTEM. diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index c8d854a94..37a230358 100644 --- a/client/ui/client_ui.go +++ b/client/ui/client_ui.go @@ -522,12 +522,12 @@ func (s *serviceClient) applySettings(iMngURL, iAdminURL string, port int64) { loginRequest := s.buildLoginRequest(iMngURL, iAdminURL, port) - if err := s.restartClient(&loginRequest); err != nil { + if err := s.restartClient(loginRequest); err != nil { log.Errorf("restarting client connection: %v", err) } } -func (s *serviceClient) buildLoginRequest(iMngURL, iAdminURL string, port int64) proto.LoginRequest { +func (s *serviceClient) buildLoginRequest(iMngURL, iAdminURL string, port int64) *proto.LoginRequest { loginRequest := proto.LoginRequest{ ManagementUrl: iMngURL, AdminURL: iAdminURL, @@ -550,7 +550,7 @@ func (s *serviceClient) buildLoginRequest(iMngURL, iAdminURL string, port int64) loginRequest.OptionalPreSharedKey = &s.iPreSharedKey.Text } - return loginRequest + return &loginRequest } func (s *serviceClient) login(openURL bool) (*proto.LoginResponse, error) {