x/crypto/ssh: allow unpadded signatures

[imirkin]

Go version

go version go1.20.13 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=""
GOARCH="amd64"
GOVERSION="go1.20.13"
GCCGO="gccgo"
GOAMD64="v1"

What did you do?

Connect a lot of times to the SSH server with an RSA public key using PuTTY.

What did you see happen?

Occasional login failures with crypto/rsa: verification error

What did you expect to see?

All successes.

The issue is due to a short signature, rejected by rsa.VerifyPKCS1v15.

https://datatracker.ietf.org/doc/html/rfc4253#section-6.6

The resulting signature is encoded as follows:

  string    "ssh-rsa"
  string    rsa_signature_blob

The value for 'rsa_signature_blob' is encoded as a string containing
s (which is an integer, without lengths or padding, unsigned, and in
network byte order).

Which requires the signature to be unpadded.

I spoke with the PuTTY maintainer about this, Simon Tatham. His view is that the SSH RFC supercedes the PKCS RFC (8017), so the short signature is OK (in fact required).

However this approach is reversed in

https://datatracker.ietf.org/doc/html/rfc8332#section-3

The resulting signature is encoded as follows:

string "rsa-sha2-256" / "rsa-sha2-512"
string rsa_signature_blob

The value for 'rsa_signature_blob' is encoded as a string that
contains an octet string S (which is the output of RSASSA-PKCS1-v1_5)
and that has the same length (in octets) as the RSA modulus. When S
contains leading zeros, there exist signers that will send a shorter
encoding of S that omits them. A verifier MAY accept shorter
encodings of S with one or more leading zeros omitted.

and I believe that PuTTY may be fixed for this when using the new signature types (or even always). But even if it is, lots of PuTTY installs out there that will not be updated for a long time. (And WinSCP embeds PuTTY, thus has a similar issue... FileZilla as well potentially.)

In practice, the OpenSSH verify logic always allows unpadded signatures, while the sign logic always pads them (at least based on a quick read of https://github.com/openssh/openssh-portable/blob/master/ssh-rsa.c ssh_rsa_sign and ssh_rsa_verify). The current Go implementation is out of spec for ssh-rsa signatures, but it would be the flexible thing to do to also always allow the short signatures, as this is allowed by the RFC and (arguably) the most popular SSH server.

I'm happy to write up a patch if there are any prospects of it being accepted (past experience suggests this is best left to the core team though).

[gabyhelp]

Related Issues

• x/crypto/ssh: Consider relaxing public key and signature matching for RSA keys in SSH_MSG_USERAUTH_REQUEST #53391 (closed)
• x/crypto/ssh: server does not accept rsa-sha2-256 and rsa-sha2-512 signatures #46569 (closed)
• x/crypto/ssh: SSH_MSG_USERAUTH_PK_OK handling differs from OpenSSH #64785 (closed)
• x/crypto/ssh: "ssh-rsa-cert-v01@openssh.com" does not work for sshd OpenSSH 7.2-7.7 #58371 (closed)
• x/crypto/ssh: publicKeyCallback cannot handshake using ssh-rsa keys signed using the ssh-rsa-sha2-256 algorithm #39885 (closed)
• x/crypto/ssh: OpenSSH 8.8 disabled RSA signatures using the SHA-1 hash algorithm #48758 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[imirkin]

Sample white-space-damaged patch which fixes it (although I'm not 100% sure if rsaPublicKey::Verify is ever called in cases where the short signature should not be accepted:

diff --git a/vendor/golang.org/x/crypto/ssh/keys.go b/vendor/golang.org/x/crypto/ssh/keys.go
index 7967665f1..1cf8e80d7 100644
--- a/vendor/golang.org/x/crypto/ssh/keys.go
+++ b/vendor/golang.org/x/crypto/ssh/keys.go
@@ -488,7 +488,15 @@ func (r *rsaPublicKey) Verify(data []byte, sig *Signature) error {
        h := hash.New()
        h.Write(data)
        digest := h.Sum(nil)
-       return rsa.VerifyPKCS1v15((*rsa.PublicKey)(r), hash, digest, sig.Blob)
+       blob := sig.Blob
+       keySize := (*rsa.PublicKey)(r).Size()
+       if len(blob) < keySize {
+               // Pad MSB bytes. Required by RFC 4253, allowed by RFC 8332.
+               padded := make([]byte, keySize)
+               copy(padded[keySize - len(blob):], blob)
+               blob = padded
+       }
+       return rsa.VerifyPKCS1v15((*rsa.PublicKey)(r), hash, digest, blob)
 }
 
 func (r *rsaPublicKey) CryptoPublicKey() crypto.PublicKey {

[thanm]

@drakkan @golang/security per owners

[imirkin]

Note the PuTTY commit here: https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=a5bcf3d384e1bf15a51a6923c3724cbbee022d8e

They will continue to send unpadded signatures for ssh-rsa, but will provided padded versions for rsa-sha2-*.

[drakkan]

Hi @imirkin

I think we should pad only ssh-rsa signatures to be compliant with the specs. Do you want to submit a CL including a small test case with a short signature? Thank you

[imirkin]

Hi @drakkan,

You could be compliant with the specs and allow the short signatures on rsa-sha2-* as well.

A verifier MAY accept shorter encodings of S with one or more leading zeros omitted.

And this would allow x/crypto/ssh to work with non-updated PuTTY versions that support the SHA2 signature algorithms (which are also included in WinSCP and FileZilla), which I strongly suspect there is a large installed base of (and will remain for years to come). The thing is that it works 99% of the time, and people just blame internet gremlins for the occasional failures. But I view this as a very undesirable state.

I'd strongly encourage allowing the short signatures for all RSA variants.

[drakkan]

@imirkin In general I think it is better to keep legacy code to a minimum. Putty will be updated over time and also ssh-rsa support will be removed in the future, so we may get rid of this code in the future. At the same time it needs to be added now because we are not compliant with the spec

[imirkin]

@drakkan So what about everyone who has to support actual users? Create & maintain a private copy of x/crypto/ssh for a decade until the version of Windows runs out of support and people are forced to think about upgrades? The SSH library has various allowances for how OpenSSH clients actually work, various oddities around picking ssh-rsa vs rsa-sha2 algos/allowing "wrong" values, in order to work with actual software. Why not let it work with PuTTY and other Windows clients that embed it?

And I'll point out again that allowing short signatures is actually allowed (but not required) by the spec.

[drakkan]

@imirkin I understand that you have a problem now and want to fix it in one place without requiring your clients to update anything and we will take that into account. It is unlikely that putty will not have a security bug for the next 10 years so the various clients should be updated sooner or later and ssh-rsa signature format is already deprecated and will be removed in the future.

If you want to submit a CL we can also discuss about this during the review. Thanks

[imirkin]

FWIW, it's not just PuTTY and its embeddors. SSH.NET had a similar bug (very recently fixed) too:

sshnet/SSH.NET#1388

[To give full credit: this bug, which was already closed by the time I saw it is what led me to understand what I was seeing with some of my users.]

I'll try to put a patch together this week for x/crypto/ssh with a test.

[gopherbot]

Change https://go.dev/cl/598534 mentions this issue: ssh: add support for unpadded RSA signatures