From 56c96a659658acdbb873aef8809a7b508434dcce Mon Sep 17 00:00:00 2001
From: Jeff Forcier <jeff@bitprophet.org>
Date: Tue, 18 Sep 2018 19:59:16 -0700
Subject: [PATCH] Fix and changelog re #1283

---
 paramiko/auth_handler.py | 36 ++++++++++++++++++++++++----
 tests/test_transport.py  | 51 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 94 insertions(+), 5 deletions(-)

--- a/paramiko/auth_handler.py
+++ b/paramiko/auth_handler.py
@@ -41,7 +41,7 @@ class AuthHandler (object):
     """
     Internal class to handle the mechanics of authentication.
     """
-    
+
     def __init__(self, transport):
         self.transport = weakref.proxy(transport)
         self.username = None
@@ -56,7 +56,7 @@ class AuthHandler (object):
         # for server mode:
         self.auth_username = None
         self.auth_fail_count = 0
-        
+
     def is_authenticated(self):
         return self.authenticated
 
@@ -97,7 +97,7 @@ class AuthHandler (object):
             self._request_auth()
         finally:
             self.transport.lock.release()
-    
+
     def auth_interactive(self, username, handler, event, submethods=''):
         """
         response_list = handler(title, instructions, prompt_list)
@@ -112,7 +112,7 @@ class AuthHandler (object):
             self._request_auth()
         finally:
             self.transport.lock.release()
-    
+
     def abort(self):
         if self.auth_event is not None:
             self.auth_event.set()
@@ -253,7 +253,7 @@ class AuthHandler (object):
             m.add_string(p[0])
             m.add_boolean(p[1])
         self.transport._send_message(m)
- 
+
     def _parse_userauth_request(self, m):
         if not self.transport.server_mode:
             # er, uh... what?
@@ -379,7 +379,7 @@ class AuthHandler (object):
         lang = m.get_string()
         self.transport._log(INFO, 'Auth banner: %s' % banner)
         # who cares.
-    
+
     def _parse_userauth_info_request(self, m):
         if self.auth_method != 'keyboard-interactive':
             raise SSHException('Illegal info request from server')
@@ -391,14 +391,14 @@ class AuthHandler (object):
         for i in range(prompts):
             prompt_list.append((m.get_text(), m.get_boolean()))
         response_list = self.interactive_handler(title, instructions, prompt_list)
-        
+
         m = Message()
         m.add_byte(cMSG_USERAUTH_INFO_RESPONSE)
         m.add_int(len(response_list))
         for r in response_list:
             m.add_string(r)
         self.transport._send_message(m)
-    
+
     def _parse_userauth_info_response(self, m):
         if not self.transport.server_mode:
             raise SSHException('Illegal info response from server')
@@ -413,13 +413,37 @@ class AuthHandler (object):
             return
         self._send_auth_result(self.auth_username, 'keyboard-interactive', result)
 
-    _handler_table = {
+    # TODO: do the same to the other tables, in Transport.
+    # TODO 3.0: MAY make sense to make these tables into actual
+    # classes/instances that can be fed a mode bool or whatever. Or,
+    # alternately (both?) make the message types small classes or enums that
+    # embed this info within themselves (which could also then tidy up the
+    # current 'integer -> human readable short string' stuff in common.py).
+    # TODO: if we do that, also expose 'em publicly.
+
+    # Messages which should be handled _by_ servers (sent by clients)
+    _server_handler_table = {
         MSG_SERVICE_REQUEST: _parse_service_request,
-        MSG_SERVICE_ACCEPT: _parse_service_accept,
         MSG_USERAUTH_REQUEST: _parse_userauth_request,
+        MSG_USERAUTH_INFO_RESPONSE: _parse_userauth_info_response,
+    }
+
+    # Messages which should be handled _by_ clients (sent by servers)
+    _client_handler_table = {
+        MSG_SERVICE_ACCEPT: _parse_service_accept,
         MSG_USERAUTH_SUCCESS: _parse_userauth_success,
         MSG_USERAUTH_FAILURE: _parse_userauth_failure,
         MSG_USERAUTH_BANNER: _parse_userauth_banner,
         MSG_USERAUTH_INFO_REQUEST: _parse_userauth_info_request,
-        MSG_USERAUTH_INFO_RESPONSE: _parse_userauth_info_response,
     }
+
+    # NOTE: prior to the fix for #1283, this was a static dict instead of a
+    # property. Should be backwards compatible in most/all cases.
+    @property
+    def _handler_table(self):
+        if self.transport.server_mode:
+            return self._server_handler_table
+        else:
+            return self._client_handler_table
+
+
--- a/tests/test_transport.py
+++ b/tests/test_transport.py
@@ -27,12 +27,19 @@ import time
 import threading
 import random
 
-from paramiko import Transport, SecurityOptions, ServerInterface, RSAKey, DSSKey, \
-    SSHException, ChannelException, Channel
+try:
+    from unittest.mock import Mock
+except ImportError:
+    from mock import Mock
+
+from paramiko import (Transport, SecurityOptions, ServerInterface, RSAKey, DSSKey,
+    SSHException, ChannelException, Channel, AuthHandler)
 from paramiko import AUTH_FAILED, AUTH_SUCCESSFUL
 from paramiko import OPEN_SUCCEEDED, OPEN_FAILED_ADMINISTRATIVELY_PROHIBITED
-from paramiko.common import MSG_KEXINIT, cMSG_CHANNEL_WINDOW_ADJUST
-from paramiko.py3compat import bytes
+from paramiko.common import (MSG_KEXINIT,
+    cMSG_CHANNEL_WINDOW_ADJUST, MSG_NAMES, cMSG_UNIMPLEMENTED, MSG_UNIMPLEMENTED,
+    MSG_USERAUTH_SUCCESS)
+from paramiko.py3compat import bytes, byte_chr
 from paramiko.message import Message
 from tests.loop import LoopSocket
 from tests.util import ParamikoTest, test_path
@@ -55,7 +62,7 @@ class NullServer (ServerInterface):
     paranoid_did_password = False
     paranoid_did_public_key = False
     paranoid_key = DSSKey.from_private_key_file(test_path('test_dss.key'))
-    
+
     def get_allowed_auths(self, username):
         if username == 'slowdive':
             return 'publickey,password'
@@ -78,7 +85,7 @@ class NullServer (ServerInterface):
 
     def check_channel_shell_request(self, channel):
         return True
-    
+
     def check_global_request(self, kind, msg):
         self._global_request = kind
         # NOTE: for w/e reason, older impl of this returned False always, even
@@ -86,20 +93,20 @@ class NullServer (ServerInterface):
         # For now, leaving that the default unless test supplies specific
         # 'acceptable' request kind
         return kind == 'acceptable'
-    
+
     def check_channel_x11_request(self, channel, single_connection, auth_protocol, auth_cookie, screen_number):
         self._x11_single_connection = single_connection
         self._x11_auth_protocol = auth_protocol
         self._x11_auth_cookie = auth_cookie
         self._x11_screen_number = screen_number
         return True
-    
+
     def check_port_forward_request(self, addr, port):
         self._listen = socket.socket()
         self._listen.bind(('127.0.0.1', 0))
         self._listen.listen(1)
         return self._listen.getsockname()[1]
-    
+
     def cancel_port_forward_request(self, addr, port):
         self._listen.close()
         self._listen = None
@@ -129,12 +136,12 @@ class TransportTest(ParamikoTest):
         host_key = RSAKey.from_private_key_file(test_path('test_rsa.key'))
         public_host_key = RSAKey(data=host_key.asbytes())
         self.ts.add_server_key(host_key)
-        
+
         if client_options is not None:
             client_options(self.tc.get_security_options())
         if server_options is not None:
             server_options(self.ts.get_security_options())
-        
+
         event = threading.Event()
         self.server = NullServer()
         self.assertTrue(not event.isSet())
@@ -166,7 +173,7 @@ class TransportTest(ParamikoTest):
             self.assertTrue(False)
         except TypeError:
             pass
-            
+
     def test_2_compute_key(self):
         self.tc.K = 123281095979686581523377256114209720774539068973101330872763622971399429481072519713536292772709507296759612401802191955568143056534122385270077606457721553469730659233569339356140085284052436697480759510519672848743794433460113118986816826624865291116513647975790797391795651716378444844877749505443714557929
         self.tc.H = b'\x0C\x83\x07\xCD\xE6\x85\x6F\xF3\x0B\xA9\x36\x84\xEB\x0F\x04\xC2\x52\x0E\x9E\xD3'
@@ -219,7 +226,7 @@ class TransportTest(ParamikoTest):
         event.wait(1.0)
         self.assertTrue(event.isSet())
         self.assertTrue(self.ts.is_active())
-        
+
     def test_4_special(self):
         """
         verify that the client can demand odd handshake settings, and can
@@ -233,7 +240,7 @@ class TransportTest(ParamikoTest):
         self.assertEqual('aes256-cbc', self.tc.remote_cipher)
         self.assertEqual(12, self.tc.packetizer.get_mac_size_out())
         self.assertEqual(12, self.tc.packetizer.get_mac_size_in())
-        
+
         self.tc.send_ignore(1024)
         self.tc.renegotiate_keys()
         self.ts.send_ignore(1024)
@@ -247,7 +254,7 @@ class TransportTest(ParamikoTest):
         self.tc.set_keepalive(1)
         time.sleep(2)
         self.assertEqual('keepalive@lag.net', self.server._global_request)
-        
+
     def test_6_exec_command(self):
         """
         verify that exec_command() does something reasonable.
@@ -261,7 +268,7 @@ class TransportTest(ParamikoTest):
             self.assertTrue(False)
         except SSHException:
             pass
-        
+
         chan = self.tc.open_session()
         chan.exec_command('yes')
         schan = self.ts.accept(1.0)
@@ -275,7 +282,7 @@ class TransportTest(ParamikoTest):
         f = chan.makefile_stderr()
         self.assertEqual('This is on stderr.\n', f.readline())
         self.assertEqual('', f.readline())
-        
+
         # now try it with combined stdout/stderr
         chan = self.tc.open_session()
         chan.exec_command('yes')
@@ -284,7 +291,7 @@ class TransportTest(ParamikoTest):
         schan.send_stderr('This is on stderr.\n')
         schan.close()
 
-        chan.set_combine_stderr(True)        
+        chan.set_combine_stderr(True)
         f = chan.makefile()
         self.assertEqual('Hello there.\n', f.readline())
         self.assertEqual('This is on stderr.\n', f.readline())
@@ -331,7 +338,7 @@ class TransportTest(ParamikoTest):
         schan.shutdown_write()
         schan.send_exit_status(23)
         schan.close()
-        
+
         f = chan.makefile()
         self.assertEqual('Hello there.\n', f.readline())
         self.assertEqual('', f.readline())
@@ -353,14 +360,14 @@ class TransportTest(ParamikoTest):
         chan.invoke_shell()
         schan = self.ts.accept(1.0)
 
-        # nothing should be ready        
+        # nothing should be ready
         r, w, e = select.select([chan], [], [], 0.1)
         self.assertEqual([], r)
         self.assertEqual([], w)
         self.assertEqual([], e)
-        
+
         schan.send('hello\n')
-        
+
         # something should be ready now (give it 1 second to appear)
         for i in range(10):
             r, w, e = select.select([chan], [], [], 0.1)
@@ -372,7 +379,7 @@ class TransportTest(ParamikoTest):
         self.assertEqual([], e)
 
         self.assertEqual(b'hello\n', chan.recv(6))
-        
+
         # and, should be dead again now
         r, w, e = select.select([chan], [], [], 0.1)
         self.assertEqual([], r)
@@ -380,7 +387,7 @@ class TransportTest(ParamikoTest):
         self.assertEqual([], e)
 
         schan.close()
-        
+
         # detect eof?
         for i in range(10):
             r, w, e = select.select([chan], [], [], 0.1)
@@ -391,14 +398,14 @@ class TransportTest(ParamikoTest):
         self.assertEqual([], w)
         self.assertEqual([], e)
         self.assertEqual(bytes(), chan.recv(16))
-        
+
         # make sure the pipe is still open for now...
         p = chan._pipe
         self.assertEqual(False, p._closed)
         chan.close()
         # ...and now is closed.
         self.assertEqual(True, p._closed)
-   
+
     def test_B_renegotiate(self):
         """
         verify that a transport can correctly renegotiate mid-stream.
@@ -413,7 +420,7 @@ class TransportTest(ParamikoTest):
         for i in range(20):
             chan.send('x' * 1024)
         chan.close()
-        
+
         # allow a few seconds for the rekeying to complete
         for i in range(50):
             if self.tc.H != self.tc.session_id:
@@ -452,28 +459,28 @@ class TransportTest(ParamikoTest):
         chan = self.tc.open_session()
         chan.exec_command('yes')
         schan = self.ts.accept(1.0)
-        
+
         requested = []
         def handler(c, addr_port):
             addr, port = addr_port
             requested.append((addr, port))
             self.tc._queue_incoming_channel(c)
-            
+
         self.assertEqual(None, getattr(self.server, '_x11_screen_number', None))
         cookie = chan.request_x11(0, single_connection=True, handler=handler)
         self.assertEqual(0, self.server._x11_screen_number)
         self.assertEqual('MIT-MAGIC-COOKIE-1', self.server._x11_auth_protocol)
         self.assertEqual(cookie, self.server._x11_auth_cookie)
         self.assertEqual(True, self.server._x11_single_connection)
-        
+
         x11_server = self.ts.open_x11_channel(('localhost', 6093))
         x11_client = self.tc.accept()
         self.assertEqual('localhost', requested[0][0])
         self.assertEqual(6093, requested[0][1])
-        
+
         x11_server.send('hello')
         self.assertEqual(b'hello', x11_client.recv(5))
-        
+
         x11_server.close()
         x11_client.close()
         chan.close()
@@ -488,13 +495,13 @@ class TransportTest(ParamikoTest):
         chan = self.tc.open_session()
         chan.exec_command('yes')
         schan = self.ts.accept(1.0)
-        
+
         requested = []
         def handler(c, origin_addr_port, server_addr_port):
             requested.append(origin_addr_port)
             requested.append(server_addr_port)
             self.tc._queue_incoming_channel(c)
-            
+
         port = self.tc.request_port_forward('127.0.0.1', 0, handler)
         self.assertEqual(port, self.server._listen.getsockname()[1])
 
@@ -503,14 +510,14 @@ class TransportTest(ParamikoTest):
         ss, _ = self.server._listen.accept()
         sch = self.ts.open_forwarded_tcpip_channel(ss.getsockname(), ss.getpeername())
         cch = self.tc.accept()
-        
+
         sch.send('hello')
         self.assertEqual(b'hello', cch.recv(5))
         sch.close()
         cch.close()
         ss.close()
         cs.close()
-        
+
         # now cancel it.
         self.tc.cancel_port_forward('127.0.0.1', port)
         self.assertTrue(self.server._listen is None)
@@ -524,7 +531,7 @@ class TransportTest(ParamikoTest):
         chan = self.tc.open_session()
         chan.exec_command('yes')
         schan = self.ts.accept(1.0)
-        
+
         # open a port on the "server" that the client will ask to forward to.
         greeting_server = socket.socket()
         greeting_server.bind(('127.0.0.1', 0))
@@ -535,13 +542,13 @@ class TransportTest(ParamikoTest):
         sch = self.ts.accept(1.0)
         cch = socket.socket()
         cch.connect(self.server._tcpip_dest)
-        
+
         ss, _ = greeting_server.accept()
         ss.send(b'Hello!\n')
         ss.close()
         sch.send(cch.recv(8192))
         sch.close()
-        
+
         self.assertEqual(b'Hello!\n', cs.recv(7))
         cs.close()
 
@@ -555,14 +562,14 @@ class TransportTest(ParamikoTest):
         chan.invoke_shell()
         schan = self.ts.accept(1.0)
 
-        # nothing should be ready        
+        # nothing should be ready
         r, w, e = select.select([chan], [], [], 0.1)
         self.assertEqual([], r)
         self.assertEqual([], w)
         self.assertEqual([], e)
-        
+
         schan.send_stderr('hello\n')
-        
+
         # something should be ready now (give it 1 second to appear)
         for i in range(10):
             r, w, e = select.select([chan], [], [], 0.1)
@@ -574,7 +581,7 @@ class TransportTest(ParamikoTest):
         self.assertEqual([], e)
 
         self.assertEqual(b'hello\n', chan.recv_stderr(6))
-        
+
         # and, should be dead again now
         r, w, e = select.select([chan], [], [], 0.1)
         self.assertEqual([], r)
@@ -610,10 +617,10 @@ class TransportTest(ParamikoTest):
     def test_I_rekey_deadlock(self):
         """
         Regression test for deadlock when in-transit messages are received after MSG_KEXINIT is sent
-        
+
         Note: When this test fails, it may leak threads.
         """
-        
+
         # Test for an obscure deadlocking bug that can occur if we receive
         # certain messages while initiating a key exchange.
         #
@@ -630,7 +637,7 @@ class TransportTest(ParamikoTest):
         #      NeedRekeyException.
         #   4. In response to NeedRekeyException, the transport thread sends
         #      MSG_KEXINIT to the remote host.
-        # 
+        #
         # On the remote host (using any SSH implementation):
         #   5. The MSG_CHANNEL_DATA is received, and MSG_CHANNEL_WINDOW_ADJUST is sent.
         #   6. The MSG_KEXINIT is received, and a corresponding MSG_KEXINIT is sent.
@@ -665,7 +672,7 @@ class TransportTest(ParamikoTest):
                 self.done_event = done_event
                 self.watchdog_event = threading.Event()
                 self.last = None
-            
+
             def run(self):
                 try:
                     for i in range(1, 1+self.iterations):
@@ -677,7 +684,7 @@ class TransportTest(ParamikoTest):
                 finally:
                     self.done_event.set()
                     self.watchdog_event.set()
-        
+
         class ReceiveThread(threading.Thread):
             def __init__(self, chan, done_event):
                 threading.Thread.__init__(self, None, None, self.__class__.__name__)
@@ -685,7 +692,7 @@ class TransportTest(ParamikoTest):
                 self.chan = chan
                 self.done_event = done_event
                 self.watchdog_event = threading.Event()
-            
+
             def run(self):
                 try:
                     while not self.done_event.isSet():
@@ -698,10 +705,10 @@ class TransportTest(ParamikoTest):
                 finally:
                     self.done_event.set()
                     self.watchdog_event.set()
-        
+
         self.setup_test_server()
         self.ts.packetizer.REKEY_BYTES = 2048
-        
+
         chan = self.tc.open_session()
         chan.exec_command('yes')
         schan = self.ts.accept(1.0)
@@ -723,7 +730,7 @@ class TransportTest(ParamikoTest):
                 self._send_message(m2)
             return _negotiate_keys(self, m)
         self.tc._handler_table[MSG_KEXINIT] = _negotiate_keys_wrapper
-        
+
         # Parameters for the test
         iterations = 500    # The deadlock does not happen every time, but it
                             # should after many iterations.
@@ -735,12 +742,12 @@ class TransportTest(ParamikoTest):
         # Start the sending thread
         st = SendThread(schan, iterations, done_event)
         st.start()
-        
+
         # Start the receiving thread
         rt = ReceiveThread(chan, done_event)
         rt.start()
 
-        # Act as a watchdog timer, checking 
+        # Act as a watchdog timer, checking
         deadlocked = False
         while not deadlocked and not done_event.isSet():
             for event in (st.watchdog_event, rt.watchdog_event):
@@ -751,7 +758,7 @@ class TransportTest(ParamikoTest):
                     deadlocked = True
                     break
                 event.clear()
-        
+
         # Tell the threads to stop (if they haven't already stopped).  Note
         # that if one or more threads are deadlocked, they might hang around
         # forever (until the process exits).
@@ -797,3 +804,48 @@ class TransportTest(ParamikoTest):
             assert "forwarding request denied" in str(e)
         else:
             assert False, "Did not raise SSHException!"
+
+    def _send_client_message(self, message_type):
+        self.setup_test_server(connect_kwargs={})
+        self.ts._send_message = Mock()
+        # NOTE: this isn't 100% realistic (most of these message types would
+        # have actual other fields in 'em) but it suffices to test the level of
+        # message dispatch we're interested in here.
+        msg = Message()
+        # TODO: really not liking the whole cMSG_XXX vs MSG_XXX duality right
+        # now, esp since the former is almost always just byte_chr(the
+        # latter)...but since that's the case...
+        msg.add_byte(byte_chr(message_type))
+        self.tc._send_message(msg)
+        # No good way to actually wait for server action (see above tests re:
+        # MSG_UNIMPLEMENTED). Grump.
+        time.sleep(0.1)
+
+    def _expect_unimplemented(self):
+        # Ensure MSG_UNIMPLEMENTED was sent (implies it hit end of loop instead
+        # of truly handling the given message).
+        # NOTE: When bug present, this will actually be the first thing that
+        # fails (since in many cases actual message handling doesn't involve
+        # sending a message back right away).
+        assert self.ts._send_message.call_count == 1
+        reply = self.ts._send_message.call_args[0][0]
+        reply.rewind()  # Because it's pre-send, not post-receive
+        assert reply.get_byte() == cMSG_UNIMPLEMENTED
+
+    def test_server_transports_reject_client_message_types(self):
+        # TODO: handle Transport's own tables too, not just its inner auth
+        # handler's table. See TODOs in auth_handler.py
+        for message_type in AuthHandler._client_handler_table:
+            self._send_client_message(message_type)
+            self._expect_unimplemented()
+            # Reset for rest of loop
+            self.tearDown()
+            self.setUp()
+
+    def test_server_rejects_client_MSG_USERAUTH_SUCCESS(self):
+        self._send_client_message(MSG_USERAUTH_SUCCESS)
+        # Sanity checks
+        assert not self.ts.authenticated
+        assert not self.ts.auth_handler.authenticated
+        # Real fix's behavior
+        self._expect_unimplemented()
