# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 2017-08-07 13:22:28
# Node ID 3fee7f7d2da04226914c2258cc2884dc27384fd7
# Parent  8cb9e921ef8c4ffa388bc78ef49ac6f997b960a0

ssh: unban the use of pipe character in user@host:port string

This vulnerability was fixed by the previous patch and there were more ways
to exploit than using '|shellcmd'. So it doesn't make sense to reject only
pipe character.

Test cases are updated to actually try to exploit the bug. As the SSH bridge
of git/svn subrepos are not managed by our code, the tests for non-hg subrepos
are just removed.

This may be folded into the original patches.

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