diff --git a/dev-requirements.txt b/dev-requirements.txt index 80e821345..a7218e9a0 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -9,3 +9,4 @@ twine>=1.11.0 flake8==2.6.2 pytest==3.2.1 pytest_relaxed==1.0.0 +mock==2.0.0 diff --git a/paramiko/transport.py b/paramiko/transport.py index ddcb2912a..4c44ad370 100644 --- a/paramiko/transport.py +++ b/paramiko/transport.py @@ -51,6 +51,7 @@ MSG_CHANNEL_WINDOW_ADJUST, MSG_CHANNEL_REQUEST, MSG_CHANNEL_EOF, MSG_CHANNEL_CLOSE, MIN_WINDOW_SIZE, MIN_PACKET_SIZE, MAX_WINDOW_SIZE, 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 @@ -1950,12 +1951,20 @@ def run(self): 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] + warning = "Oops, unhandled type {} ({!r})".format( + ptype, name + ) + self._log(WARNING, warning) + 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 be058308c..2413af31e 100644 --- a/sites/www/changelog.rst +++ b/sites/www/changelog.rst @@ -2,6 +2,11 @@ Changelog ========= +- :bug:`-` Modify protocol message handling such that ``Transport`` does not + respond to ``MSG_UNIMPLEMENTED`` with its own ``MSG_UNIMPLEMENTED``. 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). * :support:`1262 backported` Add ``*.pub`` files to the MANIFEST so distributed source packages contain some necessary test assets. Credit: Alexander Kapshuna. diff --git a/tests/test_transport.py b/tests/test_transport.py index 9474acfc3..42f18e1c6 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -30,6 +30,7 @@ import random from hashlib import sha1 import unittest +from mock import Mock from paramiko import ( Transport, SecurityOptions, ServerInterface, RSAKey, DSSKey, SSHException, @@ -38,8 +39,14 @@ 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, MIN_PACKET_SIZE, MIN_WINDOW_SIZE, - MAX_WINDOW_SIZE, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_PACKET_SIZE, + MSG_KEXINIT, + cMSG_CHANNEL_WINDOW_ADJUST, + cMSG_UNIMPLEMENTED, + MIN_PACKET_SIZE, + MIN_WINDOW_SIZE, + MAX_WINDOW_SIZE, + DEFAULT_WINDOW_SIZE, + DEFAULT_MAX_PACKET_SIZE, ) from paramiko.py3compat import bytes from paramiko.message import Message @@ -974,3 +981,26 @@ def test_server_rejects_port_forward_without_auth(self): 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)