From ad85e4ba8e14e53d64bcea80c7ac8721e22eedae Mon Sep 17 00:00:00 2001 From: folbrich Date: Tue, 19 May 2026 12:30:39 +0200 Subject: [PATCH] Prevent SSH option/shell injection via store URLs SFTPStore and RemoteSSH built their ssh destination from the store URL (user@host) and passed it as the first positional argument to ssh. A URL whose host begins with '-' (e.g. sftp://-oProxyCommand=.../) was parsed by ssh as a command-line option instead of a destination, allowing arbitrary local command execution as the desync process user -- the same class of bug as Git CVE-2017-1000117 and Mercurial CVE-2017-1000116. Store URLs come from CLI flags and from --store-file JSON config re-read on SIGHUP, so a less-privileged writer of that file could trigger it. RemoteSSH additionally interpolated the URL path into the remote command string with only single-quote wrapping, so a single quote in the path allowed remote-side shell injection. Fixes: - validateSSHHost rejects any destination beginning with '-' (portable guarantee, covers non-OpenSSH CASYNC_SSH_PATH implementations). - Pass "--" before the destination so ssh can't parse it as an option (defense in depth). For sftp, "-s" now precedes "--". - shellQuote escapes the path before embedding it in the remote command. CASYNC_REMOTE_PATH is still interpolated unquoted; it is operator- controlled and quoting it would break legitimate "casync " usage. --- remotessh.go | 9 ++++++++- sftp.go | 7 ++++++- sshcmd.go | 29 +++++++++++++++++++++++++++ sshcmd_test.go | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 sshcmd.go create mode 100644 sshcmd_test.go diff --git a/remotessh.go b/remotessh.go index e9b730f2..86cd8f73 100644 --- a/remotessh.go +++ b/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 { diff --git a/sftp.go b/sftp.go index ed6e35d6..d82b90ec 100644 --- a/sftp.go +++ b/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 { diff --git a/sshcmd.go b/sshcmd.go new file mode 100644 index 00000000..08518622 --- /dev/null +++ b/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, "'", `'\''`) + "'" +} diff --git a/sshcmd_test.go b/sshcmd_test.go new file mode 100644 index 00000000..2e129a56 --- /dev/null +++ b/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) + } +}