Merge branch '2.3' into 2.4

5 files changed, 143 insertions, 11 deletions

diff --git a/dev-requirements.txt b/dev-requirements.txt
index 7f2ab0b9..8814627f 100644
--- a/dev-requirements.txt
+++ b/dev-requirements.txt
@@ -6,6 +6,7 @@ pytest>=3.2,<3.3
 pytest-relaxed==1.1.4
 # pytest-xdist for test dir watching and the inv guard task
 pytest-xdist>=1.22,<2.0
+mock==2.0.0
 # Linting!
 flake8==2.4.0
 # Coverage!
diff --git a/paramiko/auth_handler.py b/paramiko/auth_handler.py
index 3f0456e5..26605a16 100644
--- a/paramiko/auth_handler.py
+++ b/paramiko/auth_handler.py
@@ -705,17 +705,39 @@ Error Message: {}
             self.auth_event.set()
         return
 
-    _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
+
 
 class GssapiWithMicAuthHandler(object):
     """A specialized Auth handler for gssapi-with-mic
@@ -811,9 +833,15 @@ class GssapiWithMicAuthHandler(object):
         self._restore_delegate_auth_handler()
         return self._delegate._parse_userauth_request(m)
 
-    _handler_table = {
+    __handler_table = {
         MSG_SERVICE_REQUEST: _parse_service_request,
         MSG_USERAUTH_REQUEST: _parse_userauth_request,
         MSG_USERAUTH_GSSAPI_TOKEN: _parse_userauth_gssapi_token,
         MSG_USERAUTH_GSSAPI_MIC: _parse_userauth_gssapi_mic,
     }
+
+    @property
+    def _handler_table(self):
+        # TODO: determine if we can cut this up like we did for the primary
+        # AuthHandler class.
+        return self.__handler_table
diff --git a/paramiko/transport.py b/paramiko/transport.py
index aa3b8dcc..410ff273 100644
--- a/paramiko/transport.py
+++ b/paramiko/transport.py
@@ -82,6 +82,8 @@ from paramiko.common import (
     DEFAULT_WINDOW_SIZE,
     DEFAULT_MAX_PACKET_SIZE,
     HIGHEST_USERAUTH_MESSAGE_ID,
+    MSG_UNIMPLEMENTED,
+    MSG_NAMES,
 )
 from paramiko.compress import ZlibCompressor, ZlibDecompressor
 from paramiko.dsskey import DSSKey
@@ -2035,12 +2037,22 @@ class Transport(threading.Thread, ClosingContextManager):
                         if len(self._expected_packet) > 0:
                             continue
                     else:
-                        err = "Oops, unhandled type {:d}".format(ptype)
-                        self._log(WARNING, err)
-                        msg = Message()
-                        msg.add_byte(cMSG_UNIMPLEMENTED)
-                        msg.add_int(m.seqno)
-                        self._send_message(msg)
+                        # Respond with "I don't implement this particular
+                        # message type" message (unless the message type was
+                        # itself literally MSG_UNIMPLEMENTED, in which case, we
+                        # just shut up to avoid causing a useless loop).
+                        name = MSG_NAMES[ptype]
+                        self._log(
+                            WARNING,
+                            "Oops, unhandled type {} ({!r})".format(
+                                ptype, name
+                            ),
+                        )
+                        if ptype != MSG_UNIMPLEMENTED:
+                            msg = Message()
+                            msg.add_byte(cMSG_UNIMPLEMENTED)
+                            msg.add_int(m.seqno)
+                            self._send_message(msg)
                     self.packetizer.complete_handshake()
             except SSHException as e:
                 self._log(ERROR, "Exception: " + str(e))
diff --git a/sites/www/changelog.rst b/sites/www/changelog.rst
index 67a8bb00..be488de6 100644
--- a/sites/www/changelog.rst
+++ b/sites/www/changelog.rst
@@ -2,6 +2,23 @@
 Changelog
 =========
 
+- :bug:`-` Modify protocol message handling such that ``Transport`` does not
+  respond to ``MSG_UNIMPLEMENTED`` with its own ``MSG_UNIMPLEMENTED`` message.
+  This behavior probably didn't cause any outright errors, but it doesn't seem
+  to conform to the RFCs and could cause (non-infinite) feedback loops in some
+  scenarios (usually those involving Paramiko on both ends).
+- :bug:`1283 (1.17+)` Fix exploit (CVE pending) in Paramiko's server mode
+  (**not** client mode) where hostile clients could trick the server into
+  thinking they were authenticated without actually submitting valid
+  authentication.
+
+  Specifically, steps have been taken to start separating client and server
+  related message types in the message handling tables within ``Transport`` and
+  ``AuthHandler``; this work is not complete but enough has been performed to
+  close off this particular exploit (which was the only obvious such exploit
+  for this particular channel).
+
+  Thanks to Daniel Hoffman for the detailed report.
 - :support:`1292 backported` Backport changes from :issue:`979` (added in
   Paramiko 2.3) to Paramiko 2.0-2.2, using duck-typing to preserve backwards
   compatibility. This allows these older versions to use newer Cryptography
diff --git a/tests/test_transport.py b/tests/test_transport.py
index 13fb302e..2b8ee3bc 100644
--- a/tests/test_transport.py
+++ b/tests/test_transport.py
@@ -30,6 +30,7 @@ import threading
 import random
 from hashlib import sha1
 import unittest
+from mock import Mock
 
 from paramiko import (
     Transport,
@@ -41,19 +42,24 @@ from paramiko import (
     ChannelException,
     Packetizer,
     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,
+    cMSG_UNIMPLEMENTED,
     MIN_PACKET_SIZE,
     MIN_WINDOW_SIZE,
     MAX_WINDOW_SIZE,
     DEFAULT_WINDOW_SIZE,
     DEFAULT_MAX_PACKET_SIZE,
+    MSG_NAMES,
+    MSG_UNIMPLEMENTED,
+    MSG_USERAUTH_SUCCESS,
 )
-from paramiko.py3compat import bytes
+from paramiko.py3compat import bytes, byte_chr
 from paramiko.message import Message
 
 from .util import needs_builtin, _support, slow
@@ -1027,3 +1033,71 @@ class TransportTest(unittest.TestCase):
             assert "forwarding request denied" in str(e)
         else:
             assert False, "Did not raise SSHException!"
+
+    def _send_unimplemented(self, server_is_sender):
+        self.setup_test_server()
+        sender, recipient = self.tc, self.ts
+        if server_is_sender:
+            sender, recipient = self.ts, self.tc
+        recipient._send_message = Mock()
+        msg = Message()
+        msg.add_byte(cMSG_UNIMPLEMENTED)
+        sender._send_message(msg)
+        # TODO: I hate this but I literally don't see a good way to know when
+        # the recipient has received the sender's message (there are no
+        # existing threading events in play that work for this), esp in this
+        # case where we don't WANT a response (as otherwise we could
+        # potentially try blocking on the sender's receipt of a reply...maybe).
+        time.sleep(0.1)
+        assert not recipient._send_message.called
+
+    def test_server_does_not_respond_to_MSG_UNIMPLEMENTED(self):
+        self._send_unimplemented(server_is_sender=False)
+
+    def test_client_does_not_respond_to_MSG_UNIMPLEMENTED(self):
+        self._send_unimplemented(server_is_sender=True)
+
+    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()