From ae8af71886edd8f5cdcd812a839267ee973c3864 Mon Sep 17 00:00:00 2001 From: Scott Kuhl Date: Fri, 7 Jan 2022 11:35:37 -0500 Subject: [PATCH 1/4] Gracefully exit if firewall process receives Ctrl+C/SIGINT. Typically sshuttle exits by having the main sshuttle client process terminated. This closes file descriptors which the firewall process then sees and uses as a cue to cleanup the firewall rules. The firewall process ignored SIGINT/SIGTERM signals and used setsid() to prevent Ctrl+C from sending signals to the firewall process. This patch makes the firewall process accept SIGINT/SIGTERM signals and then in turn sends a SIGINT signal to the main sshuttle client process which then triggers a regular shutdown as described above. This allows a user to manually send a SIGINT/SIGTERM to either sshuttle process and have it exit gracefully. It also is needed if setsid() fails (known to occur if sudo's use_pty option is used) and then the Ctrl+C SIGINT signal goes to the firewall process. The PID of the sshuttle client process is sent to the firewall process. Using os.getppid() in the firewall process doesn't correctly return the sshuttle client PID. --- sshuttle/client.py | 4 ++-- sshuttle/firewall.py | 31 +++++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/sshuttle/client.py b/sshuttle/client.py index f9bf04b..abc3836 100644 --- a/sshuttle/client.py +++ b/sshuttle/client.py @@ -298,8 +298,8 @@ class FirewallClient: else: user = b'%d' % self.user - self.pfile.write(b'GO %d %s %s\n' % - (udp, user, bytes(self.tmark, 'ascii'))) + self.pfile.write(b'GO %d %s %s %d\n' % + (udp, user, bytes(self.tmark, 'ascii'), os.getpid())) self.pfile.flush() line = self.pfile.readline() diff --git a/sshuttle/firewall.py b/sshuttle/firewall.py index d3806cd..fb9471d 100644 --- a/sshuttle/firewall.py +++ b/sshuttle/firewall.py @@ -13,7 +13,7 @@ from sshuttle.helpers import debug1, debug2, Fatal from sshuttle.methods import get_auto_method, get_method HOSTSFILE = '/etc/hosts' - +sshuttle_pid = None def rewrite_etc_hosts(hostmap, port): BAKFILE = '%s.sbak' % HOSTSFILE @@ -55,6 +55,23 @@ def restore_etc_hosts(hostmap, port): debug2('undoing /etc/hosts changes.') rewrite_etc_hosts({}, port) +def firewall_exit(signum, frame): + # The typical sshuttle exit is that the main sshuttle process + # exits, closes file descriptors it uses, and the firewall process + # notices that it can't read from stdin anymore and exits + # (cleaning up firewall rules). + # + # However, in some cases, Ctrl+C might get sent to the firewall + # process. This might caused if someone manually tries to kill the + # firewall process, or if sshuttle was started using sudo's use_pty option + # and they try to exit by pressing Ctrl+C. Here, we forward the + # Ctrl+C/SIGINT to the main sshuttle process which should trigger + # the typical exit process as described above. + global sshuttle_pid + if sshuttle_pid: + debug1("Relaying SIGINT to sshuttle process %d\n" % sshuttle_pid) + os.kill(sshuttle_pid, signal.SIGINT) + # Isolate function that needs to be replaced for tests def setup_daemon(): @@ -65,8 +82,8 @@ def setup_daemon(): # disappears; we still have to clean up. signal.signal(signal.SIGHUP, signal.SIG_IGN) signal.signal(signal.SIGPIPE, signal.SIG_IGN) - signal.signal(signal.SIGTERM, signal.SIG_IGN) - signal.signal(signal.SIGINT, signal.SIG_IGN) + signal.signal(signal.SIGTERM, firewall_exit) + signal.signal(signal.SIGINT, firewall_exit) # ctrl-c shouldn't be passed along to me. When the main sshuttle dies, # I'll die automatically. @@ -230,12 +247,14 @@ def main(method_name, syslog): raise Fatal('expected GO but got %r' % line) _, _, args = line.partition(" ") - udp, user, tmark = args.strip().split(" ", 2) + global sshuttle_pid + udp, user, tmark, sshuttle_pid = args.strip().split(" ", 3) udp = bool(int(udp)) + sshuttle_pid = int(sshuttle_pid) if user == '-': user = None - debug2('Got udp: %r, user: %r, tmark: %s' % - (udp, user, tmark)) + debug2('Got udp: %r, user: %r, tmark: %s, sshuttle_pid: %d' % + (udp, user, tmark, sshuttle_pid)) subnets_v6 = [i for i in subnets if i[0] == socket.AF_INET6] nslist_v6 = [i for i in nslist if i[0] == socket.AF_INET6] From 286bd3fa8058adc5fa475e4dd032ba54a029308c Mon Sep 17 00:00:00 2001 From: Scott Kuhl Date: Fri, 7 Jan 2022 12:14:57 -0500 Subject: [PATCH 2/4] Make setsid() call in firewall process optional. We previously called setsid() to ensure that the SIGINT generated by Ctrl+C went to the main sshuttle process instead of the firewall process. With the previous commit, we gracefully shutdown if either the sshuttle process or firewall process receives a SIGINT. Therefore, the setsid() call is optional. We still try calling setsid() since the preferred shutdown process involves having the signal go to the main sshuttle process. However, setsid() will fail if the firewall process is started with sudo and sudo is configured with the use_pty option. --- sshuttle/firewall.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/sshuttle/firewall.py b/sshuttle/firewall.py index fb9471d..28167e3 100644 --- a/sshuttle/firewall.py +++ b/sshuttle/firewall.py @@ -85,16 +85,17 @@ def setup_daemon(): signal.signal(signal.SIGTERM, firewall_exit) signal.signal(signal.SIGINT, firewall_exit) - # ctrl-c shouldn't be passed along to me. When the main sshuttle dies, - # I'll die automatically. + # Calling setsid() here isn't strictly necessary. However, it forces + # Ctrl+C to get sent to the main sshuttle process instead of to + # the firewall process---which is our preferred way to shutdown. + # Nonetheless, if the firewall process receives a SIGTERM/SIGINT + # signal, it will relay a SIGINT to the main sshuttle process + # automatically. try: os.setsid() except OSError: - raise Fatal("setsid() failed. This may occur if you are using sudo's " - "use_pty option. sshuttle does not currently work with " - "this option. An imperfect workaround: Run the sshuttle " - "command with sudo instead of running it as a regular " - "user and entering the sudo password when prompted.") + # setsid() fails if sudo is configured with the use_pty option. + pass # because of limitations of the 'su' command, the *real* stdin/stdout # are both attached to stdout initially. Clone stdout into stdin so we From 8e826cfa7d64f5baa60c074d63e9bfe7076c5cd3 Mon Sep 17 00:00:00 2001 From: Scott Kuhl Date: Fri, 7 Jan 2022 13:05:42 -0500 Subject: [PATCH 3/4] Print to console with \r\n line endings. If we run sudo with the use_pty option, the firewall process is started in a new pseudoterminal. Other processes that are still printing to the terminal (i.e., the main sshuttle client process, messages from the shuttle server) have their output incorreclty displayed. A newline character simply moves the output to the next line without returning the cursor to the beginning of the line. Simply changing all print commands to use \r\n line endings fixes the problem and does not appear to cause any trouble in other configurations. --- sshuttle/assembler.py | 2 +- sshuttle/helpers.py | 22 +++++++++++++--------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/sshuttle/assembler.py b/sshuttle/assembler.py index 3cffdee..6eb8800 100644 --- a/sshuttle/assembler.py +++ b/sshuttle/assembler.py @@ -18,7 +18,7 @@ while 1: name = name.decode("ASCII") nbytes = int(sys.stdin.readline()) if verbosity >= 2: - sys.stderr.write(' s: assembling %r (%d bytes)\n' + sys.stderr.write(' s: assembling %r (%d bytes)\r\n' % (name, nbytes)) content = z.decompress(sys.stdin.read(nbytes)) diff --git a/sshuttle/helpers.py b/sshuttle/helpers.py index 372feb3..8ff536a 100644 --- a/sshuttle/helpers.py +++ b/sshuttle/helpers.py @@ -18,15 +18,19 @@ def log(s): # Put newline at end of string if line doesn't have one. if not s.endswith("\n"): s = s+"\n" - # Allow multi-line messages - if s.find("\n") != -1: - prefix = logprefix - s = s.rstrip("\n") - for line in s.split("\n"): - sys.stderr.write(prefix + line + "\n") - prefix = " " - else: - sys.stderr.write(logprefix + s) + + prefix = logprefix + s = s.rstrip("\n") + for line in s.split("\n"): + # We output with \r\n instead of \n because when we use + # sudo with the use_pty option, the firewall process, the + # other processes printing to the terminal will have the + # \n move to the next line, but they will fail to reset + # cursor to the beginning of the line. Printing output + # with \r\n endings fixes that problem and does not appear + # to cause problems elsewhere. + sys.stderr.write(prefix + line + "\r\n") + prefix = " " sys.stderr.flush() except IOError: # this could happen if stderr gets forcibly disconnected, eg. because From 80a822e07921b0d1325bec0b28f4001550ebddd2 Mon Sep 17 00:00:00 2001 From: Scott Kuhl Date: Fri, 7 Jan 2022 13:21:16 -0500 Subject: [PATCH 4/4] Fix flake8 and unit test errors introduced by use_pty fixes. --- sshuttle/firewall.py | 2 ++ tests/client/test_firewall.py | 2 +- tests/client/test_helpers.py | 24 ++++++++++++------------ 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/sshuttle/firewall.py b/sshuttle/firewall.py index 28167e3..84b249d 100644 --- a/sshuttle/firewall.py +++ b/sshuttle/firewall.py @@ -15,6 +15,7 @@ from sshuttle.methods import get_auto_method, get_method HOSTSFILE = '/etc/hosts' sshuttle_pid = None + def rewrite_etc_hosts(hostmap, port): BAKFILE = '%s.sbak' % HOSTSFILE APPEND = '# sshuttle-firewall-%d AUTOCREATED' % port @@ -55,6 +56,7 @@ def restore_etc_hosts(hostmap, port): debug2('undoing /etc/hosts changes.') rewrite_etc_hosts({}, port) + def firewall_exit(signum, frame): # The typical sshuttle exit is that the main sshuttle process # exits, closes file descriptors it uses, and the firewall process diff --git a/tests/client/test_firewall.py b/tests/client/test_firewall.py index d249361..76aac89 100644 --- a/tests/client/test_firewall.py +++ b/tests/client/test_firewall.py @@ -15,7 +15,7 @@ NSLIST {inet},1.2.3.33 {inet6},2404:6800:4004:80c::33 PORTS 1024,1025,1026,1027 -GO 1 - 0x01 +GO 1 - 0x01 12345 HOST 1.2.3.3,existing """.format(inet=AF_INET, inet6=AF_INET6)) stdout = Mock() diff --git a/tests/client/test_helpers.py b/tests/client/test_helpers.py index 45e7ea5..8e69d45 100644 --- a/tests/client/test_helpers.py +++ b/tests/client/test_helpers.py @@ -24,19 +24,19 @@ def test_log(mock_stderr, mock_stdout): call.flush(), ] assert mock_stderr.mock_calls == [ - call.write('prefix: message\n'), + call.write('prefix: message\r\n'), call.flush(), - call.write('prefix: abc\n'), + call.write('prefix: abc\r\n'), call.flush(), - call.write('prefix: message 1\n'), + call.write('prefix: message 1\r\n'), call.flush(), - call.write('prefix: message 2\n'), - call.write(' line2\n'), - call.write(' line3\n'), + call.write('prefix: message 2\r\n'), + call.write(' line2\r\n'), + call.write(' line3\r\n'), call.flush(), - call.write('prefix: message 3\n'), - call.write(' line2\n'), - call.write(' line3\n'), + call.write('prefix: message 3\r\n'), + call.write(' line2\r\n'), + call.write(' line3\r\n'), call.flush(), ] @@ -51,7 +51,7 @@ def test_debug1(mock_stderr, mock_stdout): call.flush(), ] assert mock_stderr.mock_calls == [ - call.write('prefix: message\n'), + call.write('prefix: message\r\n'), call.flush(), ] @@ -76,7 +76,7 @@ def test_debug2(mock_stderr, mock_stdout): call.flush(), ] assert mock_stderr.mock_calls == [ - call.write('prefix: message\n'), + call.write('prefix: message\r\n'), call.flush(), ] @@ -101,7 +101,7 @@ def test_debug3(mock_stderr, mock_stdout): call.flush(), ] assert mock_stderr.mock_calls == [ - call.write('prefix: message\n'), + call.write('prefix: message\r\n'), call.flush(), ]