-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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: regression: no longer able to authenticate using rsa-sha2-512 key #56342
Comments
Probably related to #53391 |
@golang/security can you have a look at this? It smells like a dupe but IANAE. |
FWIW this is only somewhat related - and not dupe of - #53391 as this is the client side (cc @FiloSottile ). |
For my use case, if I patch pickSignatureAlgorithm in client_auth.go to ignore I can't speak to the security of doing this though, but it does unblock me until the code is fixed. |
Indeed, this seems to be a mismatch between key algorithm sent by the client and acknowledged by the server during authentication phase (pubkey). Checking the RFC 4252 §7 on that point, the public key authentication request sent by the client is:
In our case the The server reply as per RFC 4252 §7:
In our case, the However, there is no mention in the RFC that the client MUST, MAY or SHOULD verify these fields from the As a consequence, in the func confirmKeyAck(key PublicKey, algo string, c packetConn) (bool, error) {
pubKey := key.Marshal()
for {
packet, err := c.readPacket()
if err != nil {
return false, err
}
switch packet[0] {
case msgUserAuthBanner:
if err := handleBannerResponse(c, packet); err != nil {
return false, err
}
case msgUserAuthPubKeyOk:
var msg userAuthPubKeyOkMsg
if err := Unmarshal(packet, &msg); err != nil {
return false, err
}
// if msg.Algo != algo || !bytes.Equal(msg.PubKey, pubKey) {
if !bytes.Equal(msg.PubKey, pubKey) {
return false, nil
}
return true, nil
case msgUserAuthFailure:
return false, nil
default:
return false, unexpectedMessageError(msgUserAuthPubKeyOk, packet[0])
}
}
} Removing the algorithm match check improves client interoperability with servers that supports As far as I understand, the algorithm match check here does not improve security and removing it does not degrade it either. The client will then proceed with a Did I miss something ? |
Addresses same issue as golang/go#56342
Hello, I don't have GitHub Enterprise, I used my personal GitHub account to test. I cannot reproduce the issue. msg.Algo and algo match in my tests:
I also tried to set a different algorithm using the patch here like this
works with all supported algorithms and msg.Algo and algo always match. Is this problem still current? Or does it only happen with GitHub Enterprise accounts? |
Addresses same issue as golang/go#56342
Addresses same issue as golang/go#56342
Addresses same issue as golang/go#56342
Addresses same issue as golang/go#56342
Addresses same issue as golang/go#56342
I would say the problem is still current. We keep seeing this issue with 2 servers that some of our partners are using. We first noticed it to start failing at v0.15.0 of golang.org/x/crypto. I can try to gather more details about this. At least one of the servers in question is using HiDrive from Strato. |
Hello, I'd like to have access to a server with this behavior to test with OpenSSH and better understand what OpenSSH does so we can do something similar in If this is not possible it would be great to have debug logs from the OpenSSH CLI connecting to this server, probably we need to add some additional logs here and in other relevant code paths. Thank you |
Unfortunately I haven't figured out what the exact setup is and I cannot really provide the specific credentials for the server. In the meantime, some logs and code: openssh connection Log
I prepared a simplified code version for go to show that we are not doing anything really beyond the basics for the connection: main.gopackage main
import (
"fmt"
"log"
"net"
"os"
"time"
"github.com/joho/godotenv"
"golang.org/x/crypto/ssh"
)
func main() {
err := godotenv.Load(".env")
if err != nil {
log.Fatal(err)
}
authMethods := make([]ssh.AuthMethod, 0)
signer, err := ssh.ParsePrivateKey([]byte(os.Getenv("TEST_KEY")))
if err != nil {
log.Fatal(err)
}
authMethods = append(authMethods, ssh.PublicKeys(signer))
sshConfig := &ssh.ClientConfig{
User: os.Getenv("TEST_USERNAME"),
Auth: authMethods,
Timeout: 5 * time.Second,
HostKeyCallback: func(hostname string, remote net.Addr, key ssh.PublicKey) error {
return nil
},
}
sshConfig.SetDefaults()
hostAndPort := os.Getenv("TEST_HOST_AND_PORT")
sshClient, err := ssh.Dial(
"tcp",
hostAndPort,
sshConfig,
)
if err != nil {
log.Fatal(err)
}
defer sshClient.Close()
fmt.Println("Connection success")
} go.modmodule main
go 1.22.0
require (
github.com/joho/godotenv v1.5.1
golang.org/x/crypto v0.21.0
)
require golang.org/x/sys v0.18.0 // indirect Now, with my patch it works go.mod with replacemodule main
go 1.22.0
require (
github.com/joho/godotenv v1.5.1
golang.org/x/crypto v0.21.1
)
require golang.org/x/sys v0.18.0 // indirect
replace golang.org/x/crypto v0.21.1 => github.com/tobischo/gocrypto v0.21.1 and when I add log statements where to golang.org/x/ssh where I added my patch, then I would see the following:
|
@tobischo thanks for the logs. From your logs I see
so I downloaded and compiled from source OpenSSH-7.5p1 but I cannot reproduce the issue. I have The banner is slightly different, if I connect with ssh cli I have this
|
When I run
and I see that part whether I use a valid username / key combination or not 🤔 obviously I am not getting to the relevant bit with invalid credentials |
Change https://go.dev/cl/573360 mentions this issue: |
@tobischo can you please confirm that the above CL fixes the reported issue? Thank you |
@drakkan I can confirm that the change you prepared is working with the 2 servers where I observed the issues |
Addresses same issue as golang/go#56342
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>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Authenticating to SSH server using
rsa-sha2-512
used to work fine with in my Go program when I was usinggolang.org/x/crypto v0.0.0-20220128200615-198e4374d7ed
. However, after upgrading to the latest (v0.1.0) crypto, the same program is unable to authenticate successfully anymore. I narrowed down the regression to a particular commit5d542ad81a58c89581d596f49d0ba5d435481bcf
. Key was generated usingssh-keygen -t rsa-sha2-512
. (openssh version 8.9p1-3 on Ubuntu jammy). SSH server used was Github enterprise edition (SSH-2.0-babeld-5f3a6bb
).I have provided a test program to reproduce the issue below.
What did you expect to see?
Successful connection to the SSH server using the provided
rsa-sha2-512
key.What did you see instead?
Following error returned from the crypto library.
Go Program to Reproduce/Bisect
Placed the following program in
cmd/sshauthtest/main.go
under the local crypto git repo checked out based on v0.1.0 tag.Note: I have redacted the username, hostname and key file path. Needs to be populated by the developer when reproducing the issue.
Git Bisection Result
Additional Debug Information
CC: @FiloSottile
I tried to debug further and narrowed down the difference in behavior to
x/crypto/ssh.confirmKeyAck
function, which thinks that there is a mismatch between the algorithm used by the client and responded by the server. Note the additionalfmt.Printf
that I added undercase msgUserAuthPubKeyOk
.In the failing case, it produces the following output.
What is even more interesting is that the
algo
was reported asrsa-sha2-256
even though it was generated asrsa-sha2-512
!In the succeeding case, both are of value
ssh-rsa
.ssh-ed25519
works fine with both the new and old versions of crypto library. As a workaround for now, we are recommending our users to stick withed25519
keys because of this issue.The text was updated successfully, but these errors were encountered: