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_MSG_USERAUTH_PK_OK handling differs from OpenSSH #64785

Closed
stephenm-stripe opened this issue Dec 18, 2023 · 10 comments
Closed
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@stephenm-stripe
Copy link

Go version

go version go1.20.5 darwin/arm64

What operating system and processor architecture are you using (go env)?

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/stephenm/Library/Caches/go-build"
GOENV="/Users/stephenm/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/stephenm/go/pkg/mod"
GONOPROXY="*.corp.stripe.com,github.com/stripe-internal/*,github.com/stripe/*"
GONOSUMDB="*.corp.stripe.com,github.com/stripe-internal/*,github.com/stripe/*"
GOOS="darwin"
GOPATH="/Users/stephenm/go"
GOPRIVATE="*.corp.stripe.com,github.com/stripe-internal/*,github.com/stripe/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.5/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.5"
GCCGO="gccgo"
AR="ar"
CC="cc"
CXX="c++"
CGO_ENABLED="1"
GOMOD="/Users/stephenm/stripe/crypto/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 -fdebug-prefix-map=/var/folders/nd/8x6xvrgs3fb1ngn08_rx59t40000gn/T/go-build4288886961=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I was encountering the same error in #54027 for connecting to a remote server outside of my control. In this scenario, the remote server does not follow the RFC in creating SSH_MSG_USERAUTH_PK_OK packets.

The RFC specifies that SSH_MSG_USERAUTH_REQUEST packets include string public key algorithm name and the server would respond with a SSH_MSG_USERAUTH_PK_OK that includes string public key algorithm name from the request.

In this case, I would see our SSH_MSG_USERAUTH_REQUEST include an algorithm like rsa-sha2-256, but the SSH_MSG_USERAUTH_PK_OK response would always contain ssh-rsa for all RSA algorithms provided.

What did you expect to see?

Go SSH handshakes have the same outcome as OpenSSH handshakes for the same parameters.

What did you see instead?

OpenSSH handshakes succeed with the remote SSH server while Go SSH handshakes do not.

The Go handshake fails because the handling here checks that the outbound algorithm string matches the one in the response. In our case, the remote server is not following the RFC and sending back the type rather than the algorithm string that was sent to it. So Go's implementation considers this a failed auth and returns an error to the application.

Yet, OpenSSH via ssh on the command line succeeds for this server. The OpenSSH implementation for handling SSH_MSG_USERAUTH_PK_OK doesn't check the exact string matches, rather, it converts the algorithm names into the key types and compares on that (code for the check, code for the struct).

I admit, OpenSSH's handling here differs from the RFC, but it does result in a successful connection while Go's implementation does not. I also tried commenting out Go's logic around key validation that invokes these packets and the connection succeeded, so the broken invariant doesn't actually impact the connection overall.

I will put up a pull request to modify Go's implementation of SSH_MSG_USERAUTH_PK_OK checks to match OpenSSH's.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550716 mentions this issue: x/crypto/ssh: update SSH_MSG_USERAUTH_PK_OK handling to match OpenSSH

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 19, 2023
@thanm
Copy link
Contributor

thanm commented Dec 19, 2023

Thanks for the report. You mention that you are using Go 1.20 -- I assume the same problem exists in 1.21 and on tip?

@drakkan @golang/security per owners.

@drakkan
Copy link
Member

drakkan commented Dec 20, 2023

Hello @stephenm-stripe, thanks for the detailed info. So this is yet another egde case for RSA keys.

Can you please confirm that the following patch works with your server? (also for certificates)

0001-ssh-relax-algo-check-in-SSH_MSG_USERAUTH_PK_OK.zip

@stephenm-stripe
Copy link
Author

@thanm I haven't tested it on 1.21 or tip, but since this is in x/crypto, I don't think those will influence the outcome unless I misunderstand?

@drakkan Thanks for the patch. I have tested it against the non-compliant server and confirm it works.

@drakkan
Copy link
Member

drakkan commented Dec 26, 2023

@thanm I haven't tested it on 1.21 or tip, but since this is in x/crypto, I don't think those will influence the outcome unless I misunderstand?

@drakkan Thanks for the patch. I have tested it against the non-compliant server and confirm it works.

Thanks for confirming. Can you please modify your CL like my proof of concept? I think it is more readable to have separate conditions for each case

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/553035 mentions this issue: x/crypto/ssh: update SSH_MSG_USERAUTH_PK_OK handling to match OpenSSH

@stephenm-stripe
Copy link
Author

Thanks for confirming. Can you please modify your CL like my proof of concept? I think it is more readable to have separate conditions for each case

Yes. I meant to update the existing one, but have created https://go-review.googlesource.com/c/crypto/+/553035 instead.

@FiloSottile
Copy link
Contributor

I'd like to push back a little on this.

The rsa-sha2-256 / ssh-rsa transition is already extremely complex, and quirk compatibility edge cases add complexity that might invalidate safety assumptions at a distance. They are also a maintenance burned going forward.

We do add quirk workarounds when they are necessary for ecosystem reasons (i.e. if five OpenSSH versions in a row exhibited a broken behavior), but only after there was an attempt to fix the issue on the peer's side, and only if the broken peer is popular.

a remote server outside of my control

Can you share more about what implementation has this issue? Is there a fix? How common is it?

@Melinysh
Copy link

Replying from my personal account (as mentioned in this comment).

a remote server outside of my control

Can you share more about what implementation has this issue? Is there a fix? How common is it?

I don't have details on the implementation, other than it was a third-party enterprise software suite for Windows. There is no known fix. We did inform the server owners of this limitation of the vendor software they are using, but they seemed unlikely to pursue alternative solutions given that we were their only client experiencing SSH issues (I suspect we were the only ones using Go to connect). On our side, of the many servers we connected to for SFTP purposes, this was the only remote server that had this issue.

@gopherbot
Copy link
Contributor

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

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

6 participants