[crypto] ssh: relax RSA signature check in SSH_MSG_USERAUTH_REQUEST
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
ssh: relax RSA signature check in SSH_MSG_USERAUTH_REQUEST
Buggy SSH clients, such as gpg-agent v2.2.4 and OpenSSH v7.6 shipped
in Ubuntu 18.04, may send `ssh-rsa-512` as the public key algorithm
but actually include an `rsa-sha` signature.
If RFC 3808 (extension negotiation) is implemented, these clients will
fail to authenticate with the error:
```
ssh: signature "ssh-rsa" came in for selected algorithm "rsa-sha2-512", public key is type ssh-rsa
```
According to RFC 8332 section 3.2:
If the client includes the signature field, the client MUST encode the
same algorithm name in the signature as in SSH_MSG_USERAUTH_REQUEST --
either "rsa-sha2-256" or "rsa-sha2-512". If a server receives a
mismatching request, it MAY apply arbitrary authentication penalties,
including but not limited to authentication failure or disconnect.
...A server MAY, but is not required to, accept this variant or another
variant that corresponds to a good-faith implementation and is
considered safe to accept.
While the client is expected to do the right thing, in practice older
clients may not fully support `ssh-rsa-256` and `ssh-rsa-512`. For
example, gpg-agent v2.2.6 added support for these newer signature
types.
To accomodate these clients, relax the matching constraint: if the
`SSH_MSG_USERAUTH_REQUEST` message specifies an RSA public key
algorithm and includes an RSA public key, then allow any of the
following signature types:
- `rsa-sha-512`
- `rsa-sha-256`
- `rsa-sha`
This emulates what OpenSSH does. OpenSSH only considers that the RSA
family is specified and then verifies if the signature and public key
match.
Closes https://github.com/golang/go/issues/53391
Change-Id: I9208fba08ab792c973cc6f744a9b68a812a583e0
GitHub-Last-Rev: 443b8cb475cc16514b864a61d02fef52fbb2ae2d
GitHub-Pull-Request: golang/crypto#221
---
M ssh/server.go
1 file changed, 77 insertions(+), 1 deletion(-)
diff --git a/ssh/server.go b/ssh/server.go
index 70045bd..8a3f14e 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -396,6 +396,28 @@
// It is returned in ServerAuthError.Errors from NewServerConn.
var ErrNoAuth = errors.New("ssh: no auth passed yet")
+func isRSAType(algo string) bool {
+ return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSA
+}
+
+func algoCompatible(algo string, publicKey PublicKey, sig *Signature) bool {
+ if underlyingAlgo(algo) == sig.Format {
+ return true
+ }
+
+ // Buggy SSH clients may send ssh-rsa2-512 as the public key algorithm but
+ // actually include a rsa-sha signature.
+ // According to RFC 8832 Section 3.2:
+ // A server MAY, but is not required to, accept this variant or another variant that
+ // corresponds to a good-faith implementation and is considered safe to
+ // accept.
+ if publicKey.Type() == KeyAlgoRSA && isRSAType(algo) && isRSAType(sig.Format) {
+ return true
+ }
+
+ return false
+}
+
func (s *connection) serverAuthenticate(config *ServerConfig) (*Permissions, error) {
sessionID := s.transport.getSessionID()
var cache pubKeyCache
@@ -564,7 +586,7 @@
authErr = fmt.Errorf("ssh: algorithm %q not accepted", sig.Format)
break
}
- if underlyingAlgo(algo) != sig.Format {
+ if !algoCompatible(algo, pubKey, sig) {
authErr = fmt.Errorf("ssh: signature %q not compatible with selected algorithm %q", sig.Format, algo)
break
}
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Maxime Tremblay (Gerrit)
Attention is currently required from: Filippo Valsorda.
Patch set 1:Code-Review +1
Gerrit Bot (Gerrit)
Attention is currently required from: Filippo Valsorda, Maxime Tremblay.
Gerrit Bot uploaded patch set #2 to this change.
GitHub-Last-Rev: 08edff8304716fba1b6a681d937d2ef5f51283c3
GitHub-Pull-Request: golang/crypto#221
---
M ssh/server.go
1 file changed, 79 insertions(+), 1 deletion(-)
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Filippo Valsorda, Maxime Tremblay.
Gerrit Bot uploaded patch set #3 to this change.
GitHub-Last-Rev: 630f71961527acf21f42f6ef29ae4a2444d605c2
GitHub-Pull-Request: golang/crypto#221
---
M ssh/client_auth_test.go
M ssh/server.go
2 files changed, 159 insertions(+), 1 deletion(-)
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Maxime Tremblay (Gerrit)
Attention is currently required from: Filippo Valsorda.
Patch set 3:Code-Review +1
1 comment:
Patchset:
ssh: signature "ssh-rsa" came in for selected algorithm "rsa-sha2-512", public key is type ssh-rsa
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Jakub Nyckowski (Gerrit)
Attention is currently required from: Filippo Valsorda, Maxime Tremblay.
1 comment:
File ssh/server.go:
Patch Set #2, Line 408:
return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSAThis only covers public key authentication, but it doesn't solve the same issue when using an ssh certificate. Could we "extend" that check to also include ssh certificates?
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski.
Patch set 3:Run-TryBot +1
Roland Shoemaker (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski.
Patch set 4:Run-TryBot +1
Jakub Nyckowski (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Stan Hu.
1 comment:
File ssh/server.go:
Patch Set #2, Line 408:
return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSA
I think this should be resolved with the latest changes in https://github. […]
I tested the latest changes, and I'm still getting an error when trying to connect to OpenSSH 7.4:
ssh: signature "rsa-sha2-512" not compatible with selected algorithm "ssh-rsa-...@openssh.com", unsupported key type:
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Jakub Nyckowski (Gerrit)
File ssh/server.go:
Patch Set #2, Line 408:
return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSA
To be clear, this is with SSH certificates? Perhaps this should be a separate change. […]
I created a repository with a reproduction https://github.com/jakule/go-crypto-ssh-repro. I used two clients, OpenSSH 8.9 and 7.4. The older OpenSSH fails with the error posted above. I understand this could be a separate charge, but merging anything upstream takes years, and we're also running a fork of go/crypto because of this.
Let me know if you have more questions.
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Jakub Nyckowski (Gerrit)
File ssh/server.go:
Patch Set #2, Line 408:
return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSA
Thanks. I'll take a look. […]
This is a link to the path we're currently using https://github.com/gravitational/crypto/pull/2
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Nicola Murino (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski.
1 comment:
File ssh/server.go:
Patch Set #4, Line 410:
compatibleAlgos := algorithmsForKeyFormat(pubKeyFormat)```suggestion
compatibleAlgos := algorithmsForKeyFormat(underlyingAlgo(pubKeyFormat))
```this change should fix the reported issue with certificates.
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Jakub Nyckowski (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Nicola Murino, Stan Hu.
2 comments:
File ssh/server.go:
Patch Set #2, Line 408:
return algo == KeyAlgoRSASHA512 || algo == KeyAlgoRSASHA256 || algo == KeyAlgoRSA
This current changeset works fine for me with your reproduction test. […]
Switching to the mentioned fork "solves" the issue not because of the patch from this PR, but because the fork is based on crypto v0.2.0. OpenSSH 7.4 stopped working after https://github.com/golang/crypto/commit/6fad3dfc18918c2ac9c112e46b32473bd2e5e2f9 was merged (v0.3.0 release). Patch propsed by @nicola...@gmail.com below seems to work.
File ssh/server.go:
Patch Set #4, Line 410:
compatibleAlgos := algorithmsForKeyFormat(pubKeyFormat)
```suggestion […]
I confirm, this patch solves the cert issue.
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Nicola Murino, Roland Shoemaker, Stan Hu.
Gerrit Bot uploaded patch set #5 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Roland Shoemaker, TryBot-Result+1 by Gopher Robot
ssh: relax RSA signature check in SSH_MSG_USERAUTH_REQUEST
Buggy SSH clients, such as gpg-agent v2.2.4 and OpenSSH v7.6 shipped
in Ubuntu 18.04, may send `ssh-rsa-512` as the public key algorithm
but actually include an `rsa-sha` signature.
If RFC 3808 (extension negotiation) is implemented, these clients will
fail to authenticate with the error:
```
ssh: signature "ssh-rsa" came in for selected algorithm "rsa-sha2-512", public key is type ssh-rsa
GitHub-Last-Rev: 9c9ba6a5d725b5247ce43e5c1eafac8a10136195
GitHub-Pull-Request: golang/crypto#221
---
M ssh/client_auth_test.go
M ssh/server.go
2 files changed, 111 insertions(+), 1 deletion(-)
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Nicola Murino (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Roland Shoemaker, Stan Hu.
1 comment:
File ssh/server.go:
Patch Set #4, Line 410:
compatibleAlgos := algorithmsForKeyFormat(pubKeyFormat)
Done
Thank you Stan Hu. The compatibility with old clients should be now fixed for both keys and certificates.
I wonder if a more stringent validity check should be added. We can accept the "ssh-rsa" algorithm with the public key format "ssh-rsa-...@openssh.com" or viceversa. I think this worked even before this change. OpenSSH seems to have a stricter validation. I have errors like this "error: userauth_pubkey: type mismatch for decoded key (received 0, expected 4) [preauth]"
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Nicola Murino (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Roland Shoemaker, Stan Hu.
1 comment:
File ssh/server.go:
```suggestion
}
// Ensure the declared public key algo is compatible
// with the decoded one
if !contains(algorithmsForKeyFormat(pubKey.Type()), algo) {
authErr = fmt.Errorf("ssh: type mismatch for decoded key, received %q, expected %q", pubKey.Type(), algo)
break
}
```this way we don't accept incompatible algo. For example algo ssh-rsa-...@openssh.com with public key type ssh-rsa will be rejected.
Not sure if this change should be included here or discussed in a separate patch
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Ash McKenzie, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Nicola Murino, Roland Shoemaker.
1 comment:
Patchset:
👋 Is there anything I can do to help move this change along? I'm happy to test?
I think it needs at least on "trusted" security-oriented person to eyeball it and +2 it. I put "trusted" in square quotes because that ultimately depends on someone else's judgement (not mine, I know who's on the Go security team and a subset of who they trust).
Also for actually submitting, all the comments have to be marked "resolved" (as I will this one).
Re testing, maybe? Confirming that it works in an end-to-end test is not bad, but if that's a lot of work for you, probably not necessary either. I get the impression this CL is well-tested in the fork.
I don't know if this will make it into 1.21, but if it is approved, we should at least do it early in 1.22 so that we're done with it.
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Nicola Murino (Gerrit)
Attention is currently required from: Ash McKenzie, David Chase, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Roland Shoemaker.
1 comment:
Patchset:
I think it needs at least on "trusted" security-oriented person to eyeball it and +2 it. […]
Hello,
I also submitted, an alternative, more scoped CL based on this one
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change.
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Nicola Murino (Gerrit)
1 comment:
Patchset:
Ok, I'll close this out in favor of that.
That's just an alternative approach to fix the same issue. It depends on what the maintainers prefer. I think also this CL should be considered for a better evaluation
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.
Ash McKenzie (Gerrit)
Attention is currently required from: David Chase, Filippo Valsorda, Jakub Nyckowski, Maxime Tremblay, Nicola Murino, Roland Shoemaker.
1 comment:
Patchset:
👋 Is there anything I can do to help move this change along? I'm happy to test?
To view, visit change 412854. To unsubscribe, or for help writing mail filters, visit settings.