Prevent SSH option/shell injection via store URLs#351
Merged
Conversation
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 <args>" usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SFTPStore(sftp.go) andRemoteSSH(remotessh.go) build their ssh destination from the store URL (user@host) and pass it as the first positional argument tossh. The destination is never validated or guarded with--.-(e.g.sftp://-oProxyCommand=touch${IFS}pwned/) is parsed bysshas a command-line option rather than a destination, leading to arbitrary local command execution as the desync process user. Same class as Git CVE-2017-1000117 and Mercurial CVE-2017-1000116. Store URLs come from CLI flags and from--store-fileJSON config thatchunk-server/mount-indexre-read onSIGHUP, so a less-privileged writer of that file can trigger it.remotessh.gointerpolated the URL path into the remote command string with only single-quote wrapping (fmt.Sprintf("%s pull - - - '%s'", remoteCmd, path)), so a'in the path breaks out and injects into thesh -cthe remote runs.Verified on OpenSSH 9.9p1:
ssh -oProxyCommand=id hostparses as an option, whilessh -- -oProxyCommand=id hosttreats it as a destination.Fix
Applied at the library boundary (
newSFTPStoreBase,StartProtocol) so both the CLI and direct library users are protected;sftpindex.gosharesnewSFTPStoreBaseand is covered automatically.validateSSHHostrejects any destination beginning with-. This is the portable guarantee — it also protects whenCASYNC_SSH_PATHpoints at an ssh implementation that doesn't honor--.--before the destination as defense in depth (ssh -s -- <host> sftp,ssh -- <host> <cmd>). Confirmed the reordered sftp invocation still invokes the subsystem.shellQuotePOSIX-escapes the path before embedding it in the remote command string.CASYNC_REMOTE_PATHis left interpolated unquoted on purpose: it is operator-controlled and quoting it would break legitimatecasync <args>usage.Tests
New
sshcmd_test.go:TestValidateSSHHost— rejects-,-x,-oProxyCommand=...; acceptshost,user@host,host:port, mid-string dashes.TestShellQuote— round-trips adversarial inputs (o'brien,a'; touch pwned; ',$VAR ${IFS} "dq" |&;<>, etc.) through a realsh -cand asserts byte-for-byte equality.go test ./...andgo build -o cmd/desync/ ./cmd/desyncpass.