Skip to content
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: guessed kex handled improperly #16962

Closed
belak opened this issue Sep 2, 2016 · 3 comments
Closed

x/crypto/ssh: guessed kex handled improperly #16962

belak opened this issue Sep 2, 2016 · 3 comments
Milestone

Comments

@belak
Copy link

@belak belak commented Sep 2, 2016

Go env information

% go version
go version go1.7 linux/amd64
% go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/belak/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build164292563=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

Attempt to connect to a golang ssh server using dropbear

What did you expect to see?

A successful handshake

What did you see instead?

Dropbear fails to connect, claiming the server closed the connection.

I initially tested disabling the guess in dropbear entirely, and that avoids this bug. Enabling debugHandshake, I saw server exited key exchange (first true), err ssh: unexpected message type 30 (expected 21)

https://tools.ietf.org/html/rfc4250#section-4.1.2 states that 21 is SSH_MSG_NEWKEYS (essentially the end of the kex) and https://tools.ietf.org/html/rfc5656#section-7.1 states that 30 is SSH_MSG_KEX_ECDH_INIT. So, this means the server was getting an extra 30 before getting 21.

Potential fix

Under https://tools.ietf.org/html/rfc4253#section-7, it states that a guess can be counted as wrong if "the kex algorithm and/or the host key algorithm is guessed wrong (server and client have different preferred algorithm)" or all the algorithms cannot be agreed upon. Currently the check in x/crypto/ssh only checks algs.kex != otherInit.KexAlgos[0] and not the host key algorithm. The following patch is a quick and dirty way of solving this which also checks the host key algorithm. I'm not sure what the proper solution would be, but if requested, I can try to submit this.

This is what was causing the problem because the server and the client didn't agree that the first kex packet should be ignored, so the client sent another one (because it realized that the host key algorithms didn't match) and the server thought it was fine (because the kex algorithm matched). After the first kex packet is handled by the server, it finds another one, and not SSH_MSG_NEWKEYS as expected.

diff --git a/ssh/handshake.go b/ssh/handshake.go
index ae26191..04ad3e7 100644
--- a/ssh/handshake.go
+++ b/ssh/handshake.go
@@ -371,7 +371,7 @@ func (t *handshakeTransport) enterKeyExchangeLocked(otherInitPacket []byte) erro
        }

        // We don't send FirstKexFollows, but we handle receiving it.
-       if otherInit.FirstKexFollows && algs.kex != otherInit.KexAlgos[0] {
+       if otherInit.FirstKexFollows && (algs.kex != otherInit.KexAlgos[0] || algs.hostKey != otherInit.ServerHostKeyAlgos[0]) {
                // other side sent a kex message for the wrong algorithm,
                // which we have to ignore.
                if _, err := t.conn.readPacket(); err != nil {
@mkj
Copy link

@mkj mkj commented Sep 2, 2016

That fix looks good, though I think the original code was also wrong in comparing the "agreed" algorithm. It should compare the first ("preferred") myInit algorithm? Something like this:

diff --git a/ssh/handshake.go b/ssh/handshake.go
index ae26191..3fb58f0 100644
--- a/ssh/handshake.go
+++ b/ssh/handshake.go
@@ -371,7 +371,9 @@ func (t *handshakeTransport) enterKeyExchangeLocked(otherInitPacket []byte) erro
        }

        // We don't send FirstKexFollows, but we handle receiving it.
-       if otherInit.FirstKexFollows && algs.kex != otherInit.KexAlgos[0] {
+       if otherInit.FirstKexFollows &&
+               (myInit.KexAlgos[0] != otherInit.KexAlgos[0] ||
+                       myInit.ServerHostKeyAlgos[0] != otherInit.ServerHostKeyAlgos[0]) {
                // other side sent a kex message for the wrong algorithm,
                // which we have to ignore.
                if _, err := t.conn.readPacket(); err != nil {

(I'm the Dropbear developer)

@belak
Copy link
Author

@belak belak commented Sep 2, 2016

@quentinmit quentinmit modified the milestone: Unreleased Sep 6, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 8, 2016

CL https://golang.org/cl/28750 mentions this issue.

tg123 pushed a commit to tg123/sshpiper that referenced this issue Sep 28, 2016
In the initial key exchange, a client has the option of sending a
guessed key exchange packet. This guess is considered wrong (RFC 4253
Section 7) if "the kex algorithm and/or the host key algorithm is
guessed wrong (server and client have different preferred algorithm),
or if any of the other algorithms cannot be agreed upon...".

The library should be checking the first algorithm in the supported
algorithms, not the agreed algorithm. It also needs to check both the
kex algorithm and the host key algorithm.

Fixes golang/go#16962

Change-Id: I6b62b1f5b39731326280571d373635085135a2a1
Reviewed-on: https://go-review.googlesource.com/28750
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Sep 20, 2017
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
In the initial key exchange, a client has the option of sending a
guessed key exchange packet. This guess is considered wrong (RFC 4253
Section 7) if "the kex algorithm and/or the host key algorithm is
guessed wrong (server and client have different preferred algorithm),
or if any of the other algorithms cannot be agreed upon...".

The library should be checking the first algorithm in the supported
algorithms, not the agreed algorithm. It also needs to check both the
kex algorithm and the host key algorithm.

Fixes golang/go#16962

Change-Id: I6b62b1f5b39731326280571d373635085135a2a1
Reviewed-on: https://go-review.googlesource.com/28750
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
In the initial key exchange, a client has the option of sending a
guessed key exchange packet. This guess is considered wrong (RFC 4253
Section 7) if "the kex algorithm and/or the host key algorithm is
guessed wrong (server and client have different preferred algorithm),
or if any of the other algorithms cannot be agreed upon...".

The library should be checking the first algorithm in the supported
algorithms, not the agreed algorithm. It also needs to check both the
kex algorithm and the host key algorithm.

Fixes golang/go#16962

Change-Id: I6b62b1f5b39731326280571d373635085135a2a1
Reviewed-on: https://go-review.googlesource.com/28750
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.