New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/crypto/ssh: ssh: handshake failed: ssh: unexpected message type 21 (expected 6) #15066
Comments
/cc @hanwen |
thanks for the careful analysis. I wonder you triggered a kex during auth, as it usually only happens after lots of data transfer. I'll have a look at handling msgNewKeys in a more principled way. |
wait, this is in our unittests?! This should never happen, as both sides wait for the first msgNewKeys explicitly and then don't exchange enough data for a new kex to be trggered. |
can you tell us a little more about the circumstances under which this happens, or how to reproduce? |
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 Best, Jim |
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 https://github.com/golang/crypto/blob/master/ssh/client.go#L106 and here https://github.com/golang/crypto/blob/master/ssh/server.go#L197 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: https://github.com/golang/crypto/blob/master/ssh/handshake.go#L211 This has nothing to do with msgNewKeys being emitted twice incorrectly. It has to do with the internal handling of all Kex Packets in https://github.com/golang/crypto/blob/master/ssh/handshake.go#L173-L211 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. Adding this
or a similar sleep in Once again the issue here is that https://github.com/golang/crypto/blob/master/ssh/handshake.go#L308 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: https://github.com/golang/crypto/blob/master/ssh/server.go#L189 and forcibly requesting a key exchange here: https://github.com/golang/crypto/blob/master/ssh/server.go#L191 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:
|
I appreciate that you are trying to explain me how this thing works, but I actually wrote all of that code, and know more or less how it is supposed to work. thanks for the last diff - I'll have a closer look. |
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. |
Yes, msgNewKey should probably be filtered from the rest of the code. However, that would mask the current problem, which is that we might do 2 kexes if one side is slow; I prefer to fix the latter problem first. |
It looks like golang/crypto@d68c3ec has been merged, which fixes this issue, so it can be closed now. |
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 <agl@golang.org> Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Please answer these questions before submitting your issue. Thanks!
go version
)?go env
)?Run
go test
in crypto/ssh. You will eventually see this failure on random tests performing server/client handshakes.A passing test.
We see a failure for an unexpected message type. After digging into the issue, it appears that the
handshakeTransport.readPacket
method is used to perform the key exchange, but also in order to read packets in the client/server authentication flow. When a key exchange is initiated during authentication, we can experience failures as the packet returned by the above method is actually themsgNewKeys
message which is the result of successfully performing the key exchange.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?
The text was updated successfully, but these errors were encountered: