sftp: fix cmd shell execution of paths containing variable-expansion or newline characters

An audit of the shell escaping alongside the PowerShell fix found the
Windows Command Prompt path only rejected the double quote delimiter. A
remote filename containing % or ! can trigger environment variable expansion
even inside double quotes, and a newline or carriage return ends the
command, so a crafted filename could alter the command run during
server-side hashing. Reject paths containing these characters, matching
the existing treatment of the double quote.

backend/sftp/sftp.go

@@ -2470,9 +2470,16 @@ func quoteOrEscapeShellPath(shellType string, shellPath string) (string, error)
 		return "'" + powerShellQuoteEscaper.Replace(shellPath) + "'", nil
 	}
 	// Windows Command Prompt
+	//
+	// cmd has no reliable command-line escaping for the double quote used
+	// as the delimiter, while % expands environment variables and ! may
+	// expand them when delayed expansion is enabled, even inside double
+	// quotes. A newline or carriage return ends the command. None of these
+	// can be neutralised safely, so reject any path containing them rather
+	// than risk altering the command.
 	if shellType == "cmd" {
-		if strings.Contains(shellPath, "\"") {
-			return "", fmt.Errorf("path is not valid in shell type %s: %s", shellType, shellPath)
+		if strings.ContainsAny(shellPath, "\"%!\r\n") {
+			return "", fmt.Errorf("path is not valid in shell type %s: %q", shellType, shellPath)
 		}
 		return "\"" + shellPath + "\"", nil
 	}

backend/sftp/sftp_internal_test.go

@@ -36,6 +36,10 @@ func TestShellEscapeUnix(t *testing.T) {
 		{"$(rm -rf /)", "\\$\\(rm\\ -rf\\ /\\)"},
 		{"/test/\n", "/test/'\n'"},
 		{":\"'", ":\\\"\\'"},
+		// a backslash must be escaped so it cannot neutralise the escape
+		// of the metacharacter that follows it
+		{"a\\;id", "a\\\\\\;id"},
+		{"`id`", "\\`id\\`"},
 	} {
 		got, err := quoteOrEscapeShellPath("unix", test.unescaped)
 		assert.NoError(t, err)
@@ -50,8 +54,15 @@ func TestShellEscapeCmd(t *testing.T) {
 	}{
 		{"", "\"\"", true},
 		{"c:/this/is/harmless", "\"c:/this/is/harmless\"", true},
+		// & < > | ^ are not special inside cmd double quotes so are allowed
 		{"c:/test&notepad", "\"c:/test&notepad\"", true},
 		{"c:/test\"&\"notepad", "", false},
+		// % and ! expand environment variables even inside double quotes
+		{"c:/test%PATH%notepad", "", false},
+		{"c:/test!PATH!notepad", "", false},
+		// a newline or carriage return ends the command
+		{"c:/test\nnotepad", "", false},
+		{"c:/test\rnotepad", "", false},
 	} {
 		got, err := quoteOrEscapeShellPath("cmd", test.unescaped)
 		if test.ok {