Prevent SSH option/shell injection via store URLs

remotessh.go

@@ -88,8 +88,15 @@ func StartProtocol(u *url.URL) (*Protocol, error) {
 	if u.User != nil {
 		host = u.User.Username() + "@" + u.Host
 	}
+	// Reject destinations that ssh would parse as command-line options.
+	if err := validateSSHHost(host); err != nil {
+		return nil, err
+	}
 
-	c := exec.Command(sshCmd, host, fmt.Sprintf("%s pull - - - '%s'", remoteCmd, path))
+	// "--" terminates ssh's option parsing so the destination can't be read as a
+	// flag, and the path is shell-quoted so it can't break out of the remote
+	// command string.
+	c := exec.Command(sshCmd, "--", host, fmt.Sprintf("%s pull - - - %s", remoteCmd, shellQuote(path)))
 	c.Stderr = os.Stderr
 	r, err := c.StdoutPipe()
 	if err != nil {

sftp.go

@@ -52,8 +52,13 @@ func newSFTPStoreBase(location *url.URL, opt StoreOptions) (*SFTPStoreBase, erro
 	if location.User != nil {
 		host = location.User.Username() + "@" + location.Host
 	}
+	// Reject destinations that ssh would parse as command-line options.
+	if err := validateSSHHost(host); err != nil {
+		return nil, err
+	}
 	ctx, cancel := context.WithCancel(context.Background())
-	c := exec.CommandContext(ctx, sshCmd, host, "-s", "sftp")
+	// "--" terminates option parsing so the destination can't be read as a flag.
+	c := exec.CommandContext(ctx, sshCmd, "-s", "--", host, "sftp")
 	c.Stderr = os.Stderr
 	r, err := c.StdoutPipe()
 	if err != nil {

sshcmd.go

@@ -0,0 +1,29 @@
+package desync
+
+import (
+	"fmt"
+	"strings"
+)
+
+// validateSSHHost rejects an ssh destination that begins with a dash. Such a
+// value (built from the store URL's user and host) would otherwise be parsed by
+// ssh as a command-line option rather than a destination, allowing option
+// injection such as -oProxyCommand=... and arbitrary local command execution
+// (the same class of bug as Git CVE-2017-1000117 and Mercurial CVE-2017-1000116).
+// Callers must reject the destination before passing it to ssh; the "--"
+// argument added to the ssh invocation is a defense-in-depth measure that only
+// some ssh implementations honor.
+func validateSSHHost(host string) error {
+	if strings.HasPrefix(host, "-") {
+		return fmt.Errorf("invalid ssh destination %q: must not begin with '-'", host)
+	}
+	return nil
+}
+
+// shellQuote wraps s in single quotes for safe inclusion in a POSIX shell
+// command line, escaping any embedded single quotes. This prevents shell
+// injection when interpolating untrusted values (such as a store URL path) into
+// the remote command string passed to ssh.
+func shellQuote(s string) string {
+	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
+}

sshcmd_test.go

@@ -0,0 +1,53 @@
+package desync
+
+import (
+	"os/exec"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestValidateSSHHost(t *testing.T) {
+	valid := []string{
+		"example.com",
+		"user@example.com",
+		"user@example.com:2222",
+		"192.0.2.1",
+		"a-b.example.com", // dash allowed mid-string
+	}
+	for _, h := range valid {
+		assert.NoError(t, validateSSHHost(h), "%q should be accepted", h)
+	}
+
+	invalid := []string{
+		"-",
+		"-x",
+		"-oProxyCommand=touch${IFS}pwned",
+		"-oProxyCommand=id@example.com",
+	}
+	for _, h := range invalid {
+		assert.Error(t, validateSSHHost(h), "%q should be rejected", h)
+	}
+}
+
+func TestShellQuote(t *testing.T) {
+	// Each input must survive a round-trip through a POSIX shell unchanged,
+	// proving the quoting can't be broken out of.
+	inputs := []string{
+		"",
+		"/path/to/store/",
+		"plain",
+		"o'brien",
+		`a'; touch pwned; '`,
+		`'`,
+		`''`,
+		`back\slash`,
+		`$VAR ${IFS} "dq" |&;<>`,
+	}
+	for _, in := range inputs {
+		out, err := exec.Command("sh", "-c", "printf %s "+shellQuote(in)).Output()
+		require.NoError(t, err, "input %q", in)
+		assert.Equal(t, in, string(out), "input %q did not round-trip", in)
+	}
+}