Sending SSH client version & KEX before receiving server version does not work with some SSH servers

[philmayers]

First of all - let me start by saying that I know what the SSH2 RFCs say, and I know that the behaviour below is legal per-spec. Nonetheless, using current versions, it is impossible to connect to some SSH servers, for example the SSH server in Cisco IOS 12.2(33)SXJ1.

I am hopeful that, despite this being a server bug, you will accept a workaround. I note that this workaround will make the Twisted Conch code work the same as the OpenSSH code, which from a compatibility point of view, can only be a good thing.

So: the current conch code has:

class SSHTransportBase(protocol.Protocol):
  def connectionMade(self):
    # code to send our version string & KEX

This can result in the following behaviour on the wire:

C: SYN
S: SYN,ACK
C: ACK
C: PSH "SSH-2.0-Twisted\n<kex packet>"
S: PSH "SSH-1.99-Cisco-1.25\n"

...and the connection hangs. Clearly IOS is buggy; it is obviously doing something where it is not starting to read on the TCP socket until after the banner has been written.

On faster Cisco platforms (e.g. newer 6500s) the Cisco box "wins the race" and the hang does not happen, but I'm sure the bug is still there.

Speaking from experience, Cisco WILL NOT FIX this bug in anything less than 3-5 years. There is no realistic chance of using Twisted to talk to affected boxes without a fix client-side.

The fix can be done with a relatively trivial sub-class e.g.

class ClientTransport(transport.SSHClientTransport):

    # We MUST NOT send our banner until after they've sent theirs
    bannerSent = False

    def connectionMade(self):
        log.msg('ssh connection made')

    def connectionMade2(self):
        transport.SSHClientTransport.connectionMade(self)

    def dataReceived(self, data):
        if not self.bannerSent:
            self.bannerSent = True
            self.connectionMade2()
        return transport.SSHClientTransport.dataReceived(self, data)

However, this is unsatisfactory for two reasons. Firstly, it requires overriding connectionMade but not calling the superclass method until later on. This might cause problems if the connectionMade method ever does more work. Secondly, the simple override of dataReceived above is naive, and does not actually parse the received data to ensure the banner has been wholly received.

Rather than ask for a hacky workaround, can I suggest that the transport base class and subclass be modified along the following lines:

class SSHTransportBase(protocol.Protocol):
  def sendVersionKex(self):
    """send our own version & KEX packet"""
  def bannerReceived(self, banner):
    """called when the banner is received; can be overridden"""
  def connectionMade(self):
    # any non client/server specific setup

class SSHServerTransport(SSHTransportBase):
  def connectionMade(self):
    SSHTransportBase.connectionMade(self)
    # only send the the version/kex at connect in servers
    self.sendVersionKex()

class SSHClientTransport(SSHTransportBase):
  def bannerReceived(self, banner):
    # wait for the banner to send version/kex in clients
    self.sendVersionKex()

If this proposal is acceptable, I'll work up a patch.

[philmayers]

One point I meant to emphasise - the suggested behaviour is what OpenSSH does. See the "ssh_login" function in sshconnect.c of a recent version - it basically does this:

/* Exchange protocol version identification strings with the server. */
ssh_exchange_identification(timeout_ms);

/* Put the connection into non-blocking mode. */
packet_set_nonblocking();

/* key exchange */
/* authenticate user */
if (compat20) {
        ssh_kex2(...);
        ssh_userauth2(local_user, server_user, host, sensitive);

[exarkun]

Sounds good to me. Interop for the win. No SSH server is going to wait for the client KEX before sending its banner, right? The only downside is a little more latency in connection setup. That seems like a cost worth paying to be able to talk to other server implementations more reliably.

[desaster]

I'm also relying on a workaround like this, since my goal is to imitate a real OpenSSH system at an early phase for honeypot reasons ( https://code.google.com/p/kippo/).

At twisted version 11 the workaround broke, and I had to modify it a bit. Not sure if it's a clean fix, but at the moment it seems to work:

    hadVersion = False

    def sendKexInit(self):
        # Don't send key exchange prematurely
        if not self.gotVersion:
            return
        transport.SSHServerTransport.sendKexInit(self)

    def dataReceived(self, data):
        transport.SSHServerTransport.dataReceived(self, data)
        # later versions seem to call sendKexInit again on their own
        if twisted.version.major >= 11 and \
                not self.hadVersion and self.gotVersion:
            self.sendKexInit()
            self.hadVersion = True

I would of course be happy if Twisted's default behavior would not require me to add hacks like these... but I suppose I really can't argue if it's working as intended.

[desaster]

A little typo here, as I was just writing the fix for this message:

if twisted.version.major >= 11 and \

Should of course be:

if twisted.version.major <= 11 and \

[exarkun]

#6102 was a duplicate of this.