Here's the commit that introduced this behavior:

778035a

This change causes it to include any lines before the SSH version indicator.

Sounds consistent with what the RFC says.

This apparently causes problems, possibly causing it to read to the end of the buffer early.

Can you elaborate on the problems? What are you using as the SSH server? What is the server identification string?

That said, it's also not clear how your code change would prevent an exception being thrown if one is already being thrown. Like your commit doesn't change how much data is being read from the buffer / socket connection / whatever - it just changes what $this->server_identifier is being set to. Like as is your fix seems akin to saying that $a = file_get_contents('100gb file') is causing PHP to run out of memory BUT adding $a = ''; after makes the issue go away. I mean, sure, in this scenario, maybe dead-code elimination could result in that behavior idk but barring that it's unclear how changing what a variable is set to would prevent issues, should issues exist.

Thanks for taking a look. This is to a remote server that I do not control. After connecting, two lines are read from the stream:

Evox Images Portal FTP Server
SSH-2.0-JSCAPE9

The problem here is that having the other line before the version string is no longer working as the RFC states it should, because that extra line is now included in the server_identifier. I admit, the connection between the change and the exception is not well explained as I honestly do not fully understand it. But this simple revert does resolve my issue completely, so I do think this is worth some investigation at least. It appears the references in _key_exchange() are not prepared to handle a server_identifier with extra lines prepended to it. I'm not sure exactly what problems that would cause, but that function does call get_binary_packet() four times. If a broken algorithm ran the stream to the end, that could cause the exception.

The problem here is that having the other line before the version string is no longer working as the RFC states it should, because that extra line is now included in the server_identifier.

I don't see how that extra line being included in the server_identifier is a violation of the RFC.

Anyway, would you be able to give me access to the SSH server? I wouldn't need a login - just the IP address or domain name. If I can reproduce the problem I can independently verify it and decide on what I feel like is a more appropriate fix.

Setting VersionAddendum in /etc/ssh/sshd_config appends stuff - it doesn't prepend stuff. So that, as a testing option, won't work for me.

I could custom compile OpenSSH with a change that'd enable to current behavior but I wouldn't want to mess up my current OpenSSH installation so I'd need to try to get OpenSSH running in a disposable Docker container.

I could also write a PHP server right up to the point of key exchange but doing anything past that is quite a bit of work and I'm not looking to build a from scratch SSH server.

SSH / SFTP access to a server that reproduces the issue just makes it a lot easier for me to test it out. If you can't provide that I'll look into one of the above methods.

Thanks!

Certainly, I just emailed you the server address. In regards to the RFC, I only meant to say that originally we had been telling the owners of the server that they needed to reconfigure their server. After looking at the RFC, it seems the code should support this scenario (and indeed it did support it just fine until the recent change), so this prompted my suggestion here instead.

I think I figured it out.

So in the part of RFC4253 that you and I quoted there's this bit

The server MAY send other lines of data before sending the version string.

The implication of that is that the other lines that come before the version string are not part of the version string.

As for why this is an issue... later in the same RFC there's this:

   The hash H is computed as the HASH hash of the concatenation of the
   following:

      string    V_C, the client's identification string (CR and LF
                excluded)
      string    V_S, the server's identification string (CR and LF
                excluded)
      string    I_C, the payload of the client's SSH_MSG_KEXINIT
      string    I_S, the payload of the server's SSH_MSG_KEXINIT
      string    K_S, the host key
      mpint     e, exchange value sent by the client
      mpint     f, exchange value sent by the server
      mpint     K, the shared secret

So the server identification string needs to be included in the hash. With your code change SSH-2.0-JSCAPE9 is being fed into the hash whereas with mine it's Evox Images Portal FTP Server and SSH-2.0-JSCAPE9.

So per that I guess getServerIdentification() prob should return just SSH-2.0-JSCAPE9, however, I think getLog() should return both.

I'll do a little more testing. Like with your changes what if you have a server identification string that's 300 bytes long? ie. SSH-2.0-zzzzzzzz where z is repeated 300x or whatever?

I gotta head out very shortly but I'll take a look at this further prob tomorrow.

Thank you for your patience!

9d8e42d should fix this.

Thanks!

Still need to backport the fix to the 2.0 and possibly the 1.0 branches but I just wanted to get the 3.0 fix out before that.

Thanks for taking care of this so quickly!