ssh: quote parameters using shellquote (SEC) stable
authorJun Wu <quark@fb.com>
Fri, 04 Aug 2017 23:54:12 -0700
branchstable
changeset 39178 8cb9e921ef8c4ffa388bc78ef49ac6f997b960a0
parent 39177 db83a1df03fe6f131773eb1813dd7ccd222f89a8
child 39179 3fee7f7d2da04226914c2258cc2884dc27384fd7
push id560
push usergszorc@mozilla.com
push dateFri, 11 Aug 2017 05:35:26 +0000
ssh: quote parameters using shellquote (SEC) This patch uses shellquote to quote ssh parameters more strictly to avoid shell injection.
mercurial/posix.py
mercurial/sshpeer.py
mercurial/windows.py
tests/test-clone.t
tests/test-ssh-bundle1.t
tests/test-ssh.t
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -87,20 +87,23 @@ def parsepatchoutput(output_line):
     else:
         if pf.startswith("'") and pf.endswith("'") and " " in pf:
             pf = pf[1:-1] # Remove the quotes
     return pf
 
 def sshargs(sshcmd, host, user, port):
     '''Build argument list for ssh'''
     args = user and ("%s@%s" % (user, host)) or host
-    if '-' in args[:2]:
+    if '-' in args[:1]:
         raise error.Abort(
             _('illegal ssh hostname or username starting with -: %s') % args)
-    return port and ("%s -p %s" % (args, port)) or args
+    args = shellquote(args)
+    if port:
+        args = '-p %s %s' % (shellquote(port), args)
+    return args
 
 def isexec(f):
     """check whether a file is executable"""
     return (os.lstat(f).st_mode & 0o100 != 0)
 
 def setflags(f, l, x):
     st = os.lstat(f)
     s = st.st_mode
--- a/mercurial/sshpeer.py
+++ b/mercurial/sshpeer.py
@@ -146,20 +146,17 @@ class sshpeer(wireproto.wirepeer):
             self._abort(error.RepoError(_("password in URL not supported")))
         self.host = u.host
         self.port = u.port
         self.path = u.path or "."
 
         sshcmd = self.ui.config("ui", "ssh")
         remotecmd = self.ui.config("ui", "remotecmd")
 
-        args = util.sshargs(sshcmd,
-                            _serverquote(self.host),
-                            _serverquote(self.user),
-                            _serverquote(self.port))
+        args = util.sshargs(sshcmd, self.host, self.user, self.port)
 
         if create:
             cmd = '%s %s %s' % (sshcmd, args,
                 util.shellquote("%s init %s" %
                     (_serverquote(remotecmd), _serverquote(self.path))))
             ui.debug('running %s\n' % cmd)
             res = ui.system(cmd, blockedtag='sshpeer')
             if res != 0:
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -203,17 +203,20 @@ def parsepatchoutput(output_line):
 def sshargs(sshcmd, host, user, port):
     '''Build argument list for ssh or Plink'''
     pflag = 'plink' in sshcmd.lower() and '-P' or '-p'
     args = user and ("%s@%s" % (user, host)) or host
     if args.startswith('-') or args.startswith('/'):
         raise error.Abort(
             _('illegal ssh hostname or username starting with - or /: %s') %
             args)
-    return port and ("%s %s %s" % (args, pflag, port)) or args
+    args = shellquote(args)
+    if port:
+        args = '%s %s %s' % (pflag, shellquote(port), args)
+    return args
 
 def setflags(f, l, x):
     pass
 
 def copymode(src, dst, mode=None):
     pass
 
 def checkexec(path):
--- a/tests/test-clone.t
+++ b/tests/test-clone.t
@@ -1095,27 +1095,68 @@ pooled".
   searching for changes
   no changes found
   adding remote bookmark bookA
   updating working directory
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
 SEC: check for unsafe ssh url
 
+  $ cat >> $HGRCPATH << EOF
+  > [ui]
+  > ssh = sh -c "read l; read l; read l"
+  > EOF
+
   $ hg clone 'ssh://-oProxyCommand=touch${IFS}owned/path'
   abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path'
   [255]
   $ hg clone 'ssh://%2DoProxyCommand=touch${IFS}owned/path'
   abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path'
   [255]
   $ hg clone 'ssh://fakehost|shellcommand/path'
   abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path'
   [255]
   $ hg clone 'ssh://fakehost%7Cshellcommand/path'
   abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path'
   [255]
 
   $ hg clone 'ssh://-oProxyCommand=touch owned%20foo@example.com/nonexistent/path'
   abort: potentially unsafe url: 'ssh://-oProxyCommand=touch owned foo@example.com/nonexistent/path'
   [255]
+
+#if windows
+  $ hg clone "ssh://%26touch%20owned%20/" --debug
+  running sh -c "read l; read l; read l" "&touch owned " "hg -R . serve --stdio"
+  sending hello command
+  sending between command
+  abort: no suitable response from remote hg!
+  [255]
+  $ hg clone "ssh://example.com:%26touch%20owned%20/" --debug
+  running sh -c "read l; read l; read l" -p "&touch owned " example.com "hg -R . serve --stdio"
+  sending hello command
+  sending between command
+  abort: no suitable response from remote hg!
+  [255]
+#else
+  $ hg clone "ssh://%3btouch%20owned%20/" --debug
+  running sh -c "read l; read l; read l" ';touch owned ' 'hg -R . serve --stdio'
+  sending hello command
+  sending between command
+  abort: no suitable response from remote hg!
+  [255]
+  $ hg clone "ssh://example.com:%3btouch%20owned%20/" --debug
+  running sh -c "read l; read l; read l" -p ';touch owned ' example.com 'hg -R . serve --stdio'
+  sending hello command
+  sending between command
+  abort: no suitable response from remote hg!
+  [255]
+#endif
+
+  $ hg clone "ssh://v-alid.example.com/" --debug
+  running sh -c "read l; read l; read l" v-alid\.example\.com ['"]hg -R \. serve --stdio['"] (re)
+  sending hello command
+  sending between command
+  abort: no suitable response from remote hg!
+  [255]
+
 We should not have created a file named owned - if it exists, the
 attack succeeded.
   $ if test -f owned; then echo 'you got owned'; fi
--- a/tests/test-ssh-bundle1.t
+++ b/tests/test-ssh-bundle1.t
@@ -456,17 +456,17 @@ stderr from remote commands should be pr
   remote: added 1 changesets with 1 changes to 1 files
   remote: KABOOM
   local stdout
 
 debug output
 
   $ hg pull --debug ssh://user@dummy/remote
   pulling from ssh://user@dummy/remote
-  running .* ".*/dummyssh" user@dummy ('|")hg -R remote serve --stdio('|") (re)
+  running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re)
   sending hello command
   sending between command
   remote: 355
   remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN
   remote: 1
   preparing listkeys for "bookmarks"
   sending listkeys command
   received listkey for "bookmarks": 45 bytes
--- a/tests/test-ssh.t
+++ b/tests/test-ssh.t
@@ -472,17 +472,17 @@ stderr from remote commands should be pr
   remote: KABOOM
   remote: KABOOM IN PROCESS
   local stdout
 
 debug output
 
   $ hg pull --debug ssh://user@dummy/remote
   pulling from ssh://user@dummy/remote
-  running .* ".*/dummyssh" user@dummy ('|")hg -R remote serve --stdio('|") (re)
+  running .* ".*/dummyssh" ['"]user@dummy['"] ('|")hg -R remote serve --stdio('|") (re)
   sending hello command
   sending between command
   remote: 355
   remote: capabilities: lookup changegroupsubset branchmap pushkey known getbundle unbundlehash batch streamreqs=generaldelta,revlogv1 bundle2=HG20%0Achangegroup%3D01%2C02%0Adigests%3Dmd5%2Csha1%2Csha512%0Aerror%3Dabort%2Cunsupportedcontent%2Cpushraced%2Cpushkey%0Ahgtagsfnodes%0Alistkeys%0Apushkey%0Aremote-changegroup%3Dhttp%2Chttps unbundle=HG10GZ,HG10BZ,HG10UN
   remote: 1
   query 1; heads
   sending batch command
   searching for changes