changeset 33641:173ecccb9ee7 stable

subrepo: add tests for svn rogue ssh urls (SEC) 'ssh://' has an exploit that will pass the url blindly to the ssh command, allowing a malicious person to have a subrepo with '-oProxyCommand' which could run arbitrary code on a user's machine. In addition, at least on Windows, a pipe '|' is able to execute arbitrary commands. When this happens, let's throw a big abort into the user's face so that they can inspect what's going on.
author Sean Farley <sean@farley.io>
date Mon, 31 Jul 2017 16:44:17 -0700
parents 55681baf4cf9
children ca398a50ca00
files mercurial/subrepo.py mercurial/util.py tests/test-subrepo-svn.t
diffstat 3 files changed, 70 insertions(+), 1 deletions(-) [+]
line wrap: on
line diff
--- a/mercurial/subrepo.py	Mon Jul 31 16:04:44 2017 -0700
+++ b/mercurial/subrepo.py	Mon Jul 31 16:44:17 2017 -0700
@@ -1274,6 +1274,10 @@
         # The revision must be specified at the end of the URL to properly
         # update to a directory which has since been deleted and recreated.
         args.append('%s@%s' % (state[0], state[1]))
+
+        # SEC: check that the ssh url is safe
+        util.checksafessh(state[0])
+
         status, err = self._svncommand(args, failok=True)
         _sanitize(self.ui, self.wvfs, '.svn')
         if not re.search('Checked out revision [0-9]+.', status):
--- a/mercurial/util.py	Mon Jul 31 16:04:44 2017 -0700
+++ b/mercurial/util.py	Mon Jul 31 16:44:17 2017 -0700
@@ -2890,7 +2890,8 @@
     Raises an error.Abort when the url is unsafe.
     """
     path = urlreq.unquote(path)
-    if path.startswith('ssh://-') or '|' in path:
+    if (path.startswith('ssh://-') or path.startswith('svn+ssh://-')
+        or '|' in path):
         raise error.Abort(_('potentially unsafe url: %r') %
                           (path,))
 
--- a/tests/test-subrepo-svn.t	Mon Jul 31 16:04:44 2017 -0700
+++ b/tests/test-subrepo-svn.t	Mon Jul 31 16:44:17 2017 -0700
@@ -639,3 +639,67 @@
   $ hg update -q -C '.^1'
 
   $ cd ../..
+
+SEC: test for ssh exploit
+
+  $ hg init ssh-vuln
+  $ cd ssh-vuln
+  $ echo "s = [svn]$SVNREPOURL/src" >> .hgsub
+  $ svn co --quiet "$SVNREPOURL"/src s
+  $ hg add .hgsub
+  $ hg ci -m1
+  $ echo "s = [svn]svn+ssh://-oProxyCommand=touch%20owned%20nested" > .hgsub
+  $ hg ci -m2
+  $ cd ..
+  $ hg clone ssh-vuln ssh-vuln-clone
+  updating to branch default
+  abort: potentially unsafe url: 'svn+ssh://-oProxyCommand=touch owned nested' (in subrepo s)
+  [255]
+
+also check that a percent encoded '-' (%2D) doesn't work
+
+  $ cd ssh-vuln
+  $ echo "s = [svn]svn+ssh://%2DoProxyCommand=touch%20owned%20nested" > .hgsub
+  $ hg ci -m3
+  $ cd ..
+  $ rm -r ssh-vuln-clone
+  $ hg clone ssh-vuln ssh-vuln-clone
+  updating to branch default
+  abort: potentially unsafe url: 'svn+ssh://-oProxyCommand=touch owned nested' (in subrepo s)
+  [255]
+
+also check for a pipe
+
+  $ cd ssh-vuln
+  $ echo "s = [svn]svn+ssh://fakehost|sh%20nested" > .hgsub
+  $ hg ci -m3
+  $ cd ..
+  $ rm -r ssh-vuln-clone
+  $ hg clone ssh-vuln ssh-vuln-clone
+  updating to branch default
+  abort: potentially unsafe url: 'svn+ssh://fakehost|sh nested' (in subrepo s)
+  [255]
+
+also check that a percent encoded '|' (%7C) doesn't work
+
+  $ cd ssh-vuln
+  $ echo "s = [svn]svn+ssh://fakehost%7Csh%20nested" > .hgsub
+  $ hg ci -m3
+  $ cd ..
+  $ rm -r ssh-vuln-clone
+  $ hg clone ssh-vuln ssh-vuln-clone
+  updating to branch default
+  abort: potentially unsafe url: 'svn+ssh://fakehost|sh nested' (in subrepo s)
+  [255]
+
+also check that hiding the attack in the username doesn't work:
+
+  $ cd ssh-vuln
+  $ echo "s = [svn]svn+ssh://%2DoProxyCommand=touch%20owned%20foo@example.com/nested" > .hgsub
+  $ hg ci -m3
+  $ cd ..
+  $ rm -r ssh-vuln-clone
+  $ hg clone ssh-vuln ssh-vuln-clone
+  updating to branch default
+  abort: potentially unsafe url: 'svn+ssh://-oProxyCommand=touch owned foo@example.com/nested' (in subrepo s)
+  [255]