Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.Sign up
GitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
x/crypto/ssh: ssh: handshake failed: ssh: unexpected message type 21 (expected 6) #15066
Please answer these questions before submitting your issue. Thanks!
A passing test.
We see a failure for an unexpected message type. After digging into the issue, it appears that the
We went about solving this by making the following commit
Could you provide some feedback on the proposed changes and help us progress with getting the above fix into the additional crypto libraries?
Just keep running the tests. This generally fails faster in resource constrained environments such such as a container or a VM. On my personal laptop it can take anywhere from 50-10000 runs in order to see this failure occur. Also you can see this issue reported in public builds of golang, for example here.
RFC 4253 Section 9 states that either the client or server may re-initiate a key exchange at any point, and the other side must be able to handle this key exchange. This would imply that a key exchange during authentication is theoretically possible, and should not result in a
I believe that this is failing intermittently because of timing changes introduced in go 1.6. If you look at the test setup for the above test here, you will see that both the
Granted this could fail on any calls to
I agree that the library doesn't perform up to spec, since a kex may be requested at any time and should not cause problems during auth.
However, in the unittests, the client and server are both golang.crypto, which should issue only one key exchange, the initial one right before the auth step. Client and server synchronize on both sides, see here
I would guess that there is still some sort of race condition lingering that somehow causes msgNewKeys to be read once, but emitted twice from the packetTransport.
If you have a platform where this problem reproduces easily, it would be great if you could try to run with the race detector to see if there are any race conditions I missed.
@hanwen msgNewKeys is not a message that is actually ever read over the wire. It is a hard-coded message in the client/server here:
This has nothing to do with msgNewKeys being emitted twice incorrectly. It has to do with the internal handling of all Kex Packets in
The two lines you linked above do not show that the client/server synchronize on requesting their key exchanges at the same time. All they show is that the respective client/server requested a key exchange and received the correct response. There is nothing stopping them from both requesting a key exchange.
Understanding the race, you could artificially introduce timing into the production code to get this to fail easier.
or a similar sleep in
Once again the issue here is that
Thus the server can receive the clients KexInit, respond with a KexInit successfully and then send another KexInit because of disconnect between reading and buffering packets which begins here:
and forcibly requesting a key exchange here:
Also please note that this timing issue exists in both the client and the server, just in the example above I artificially introduced the timing in the server. I could have just have easily added the
Please let me know if you have any other questions.
For example, here is some output from
With this diff:
OK, I get it now.
The problem is that on setting up the connection both sides should send a kexInit to do the first (mandatory) key exchange, and they should both only proceed if they are sure the kex succeeded.
The current code does this by doing requestKeyExchange in both the server and the client.
However, if one of both sides is slow, the kex initiated by the fast side may have already run and completed. Then the requestKeyExchange effectively executes a 2nd kex, the confirmation of which (msgNewKeys) causes havoc.
I think the solution would be to have a separate requestFirstKeyChange(), which does nothing if sessionID != nil.
That seems reasonable. I believe the
While highly unlikely, the problem around receiving a KexInit while reading other packets is still a possibility. Due to the manner in which key exchanges are handled internally, I still think it would make sense to explore ideas around ignoring these packets in all cases in which we are not performing a key exchange.
Thanks for taking the time to look at this.
This is done by running the key exchange and setting the session ID under mutex. If the first exchange encounters an already set session ID, then do nothing. This fixes a race condition: On setting up the connection, both sides sent a kexInit to initiate the first (mandatory) key exchange. If one side was faster, the faster side might have completed the key exchange, before the slow side had a chance to send a kexInit. The slow side would send a kexInit which would trigger a second key exchange. The resulting confirmation message (msgNewKeys) would confuse the authentication loop. This fix removes sessionID from the transport struct. This fix also deletes the unused interface rekeyingTransport. Fixes golang#15066 Change-Id: I7f303bce5d3214c9bdd58f52d21178a185871d90 Reviewed-on: https://go-review.googlesource.com/21606 Reviewed-by: Adam Langley <firstname.lastname@example.org> Reviewed-by: Han-Wen Nienhuys <email@example.com>