diff --git a/mercurial/util.py b/mercurial/util.py --- a/mercurial/util.py +++ b/mercurial/util.py @@ -2905,8 +2905,7 @@ def checksafessh(path): Raises an error.Abort when the url is unsafe. """ path = urlreq.unquote(path) - if (path.startswith('ssh://-') or path.startswith('svn+ssh://-') - or '|' in path): + if path.startswith('ssh://-') or path.startswith('svn+ssh://-'): raise error.Abort(_('potentially unsafe url: %r') % (path,)) diff --git a/tests/test-clone.t b/tests/test-clone.t --- a/tests/test-clone.t +++ b/tests/test-clone.t @@ -1111,11 +1111,11 @@ SEC: check for unsafe ssh url $ 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' + $ hg clone 'ssh://fakehost|touch%20owned/path' + abort: no suitable response from remote hg! [255] - $ hg clone 'ssh://fakehost%7Cshellcommand/path' - abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path' + $ hg clone 'ssh://fakehost%7Ctouch%20owned/path' + abort: no suitable response from remote hg! [255] $ hg clone 'ssh://-oProxyCommand=touch owned%20foo@example.com/nonexistent/path' diff --git a/tests/test-pull.t b/tests/test-pull.t --- a/tests/test-pull.t +++ b/tests/test-pull.t @@ -107,6 +107,11 @@ regular shell commands. SEC: check for unsafe ssh url + $ cat >> $HGRCPATH << EOF + > [ui] + > ssh = sh -c "read l; read l; read l" + > EOF + $ hg pull 'ssh://-oProxyCommand=touch${IFS}owned/path' pulling from ssh://-oProxyCommand%3Dtouch%24%7BIFS%7Downed/path abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path' @@ -115,13 +120,15 @@ SEC: check for unsafe ssh url pulling from ssh://-oProxyCommand%3Dtouch%24%7BIFS%7Downed/path abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path' [255] - $ hg pull 'ssh://fakehost|shellcommand/path' - pulling from ssh://fakehost%7Cshellcommand/path - abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path' + $ hg pull 'ssh://fakehost|touch${IFS}owned/path' + pulling from ssh://fakehost%7Ctouch%24%7BIFS%7Downed/path + abort: no suitable response from remote hg! [255] - $ hg pull 'ssh://fakehost%7Cshellcommand/path' - pulling from ssh://fakehost%7Cshellcommand/path - abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path' + $ hg pull 'ssh://fakehost%7Ctouch%20owned/path' + pulling from ssh://fakehost%7Ctouch%20owned/path + abort: no suitable response from remote hg! [255] + $ [ ! -f owned ] || echo 'you got owned' + $ cd .. diff --git a/tests/test-push.t b/tests/test-push.t --- a/tests/test-push.t +++ b/tests/test-push.t @@ -319,6 +319,11 @@ Test bare push with multiple race checki SEC: check for unsafe ssh url + $ cat >> $HGRCPATH << EOF + > [ui] + > ssh = sh -c "read l; read l; read l" + > EOF + $ hg -R test-revflag push 'ssh://-oProxyCommand=touch${IFS}owned/path' pushing to ssh://-oProxyCommand%3Dtouch%24%7BIFS%7Downed/path abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path' @@ -327,11 +332,13 @@ SEC: check for unsafe ssh url pushing to ssh://-oProxyCommand%3Dtouch%24%7BIFS%7Downed/path abort: potentially unsafe url: 'ssh://-oProxyCommand=touch${IFS}owned/path' [255] - $ hg -R test-revflag push 'ssh://fakehost|shellcommand/path' - pushing to ssh://fakehost%7Cshellcommand/path - abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path' + $ hg -R test-revflag push 'ssh://fakehost|touch${IFS}owned/path' + pushing to ssh://fakehost%7Ctouch%24%7BIFS%7Downed/path + abort: no suitable response from remote hg! [255] - $ hg -R test-revflag push 'ssh://fakehost%7Cshellcommand/path' - pushing to ssh://fakehost%7Cshellcommand/path - abort: potentially unsafe url: 'ssh://fakehost|shellcommand/path' + $ hg -R test-revflag push 'ssh://fakehost%7Ctouch%20owned/path' + pushing to ssh://fakehost%7Ctouch%20owned/path + abort: no suitable response from remote hg! [255] + + $ [ ! -f owned ] || echo 'you got owned' diff --git a/tests/test-subrepo-git.t b/tests/test-subrepo-git.t --- a/tests/test-subrepo-git.t +++ b/tests/test-subrepo-git.t @@ -1213,27 +1213,3 @@ also check that a percent encoded '-' (% updating to branch default abort: potentially unsafe url: 'ssh://-oProxyCommand=rm${IFS}non-existent/path' (in subrepository "s") [255] - -also check for a pipe - - $ cd malicious-proxycommand - $ echo 's = [git]ssh://fakehost|shell/path' > .hgsub - $ hg ci -m 'change url to pipe' - $ cd .. - $ rm -r malicious-proxycommand-clone - $ hg clone malicious-proxycommand malicious-proxycommand-clone - updating to branch default - abort: potentially unsafe url: 'ssh://fakehost|shell/path' (in subrepository "s") - [255] - -also check that a percent encoded '|' (%7C) doesn't work - - $ cd malicious-proxycommand - $ echo 's = [git]ssh://fakehost%7Cshell/path' > .hgsub - $ hg ci -m 'change url to percent encoded' - $ cd .. - $ rm -r malicious-proxycommand-clone - $ hg clone malicious-proxycommand malicious-proxycommand-clone - updating to branch default - abort: potentially unsafe url: 'ssh://fakehost|shell/path' (in subrepository "s") - [255] diff --git a/tests/test-subrepo-svn.t b/tests/test-subrepo-svn.t --- a/tests/test-subrepo-svn.t +++ b/tests/test-subrepo-svn.t @@ -668,30 +668,6 @@ also check that a percent encoded '-' (% abort: potentially unsafe url: 'svn+ssh://-oProxyCommand=touch owned nested' (in subrepository "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 subrepository "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 subrepository "s") - [255] - also check that hiding the attack in the username doesn't work: $ cd ssh-vuln diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t --- a/tests/test-subrepo.t +++ b/tests/test-subrepo.t @@ -1792,6 +1792,11 @@ Test that '[paths]' is configured correc test for ssh exploit 2017-07-25 + $ cat >> $HGRCPATH << EOF + > [ui] + > ssh = sh -c "read l; read l; read l" + > EOF + $ hg init malicious-proxycommand $ cd malicious-proxycommand $ echo 's = [hg]ssh://-oProxyCommand=touch${IFS}owned/path' > .hgsub @@ -1825,26 +1830,28 @@ also check that a percent encoded '-' (% also check for a pipe $ cd malicious-proxycommand - $ echo 's = [hg]ssh://fakehost|shell/path' > .hgsub + $ echo 's = [hg]ssh://fakehost|touch${IFS}owned/path' > .hgsub $ hg ci -m 'change url to pipe' $ cd .. $ rm -r malicious-proxycommand-clone $ hg clone malicious-proxycommand malicious-proxycommand-clone updating to branch default - abort: potentially unsafe url: 'ssh://fakehost|shell/path' (in subrepository "s") + abort: no suitable response from remote hg! [255] + $ [ ! -f owned ] || echo 'you got owned' also check that a percent encoded '|' (%7C) doesn't work $ cd malicious-proxycommand - $ echo 's = [hg]ssh://fakehost%7Cshell/path' > .hgsub + $ echo 's = [hg]ssh://fakehost%7Ctouch%20owned/path' > .hgsub $ hg ci -m 'change url to percent encoded pipe' $ cd .. $ rm -r malicious-proxycommand-clone $ hg clone malicious-proxycommand malicious-proxycommand-clone updating to branch default - abort: potentially unsafe url: 'ssh://fakehost|shell/path' (in subrepository "s") + abort: no suitable response from remote hg! [255] + $ [ ! -f owned ] || echo 'you got owned' and bad usernames: $ cd malicious-proxycommand