Improve error messages related to sshuttle server. (#580)

* Improve error messages related to sshuttle server.

There are many GitHub issues related to the cryptic message:
fatal: expected server init string 'SSHUTTLE0001'; got b''

The code that prints that message is after another check that is
intended to verify that the server is still running. This code was
faulty since the server is still running when rv==None (but exited
when rv==0).

I corrected this problem and then investigated ways to clarify the
error message. I added additional exit codes for the server: 97 (exec
in the shell returned), 98 (the python exec() function called
returned). The end result is that the cryptic error message above will
now print a more appropriate error message that should aid in
debugging.

I also changed the server so that it catches Fatal() and exits with
exit code 99 (like the client does). Previously, it was just an
unhandled exception on the server.

I suspect some of the error messages were caused by restricted shells.
I also investigated and added comments about how sshuttle might behave
if it is being run on a server that has a restricted shell.

This commit also replaces a couple of exit() calls in cmdline.py with
'return' since exit() is intended for interactive use. This change
doesn't impact the server.

* Remind user to exclude remote host when server exits with 255.

4 files changed, 270 insertions, 137 deletions

diff --git a/sshuttle/client.py b/sshuttle/client.py
index d67d0a6..99e6092 100644
--- a/sshuttle/client.py
+++ b/sshuttle/client.py
@@ -485,9 +485,85 @@ def _main(tcp_listener, udp_listener, fw, ssh_cmd, remotename,
         else:
             raise
 
+    # Returns None if process is still running (or returns exit code)
     rv = serverproc.poll()
-    if rv:
-        raise Fatal('server died with error code %d' % rv)
+    if rv is not None:
+        errmsg = "server died with error code %d\n" % rv
+
+        # Our fatal exceptions return exit code 99
+        if rv == 99:
+            errmsg += "This error code likely means that python started and " \
+                "the sshuttle server started. However, the sshuttle server " \
+                "may have raised a 'Fatal' exception after it started."
+        elif rv == 98:
+            errmsg += "This error code likely means that we were able to " \
+                "run python on the server, but that the program continued " \
+                "to the line after we call python's exec() to execute " \
+                "sshuttle's server code. Try specifying the python " \
+                "executable to user on the server by passing --python " \
+                "to sshuttle."
+
+        # This error should only be possible when --python is not specified.
+        elif rv == 97 and not python:
+            errmsg += "This error code likely means that either we " \
+                "couldn't find python3 or python in the PATH on the " \
+                "server or that we do not have permission to run 'exec' in " \
+                "the /bin/sh shell on the server. Try specifying the " \
+                "python executable to use on the server by passing " \
+                "--python to sshuttle."
+
+        # POSIX sh standards says error code 127 is used when you try
+        # to execute a program that does not exist. See section 2.8.2
+        # of
+        # https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08
+        elif rv == 127:
+            if python:
+                errmsg += "This error code likely means that we were not " \
+                    "able to execute the python executable that specified " \
+                    "with --python. You specified '%s'.\n" % python
+                if python.startswith("/"):
+                    errmsg += "\nTip for users in a restricted shell on the " \
+                        "server: The server may refuse to run programs " \
+                        "specified with an absolute path. Try specifying " \
+                        "just the name of the python executable. However, " \
+                        "if python is not in your PATH and you cannot " \
+                        "run programs specified with an absolute path, " \
+                        "it is possible that sshuttle will not work."
+            else:
+                errmsg += "This error code likely means that we were unable " \
+                    "to execute /bin/sh on the remote server. This can " \
+                    "happen if /bin/sh does not exist on the server or if " \
+                    "you are in a restricted shell that does not allow you " \
+                    "to run programs specified with an absolute path. " \
+                    "Try rerunning sshuttle with the --python parameter."
+
+        # When the redirected subnet includes the remote ssh host, the
+        # firewall rules can interrupt the ssh connection to the
+        # remote machine. This issue impacts some Linux machines. The
+        # user sees that the server dies with a broken pipe error and
+        # code 255.
+        #
+        # The solution to this problem is to exclude the remote
+        # server.
+        #
+        # There are many github issues from users encountering this
+        # problem. Most of the discussion on the topic is here:
+        # https://github.com/sshuttle/sshuttle/issues/191
+        elif rv == 255:
+            errmsg += "It might be possible to resolve this error by " \
+                "excluding the server that you are ssh'ing to. For example, " \
+                "if you are running 'sshuttle -v -r example.com 0/0' to " \
+                "redirect all traffic through example.com, then try " \
+                "'sshuttle -v -r example.com -x example.com 0/0' to " \
+                "exclude redirecting the connection to example.com itself " \
+                "(i.e., sshuttle's firewall rules may be breaking the " \
+                "ssh connection that it previously established). " \
+                "Alternatively, you may be able to use 'sshuttle -v -r " \
+                "example.com -x example.com:22 0/0' to redirect " \
+                "everything except ssh connections between your machine " \
+                "and example.com."
+
+        raise Fatal(errmsg)
 
     if initstring != expected:
         raise Fatal('expected server init string %r; got %r'
diff --git a/sshuttle/cmdline.py b/sshuttle/cmdline.py
index 70685c1..c7438ad 100644
--- a/sshuttle/cmdline.py
+++ b/sshuttle/cmdline.py
@@ -17,11 +17,11 @@ def main():
     if opt.sudoers or opt.sudoers_no_modify:
         if platform.platform().startswith('OpenBSD'):
             log('Automatic sudoers does not work on BSD')
-            exit(1)
+            return 1
 
         if not opt.sudoers_filename:
             log('--sudoers-file must be set or omited.')
-            exit(1)
+            return 1
 
         sudoers(
             user_name=opt.sudoers_user,
diff --git a/sshuttle/server.py b/sshuttle/server.py
index 61ebc53..ccebc72 100644
--- a/sshuttle/server.py
+++ b/sshuttle/server.py
@@ -272,139 +272,147 @@ class UdpProxy(Handler):
 
 
 def main(latency_control, auto_hosts, to_nameserver, auto_nets):
-    helpers.logprefix = ' s: '
-
-    debug1('Starting server with Python version %s'
-           % platform.python_version())
-    debug1('latency control setting = %r' % latency_control)
-
-    # synchronization header
-    sys.stdout.write('\0\0SSHUTTLE0001')
-    sys.stdout.flush()
-
-    handlers = []
-    mux = Mux(sys.stdin, sys.stdout)
-    handlers.append(mux)
+    try:
+        helpers.logprefix = ' s: '
+        debug1('Starting server with Python version %s'
+               % platform.python_version())
+
+        debug1('latency control setting = %r' % latency_control)
+
+        # synchronization header
+        sys.stdout.write('\0\0SSHUTTLE0001')
+        sys.stdout.flush()
+
+        handlers = []
+        mux = Mux(sys.stdin, sys.stdout)
+        handlers.append(mux)
+
+        debug1('auto-nets:' + str(auto_nets))
+        if auto_nets:
+            routes = list(list_routes())
+            debug1('available routes:')
+            for r in routes:
+                debug1('  %d/%s/%d' % r)
+        else:
+            routes = []
 
-    debug1('auto-nets:' + str(auto_nets))
-    if auto_nets:
-        routes = list(list_routes())
-        debug1('available routes:')
+        routepkt = ''
         for r in routes:
-            debug1('  %d/%s/%d' % r)
-    else:
-        routes = []
-
-    routepkt = ''
-    for r in routes:
-        routepkt += '%d,%s,%d\n' % r
-    mux.send(0, ssnet.CMD_ROUTES, b(routepkt))
-
-    hw = Hostwatch()
-    hw.leftover = b('')
-
-    def hostwatch_ready(sock):
-        assert(hw.pid)
-        content = hw.sock.recv(4096)
-        if content:
-            lines = (hw.leftover + content).split(b('\n'))
-            if lines[-1]:
-                # no terminating newline: entry isn't complete yet!
-                hw.leftover = lines.pop()
-                lines.append(b(''))
+            routepkt += '%d,%s,%d\n' % r
+        mux.send(0, ssnet.CMD_ROUTES, b(routepkt))
+
+        hw = Hostwatch()
+        hw.leftover = b('')
+
+        def hostwatch_ready(sock):
+            assert(hw.pid)
+            content = hw.sock.recv(4096)
+            if content:
+                lines = (hw.leftover + content).split(b('\n'))
+                if lines[-1]:
+                    # no terminating newline: entry isn't complete yet!
+                    hw.leftover = lines.pop()
+                    lines.append(b(''))
+                else:
+                    hw.leftover = b('')
+                mux.send(0, ssnet.CMD_HOST_LIST, b('\n').join(lines))
             else:
-                hw.leftover = b('')
-            mux.send(0, ssnet.CMD_HOST_LIST, b('\n').join(lines))
-        else:
-            raise Fatal('hostwatch process died')
-
-    def got_host_req(data):
-        if not hw.pid:
-            (hw.pid, hw.sock) = start_hostwatch(
-                    data.decode("ASCII").strip().split(), auto_hosts)
-            handlers.append(Handler(socks=[hw.sock],
-                                    callback=hostwatch_ready))
-    mux.got_host_req = got_host_req
-
-    def new_channel(channel, data):
-        (family, dstip, dstport) = data.decode("ASCII").split(',', 2)
-        family = int(family)
-        # AF_INET is the same constant on Linux and BSD but AF_INET6
-        # is different. As the client and server can be running on
-        # different platforms we can not just set the socket family
-        # to what comes in the wire.
-        if family != socket.AF_INET:
-            family = socket.AF_INET6
-        dstport = int(dstport)
-        outwrap = ssnet.connect_dst(family, dstip, dstport)
-        handlers.append(Proxy(MuxWrapper(mux, channel), outwrap))
-    mux.new_channel = new_channel
-
-    dnshandlers = {}
-
-    def dns_req(channel, data):
-        debug2('Incoming DNS request channel=%d.' % channel)
-        h = DnsProxy(mux, channel, data, to_nameserver)
-        handlers.append(h)
-        dnshandlers[channel] = h
-    mux.got_dns_req = dns_req
-
-    udphandlers = {}
-
-    def udp_req(channel, cmd, data):
-        debug2('Incoming UDP request channel=%d, cmd=%d' % (channel, cmd))
-        if cmd == ssnet.CMD_UDP_DATA:
-            (dstip, dstport, data) = data.split(b(','), 2)
+                raise Fatal('hostwatch process died')
+
+        def got_host_req(data):
+            if not hw.pid:
+                (hw.pid, hw.sock) = start_hostwatch(
+                        data.decode("ASCII").strip().split(), auto_hosts)
+                handlers.append(Handler(socks=[hw.sock],
+                                        callback=hostwatch_ready))
+        mux.got_host_req = got_host_req
+
+        def new_channel(channel, data):
+            (family, dstip, dstport) = data.decode("ASCII").split(',', 2)
+            family = int(family)
+            # AF_INET is the same constant on Linux and BSD but AF_INET6
+            # is different. As the client and server can be running on
+            # different platforms we can not just set the socket family
+            # to what comes in the wire.
+            if family != socket.AF_INET:
+                family = socket.AF_INET6
             dstport = int(dstport)
-            debug2('is incoming UDP data. %r %d.' % (dstip, dstport))
-            h = udphandlers[channel]
-            h.send((dstip, dstport), data)
-        elif cmd == ssnet.CMD_UDP_CLOSE:
-            debug2('is incoming UDP close')
-            h = udphandlers[channel]
-            h.ok = False
-            del mux.channels[channel]
-
-    def udp_open(channel, data):
-        debug2('Incoming UDP open.')
-        family = int(data)
-        mux.channels[channel] = lambda cmd, data: udp_req(channel, cmd, data)
-        if channel in udphandlers:
-            raise Fatal('UDP connection channel %d already open' % channel)
-        else:
-            h = UdpProxy(mux, channel, family)
+            outwrap = ssnet.connect_dst(family, dstip, dstport)
+            handlers.append(Proxy(MuxWrapper(mux, channel), outwrap))
+        mux.new_channel = new_channel
+
+        dnshandlers = {}
+
+        def dns_req(channel, data):
+            debug2('Incoming DNS request channel=%d.' % channel)
+            h = DnsProxy(mux, channel, data, to_nameserver)
             handlers.append(h)
-            udphandlers[channel] = h
-    mux.got_udp_open = udp_open
-
-    while mux.ok:
-        if hw.pid:
-            assert(hw.pid > 0)
-            (rpid, rv) = os.waitpid(hw.pid, os.WNOHANG)
-            if rpid:
-                raise Fatal(
-                    'hostwatch exited unexpectedly: code 0x%04x' % rv)
-
-        ssnet.runonce(handlers, mux)
-        if latency_control:
-            mux.check_fullness()
-
-        if dnshandlers:
-            now = time.time()
-            remove = []
-            for channel, h in dnshandlers.items():
-                if h.timeout < now or not h.ok:
-                    debug3('expiring dnsreqs channel=%d' % channel)
-                    remove.append(channel)
-                    h.ok = False
-            for channel in remove:
-                del dnshandlers[channel]
-        if udphandlers:
-            remove = []
-            for channel, h in udphandlers.items():
-                if not h.ok:
-                    debug3('expiring UDP channel=%d' % channel)
-                    remove.append(channel)
-                    h.ok = False
-            for channel in remove:
-                del udphandlers[channel]
+            dnshandlers[channel] = h
+        mux.got_dns_req = dns_req
+
+        udphandlers = {}
+
+        def udp_req(channel, cmd, data):
+            debug2('Incoming UDP request channel=%d, cmd=%d' %
+                   (channel, cmd))
+            if cmd == ssnet.CMD_UDP_DATA:
+                (dstip, dstport, data) = data.split(b(','), 2)
+                dstport = int(dstport)
+                debug2('is incoming UDP data. %r %d.' % (dstip, dstport))
+                h = udphandlers[channel]
+                h.send((dstip, dstport), data)
+            elif cmd == ssnet.CMD_UDP_CLOSE:
+                debug2('is incoming UDP close')
+                h = udphandlers[channel]
+                h.ok = False
+                del mux.channels[channel]
+
+        def udp_open(channel, data):
+            debug2('Incoming UDP open.')
+            family = int(data)
+            mux.channels[channel] = lambda cmd, data: udp_req(channel, cmd,
+                                                              data)
+            if channel in udphandlers:
+                raise Fatal('UDP connection channel %d already open' %
+                            channel)
+            else:
+                h = UdpProxy(mux, channel, family)
+                handlers.append(h)
+                udphandlers[channel] = h
+        mux.got_udp_open = udp_open
+
+        while mux.ok:
+            if hw.pid:
+                assert(hw.pid > 0)
+                (rpid, rv) = os.waitpid(hw.pid, os.WNOHANG)
+                if rpid:
+                    raise Fatal(
+                        'hostwatch exited unexpectedly: code 0x%04x' % rv)
+
+            ssnet.runonce(handlers, mux)
+            if latency_control:
+                mux.check_fullness()
+
+            if dnshandlers:
+                now = time.time()
+                remove = []
+                for channel, h in dnshandlers.items():
+                    if h.timeout < now or not h.ok:
+                        debug3('expiring dnsreqs channel=%d' % channel)
+                        remove.append(channel)
+                        h.ok = False
+                for channel in remove:
+                    del dnshandlers[channel]
+            if udphandlers:
+                remove = []
+                for channel, h in udphandlers.items():
+                    if not h.ok:
+                        debug3('expiring UDP channel=%d' % channel)
+                        remove.append(channel)
+                        h.ok = False
+                for channel in remove:
+                    del udphandlers[channel]
+
+    except Fatal as e:
+        log('fatal: %s' % e)
+        sys.exit(99)
diff --git a/sshuttle/ssh.py b/sshuttle/ssh.py
index a735e97..5c3fc96 100644
--- a/sshuttle/ssh.py
+++ b/sshuttle/ssh.py
@@ -103,11 +103,21 @@ def connect(ssh_cmd, rhostport, python, stderr, options):
                 empackage(z, 'sshuttle.server') +
                 b"\n")
 
+    # If the exec() program calls sys.exit(), it should exit python
+    # and the sys.exit(98) call won't be reached (so we try to only
+    # exit that way in the server). However, if the code that we
+    # exec() simply returns from main, then we will return from
+    # exec(). If the server's python process dies, it should stop
+    # executing and also won't reach sys.exit(98).
+    #
+    # So, we shouldn't reach sys.exit(98) and we certainly shouldn't
+    # reach it immediately after trying to start the server.
     pyscript = r"""
                 import sys, os;
                 verbosity=%d;
                 sys.stdin = os.fdopen(0, "rb");
-                exec(compile(sys.stdin.read(%d), "assembler.py", "exec"))
+                exec(compile(sys.stdin.read(%d), "assembler.py", "exec"));
+                sys.exit(98);
                 """ % (helpers.verbose or 0, len(content))
     pyscript = re.sub(r'\s+', ' ', pyscript.strip())
 
@@ -127,8 +137,47 @@ def connect(ssh_cmd, rhostport, python, stderr, options):
         if python:
             pycmd = "'%s' -c '%s'" % (python, pyscript)
         else:
+            # By default, we run the following code in a shell.
+            # However, with restricted shells and other unusual
+            # situations, there can be trouble. See the RESTRICTED
+            # SHELL section in "man bash" for more information. The
+            # code makes many assumptions:
+            #
+            # (1) That /bin/sh exists and that we can call it.
+            # Restricted shells often do *not* allow you to run
+            # programs specified with an absolute path like /bin/sh.
+            # Either way, if there is trouble with this, it should
+            # return error code 127.
+            #
+            # (2) python3 or python exists in the PATH and is
+            # executable. If they aren't, then exec wont work (see (4)
+            # below).
+            #
+            # (3) In /bin/sh, that we can redirect stderr in order to
+            # hide the version that "python3 -V" might print (some
+            # restricted shells don't allow redirection, see
+            # RESTRICTED SHELL section in 'man bash'). However, if we
+            # are in a restricted shell, we'd likely have trouble with
+            # assumption (1) above.
+            #
+            # (4) The 'exec' command should work except if we failed
+            # to exec python because it doesn't exist or isn't
+            # executable OR if exec isn't allowed (some restricted
+            # shells don't allow exec). If the exec succeeded, it will
+            # not return and not get to the "exit 97" command. If exec
+            # does return, we exit with code 97.
+            #
+            # Specifying the exact python program to run with --python
+            # avoids many of the issues above. However, if
+            # you have a restricted shell on remote, you may only be
+            # able to run python if it is in your PATH (and you can't
+            # run programs specified with an absolute path). In that
+            # case, sshuttle might not work at all since it is not
+            # possible to run python on the remote machine---even if
+            # it is present.
             pycmd = ("P=python3; $P -V 2>%s || P=python; "
-                     "exec \"$P\" -c %s") % (os.devnull, quote(pyscript))
+                     "exec \"$P\" -c %s; exit 97") % \
+                     (os.devnull, quote(pyscript))
             pycmd = ("/bin/sh -c {}".format(quote(pycmd)))
 
         if password is not None: