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: SSH Client fails handshake when using publickey method #66438

Closed
0x7f opened this issue Mar 21, 2024 · 9 comments
Closed

x/crypto/ssh: SSH Client fails handshake when using publickey method #66438

0x7f opened this issue Mar 21, 2024 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@0x7f
Copy link

0x7f commented Mar 21, 2024

Go version

go version go1.22.1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/max/Library/Caches/go-build'
GOENV='/Users/max/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/max/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/max/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.1/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.1'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/max/Code/sandbox/ssh-authentication-bug/client/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/1m/z4zys8c11n92mxs598p0hc1r0000gn/T/go-build3310712318=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I'm building a SSH server using the Node.js library https://github.com/mscdex/ssh2 and I'm building the SSH client using the x/crypto/ssh library. The client aborts the SSH handshake when using the publickey authentication method.

I built an example project including server and client here: https://github.com/0x7f/ssh-authentication-bug

What did you see happen?

Even though the server offers [publickey] authentication methods when initiating the handshake with authentication method none, the client fails to connect. The client uses the publickey method and offers the key, and the server accepts the key, but the client still prints the error:

2024/03/21 09:07:40 Failed to dial: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain

What did you expect to see?

When using OpenSSH client, it connects successfully. I would expect the same from the Golang x/crypto/ssh client.

Maybe related to #64785

@dr2chase
Copy link
Contributor

@golang/security

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2024
@0x7f
Copy link
Author

0x7f commented Mar 21, 2024

Quick not about findings from the issue over at ssh2 (the Node.js SSH2 server library). When switching from RSA to ED25519 keys, the handshake works as expected. So seems the issue is related to RSA keys somehow.

@gopherbot
Copy link

Change https://go.dev/cl/573360 mentions this issue: ssh: validate key type in SSH_MSG_USERAUTH_PK_OK response

@drakkan
Copy link
Member

drakkan commented Mar 23, 2024

@0x7f thank you for providing a reproducer. I think the linked CL fixes the reported issue. Can you please confirm?

According to RFC 4252 Section 7 the algorithm in SSH_MSG_USERAUTH_REQUEST should match that of the request, so our code was correct, but some server send the key type instead.

OpenSSH checks the key type (maybe to avoid problems with these buggy servers) so I think we should do the same

@omgitsotis
Copy link

I ran into this problem too. Through some gross debugging I found that it was failing here. My public key algo was ssh-rsa and the server algo was rsa-sha2-256, as they weren't the same, it returned false here.

I managed to get around it by changing the code (I forked the repo to unblock myself) to check if the algo matched any of the algos returned in algorithmsForKeyFormat(msg.Algo). Not sure if it is the right thing to do here though.

@drakkan
Copy link
Member

drakkan commented Mar 26, 2024

I ran into this problem too. Through some gross debugging I found that it was failing here. My public key algo was ssh-rsa and the server algo was rsa-sha2-256, as they weren't the same, it returned false here.

I managed to get around it by changing the code (I forked the repo to unblock myself) to check if the algo matched any of the algos returned in algorithmsForKeyFormat(msg.Algo). Not sure if it is the right thing to do here though.

Can you please confirm that this issue is fixed by applying this CL? Thank you

@0x7f
Copy link
Author

0x7f commented Mar 27, 2024

@drakkan the CL fixes it for me 👍 Thanks a lot.

I had trouble referencing the CL in my test project, so I manually applied the changes to the local code in $GOPATH.

@0x7f
Copy link
Author

0x7f commented Apr 28, 2024

@drakkan any updates on this one? I'm not familiar with Gerrit, but to me it looks like the CL was not merged yet, right?

@drakkan
Copy link
Member

drakkan commented May 2, 2024

@drakkan any updates on this one? I'm not familiar with Gerrit, but to me it looks like the CL was not merged yet, right?

It is in the review queue. It should be merged soon, I think another Google employee should approve it

drakkan added a commit to drakkan/crypto that referenced this issue May 11, 2024
According to RFC 4252 Section 7 the algorithm in SSH_MSG_USERAUTH_PK_OK
should match that of the request but some servers send the key type instead.
OpenSSH checks for the key type, so we do the same.

Fixes golang/go#66438
Fixes golang/go#64785
Fixes golang/go#56342
Fixes golang/go#54027

Change-Id: I2f733f0faece097e44ba7a97c868d30a53e21d79
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/573360
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Joedian Reid <joedian@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants