Harden SSH and SCP

internal/target/remote_target.go

@@ -397,12 +397,16 @@ func (t *RemoteTarget) prepareSSHCommand(command []string, useControlMaster bool
 	var cmd []string
 	cmd = append(cmd, "ssh")
 	cmd = append(cmd, t.prepareSSHFlags(false, useControlMaster, prompt)...)
+	// end ssh's option parsing before the destination so that a destination beginning with a dash
+	// cannot be interpreted as an ssh option, e.g., -oProxyCommand=<command to run locally>
+	cmd = append(cmd, "--")
 	if t.user != "" {
 		cmd = append(cmd, t.user+"@"+t.host)
 	} else {
 		cmd = append(cmd, t.host)
 	}
-	cmd = append(cmd, "--")
+	// note: no "--" separator before the remote command; option parsing already ended above and ssh
+	// would pass a second separator to the remote shell as part of the command
 	cmd = append(cmd, command...)
 	return cmd
 }
@@ -420,6 +424,8 @@ func (t *RemoteTarget) prepareSCPCommand(src string, dstDir string, push bool) [
 		if fileInfo.IsDir() {
 			cmd = append(cmd, "-r")
 		}
+		// end scp's option parsing before the paths, see prepareSSHCommand
+		cmd = append(cmd, "--")
 		cmd = append(cmd, src)
 		dst := t.host + ":" + dstDir
 		if t.user != "" {
@@ -431,6 +437,8 @@ func (t *RemoteTarget) prepareSCPCommand(src string, dstDir string, push bool) [
 		if t.user != "" {
 			s = t.user + "@" + s
 		}
+		// end scp's option parsing before the paths, see prepareSSHCommand
+		cmd = append(cmd, "--")
 		cmd = append(cmd, s)
 		cmd = append(cmd, dstDir)
 	}

internal/workflow/targets.go

@@ -92,38 +92,102 @@ func ValidateTargetFlags(cmd *cobra.Command) error {
 		}
 	}
 	// confirm that port is a positive integer
-	if flagTargetPort != "" {
-		var port int
-		var err error
-		if port, err = strconv.Atoi(flagTargetPort); err != nil || port <= 0 {
-			return fmt.Errorf("port %s is not a positive integer", flagTargetPort)
-		}
+	if err := validateTargetPort(flagTargetPort); err != nil {
+		return err
 	}
 	// confirm that the key file exists
-	if flagTargetKeyFile != "" {
-		if _, err := os.Stat(flagTargetKeyFile); os.IsNotExist(err) {
-			return fmt.Errorf("key file %s does not exist", flagTargetKeyFile)
-		}
+	if err := validateTargetKeyFile(flagTargetKeyFile); err != nil {
+		return err
 	}
 	// confirm that user is a valid user name
-	if flagTargetUser != "" {
-		userNameRegex := `^[a-z_][a-z0-9_.-]{0,63}$`
-		re := regexp.MustCompile(userNameRegex)
-		if !re.MatchString(flagTargetUser) {
-			return fmt.Errorf("user name %s does not match the user name regex '%s'", flagTargetUser, userNameRegex)
-		}
+	if err := validateTargetUser(flagTargetUser); err != nil {
+		return err
 	}
 	// confirm that host is a valid host name or IP address
-	if flagTargetHost != "" {
-		hostNameRegex := `^([a-zA-Z0-9.-]+)$`
-		re := regexp.MustCompile(hostNameRegex)
-		if !re.MatchString(flagTargetHost) {
-			return fmt.Errorf("host name %s does not match the host name regex '%s'", flagTargetHost, hostNameRegex)
-		}
+	if err := validateTargetHost(flagTargetHost); err != nil {
+		return err
 	}
 	return nil
 }
 
+// Connection details arrive from the command line and from the targets file. They are placed
+// directly into the argument vector of the local ssh/scp process, so a value that begins with a
+// dash would be interpreted by ssh as an option, e.g., -oProxyCommand=<arbitrary command>, which
+// ssh runs on the local host. The host name additionally becomes a path component of the target's
+// output directory when no target name is given. The regular expressions below therefore exclude
+// both leading dashes and path separators. They are deliberately strict; note that IPv6 literals
+// are not accepted (a limitation that predates this validation).
+const (
+	hostNameRegex = `^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?$`
+	userNameRegex = `^[a-z_][a-z0-9_.-]{0,63}$`
+)
+
+// validateTargetHost confirms that host is a valid host name or IP address. An empty host is
+// valid; callers that require a host check for it separately.
+func validateTargetHost(host string) error {
+	if host == "" {
+		return nil
+	}
+	if !regexp.MustCompile(hostNameRegex).MatchString(host) {
+		return fmt.Errorf("host name %s does not match the host name regex '%s'", host, hostNameRegex)
+	}
+	return nil
+}
+
+// validateTargetUser confirms that user is a valid user name. An empty user is valid; ssh falls
+// back to the local user name.
+func validateTargetUser(user string) error {
+	if user == "" {
+		return nil
+	}
+	if !regexp.MustCompile(userNameRegex).MatchString(user) {
+		return fmt.Errorf("user name %s does not match the user name regex '%s'", user, userNameRegex)
+	}
+	return nil
+}
+
+// validateTargetPort confirms that port is a positive integer. An empty port is valid; ssh uses
+// its default port.
+func validateTargetPort(port string) error {
+	if port == "" {
+		return nil
+	}
+	portNum, err := strconv.Atoi(port)
+	if err != nil || portNum <= 0 || portNum > 65535 {
+		return fmt.Errorf("port %s is not a valid port number", port)
+	}
+	return nil
+}
+
+// validateTargetKeyFile confirms that the key file exists. An empty key file is valid; ssh uses
+// its default keys or a password.
+func validateTargetKeyFile(keyFile string) error {
+	if keyFile == "" {
+		return nil
+	}
+	if _, err := os.Stat(keyFile); os.IsNotExist(err) {
+		return fmt.Errorf("key file %s does not exist", keyFile)
+	}
+	return nil
+}
+
+// validateTargetFromFile validates the connection details of one target read from a targets file.
+func validateTargetFromFile(t targetFromYAML) error {
+	if t.Host == "" {
+		return fmt.Errorf("target in targets file has no host")
+	}
+	if err := validateTargetHost(t.Host); err != nil {
+		return err
+	}
+	if err := validateTargetUser(t.User); err != nil {
+		return err
+	}
+	if err := validateTargetPort(t.Port); err != nil {
+		return err
+	}
+	return validateTargetKeyFile(t.Key)
+}
+
 // GetTargets retrieves the list of targets based on the provided command and parameters. It creates
 // a temporary directory for each target and returns a slice of target.Target objects.
 func GetTargets(cmd *cobra.Command, needsElevatedPrivileges bool, failIfCantElevate bool, localTempDir string) (targets []target.Target, targetErrs []error, err error) {
@@ -313,6 +377,8 @@ type targetsFile struct {
 }
 
 // sanitizeTargetName sanitizes the target name by removing any invalid characters.
+// The result is used as a file and directory name, so names that would refer to another directory
+// are replaced entirely.
 func sanitizeTargetName(targetName string) string {
 	// remove any invalid characters from the target name
 	// this is needed for the report file names
@@ -333,6 +399,10 @@ func sanitizeTargetName(targetName string) string {
 		}
 		return '_'
 	}, targetName)
+	// "." and ".." survive the mapping above but refer to a directory rather than naming one
+	if sanitizedTargetName == "." || sanitizedTargetName == ".." {
+		return strings.Repeat("_", len(sanitizedTargetName))
+	}
 	return sanitizedTargetName
 }
 
@@ -356,7 +426,7 @@ func getTargetsFromFile(targetsFilePath string, localTempDir string) (targets []
 		return
 	}
 	targetNameUsed := make(map[string]bool)
-	for _, t := range targetsFile.Targets {
+	for targetIdx, t := range targetsFile.Targets {
 		// create a target object
 		// target name is not required, but if it is provided there must not be duplicate names
 		var targetName string
@@ -367,6 +437,22 @@ func getTargetsFromFile(targetsFilePath string, localTempDir string) (targets []
 				return
 			}
 			targetNameUsed[targetName] = true
+		} else {
+			// without a name, the target is identified by its host name, which is also used to
+			// name the target's output directory and files, so it must be sanitized
+			targetName = sanitizeTargetName(t.Host)
+		}
+		if targetName == "" { // neither a name nor a host was provided
+			targetName = fmt.Sprintf("target %d", targetIdx+1)
+		}
+		// the connection details are used to build the local ssh/scp command line, so they must be
+		// validated before they are used, see validateTargetHost for details
+		if targetErr := validateTargetFromFile(t); targetErr != nil {
+			// retain a placeholder target, without connection details, so that the caller's
+			// targets and errors remain parallel and the invalid details are never used
+			targets = append(targets, target.NewRemoteTarget(targetName, "", "", "", ""))
+			targetErrs = append(targetErrs, targetErr)
+			continue
 		}
 		newTarget := target.NewRemoteTarget(targetName, t.Host, t.Port, t.User, t.Key)
 		newTarget.SetSshPass(t.Pwd)

internal/workflow/targets_test.go

@@ -4,6 +4,8 @@
 package workflow
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -178,6 +180,21 @@ func TestSanitizeTargetName(t *testing.T) {
 			input:    "valid@name#123!.txt",
 			expected: "valid_name_123_.txt",
 		},
+		{
+			name:     "Name referring to the current directory",
+			input:    ".",
+			expected: "_",
+		},
+		{
+			name:     "Name referring to the parent directory",
+			input:    "..",
+			expected: "__",
+		},
+		{
+			name:     "Name attempting path traversal",
+			input:    "../../etc",
+			expected: ".._.._etc",
+		},
 	}
 
 	for _, tt := range tests {
@@ -187,3 +204,119 @@ func TestSanitizeTargetName(t *testing.T) {
 		})
 	}
 }
+
+// TestValidateTargetHost confirms the host name rules. A host name becomes an argument of the local
+// ssh process and, when a target is not named, a component of the target's output path, so a leading
+// dash and a path separator must both be rejected. See validateTargetHost for the reasoning.
+func TestValidateTargetHost(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		wantErr bool
+	}{
+		{name: "Empty host", input: "", wantErr: false},
+		{name: "Host name", input: "my-host.example.com", wantErr: false},
+		{name: "IPv4 address", input: "192.168.1.1", wantErr: false},
+		{name: "Single character host", input: "h", wantErr: false},
+		{name: "Host beginning with a dash", input: "-host", wantErr: true},
+		{name: "Host ending with a dash", input: "host-", wantErr: true},
+		{name: "Host with a path separator", input: "../../etc", wantErr: true},
+		{name: "Host with a space", input: "host name", wantErr: true},
+		{name: "Host with a semicolon", input: "host;name", wantErr: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateTargetHost(tt.input)
+			if tt.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestValidateTargetFromFile(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   targetFromYAML
+		wantErr bool
+	}{
+		{
+			name:    "Valid target",
+			input:   targetFromYAML{Name: "target1", Host: "host1", Port: "22", User: "user1"},
+			wantErr: false,
+		},
+		{
+			name:    "Valid target with only a host",
+			input:   targetFromYAML{Host: "host1"},
+			wantErr: false,
+		},
+		{
+			name:    "Missing host",
+			input:   targetFromYAML{Name: "target1", User: "user1"},
+			wantErr: true,
+		},
+		{
+			name:    "Port is not a number",
+			input:   targetFromYAML{Host: "host1", Port: "notanumber"},
+			wantErr: true,
+		},
+		{
+			name:    "Port is out of range",
+			input:   targetFromYAML{Host: "host1", Port: "65536"},
+			wantErr: true,
+		},
+		{
+			name:    "Key file does not exist",
+			input:   targetFromYAML{Host: "host1", Key: "/no/such/key/file"},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateTargetFromFile(tt.input)
+			if tt.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
+
+// TestGetTargetsFromFileRejectsInvalidTargets confirms that an entry with invalid connection details
+// yields a target error rather than a usable target, and that the returned targets and errors stay
+// parallel, which the callers of GetTargets require.
+func TestGetTargetsFromFileRejectsInvalidTargets(t *testing.T) {
+	tempDir := t.TempDir()
+	// every entry is invalid, so that no connection is attempted and the test cannot hang on name
+	// resolution; the second entry is unnamed, so its name comes from its host
+	yaml := `targets:
+  - name: target1
+    host: "host name with a space"
+    port:
+    user:
+    key:
+    pwd:
+  - host: host2
+    port: 65536
+`
+	targetsFilePath := filepath.Join(tempDir, "targets.yaml")
+	if err := os.WriteFile(targetsFilePath, []byte(yaml), 0600); err != nil {
+		t.Fatalf("failed to write targets file: %v", err)
+	}
+
+	targets, targetErrs, err := getTargetsFromFile(targetsFilePath, tempDir)
+	assert.NoError(t, err)
+	// the targets and their errors remain parallel, as the caller requires
+	assert.Len(t, targets, 2)
+	assert.Len(t, targetErrs, 2)
+	// both targets are rejected, and both keep a usable name for display
+	assert.Error(t, targetErrs[0])
+	assert.Equal(t, "target1", targets[0].GetName())
+	assert.Error(t, targetErrs[1])
+	assert.Equal(t, "host2", targets[1].GetName())
+}