Skip to content

Commit

Permalink
ssh: add support for unpadded RSA signatures
Browse files Browse the repository at this point in the history
The original SSH RFC 4253 explicitly disallows padding. This applies to
ssh-rsa signatures.

The updated SSH RFC 8332 which defines the SHA2 RSA signature variants
explicitly calls out the existence of signers who produce short
signatures and specifies that verifiers may allow this behavior.

In practice, PuTTY 0.81 and prior versions, as well as SSH.NET prior to
2024.1.0 always generated short signatures. Furthermore, PuTTY is
embedded in other software like WinSCP and FileZilla, which are updated
on their own schedules as well. This leads to occasional unexplained
login errors, when using RSA keys.

OpenSSH server allows these short signatures for all RSA algorithms.

Fixes golang/go#68286

Change-Id: Ia60ece21bf9c111c490fac0c066443ed5ff7dd29
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/598534
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
imirkin authored and gopherbot committed Jul 26, 2024
1 parent bb80217 commit 3375612
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 1 deletion.
44 changes: 43 additions & 1 deletion ssh/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,49 @@ 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)

// Signatures in PKCS1v15 must match the key's modulus in
// length. However with SSH, some signers provide RSA
// signatures which are missing the MSB 0's of the bignum
// represented. With ssh-rsa signatures, this is encouraged by
// the spec (even though e.g. OpenSSH will give the full
// length unconditionally). With rsa-sha2-* signatures, the
// verifier is allowed to support these, even though they are
// out of spec. See RFC 4253 Section 6.6 for ssh-rsa and RFC
// 8332 Section 3 for rsa-sha2-* details.
//
// In practice:
// * OpenSSH always allows "short" signatures:
// https://github.com/openssh/openssh-portable/blob/V_9_8_P1/ssh-rsa.c#L526
// but always generates padded signatures:
// https://github.com/openssh/openssh-portable/blob/V_9_8_P1/ssh-rsa.c#L439
//
// * PuTTY versions 0.81 and earlier will generate short
// signatures for all RSA signature variants. Note that
// PuTTY is embedded in other software, such as WinSCP and
// FileZilla. At the time of writing, a patch has been
// applied to PuTTY to generate padded signatures for
// rsa-sha2-*, but not yet released:
// https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=a5bcf3d384e1bf15a51a6923c3724cbbee022d8e
//
// * SSH.NET versions 2024.0.0 and earlier will generate short
// signatures for all RSA signature variants, fixed in 2024.1.0:
// https://github.com/sshnet/SSH.NET/releases/tag/2024.1.0
//
// As a result, we pad these up to the key size by inserting
// leading 0's.
//
// Note that support for short signatures with rsa-sha2-* may
// be removed in the future due to such signatures not being
// allowed by the spec.
blob := sig.Blob
keySize := (*rsa.PublicKey)(r).Size()
if len(blob) < keySize {
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 {
Expand Down
38 changes: 38 additions & 0 deletions ssh/keys_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,44 @@ func TestKeySignWithAlgorithmVerify(t *testing.T) {
}
}

func TestKeySignWithShortSignature(t *testing.T) {
signer := testSigners["rsa"].(AlgorithmSigner)
pub := signer.PublicKey()
// Note: data obtained by empirically trying until a result
// starting with 0 appeared
tests := []struct {
algorithm string
data []byte
}{
{
algorithm: KeyAlgoRSA,
data: []byte("sign me92"),
},
{
algorithm: KeyAlgoRSASHA256,
data: []byte("sign me294"),
},
{
algorithm: KeyAlgoRSASHA512,
data: []byte("sign me60"),
},
}

for _, tt := range tests {
sig, err := signer.SignWithAlgorithm(rand.Reader, tt.data, tt.algorithm)
if err != nil {
t.Fatalf("Sign(%T): %v", signer, err)
}
if sig.Blob[0] != 0 {
t.Errorf("%s: Expected signature with a leading 0", tt.algorithm)
}
sig.Blob = sig.Blob[1:]
if err := pub.Verify(tt.data, sig); err != nil {
t.Errorf("publicKey.Verify(%s): %v", tt.algorithm, err)
}
}
}

func TestParseRSAPrivateKey(t *testing.T) {
key := testPrivateKeys["rsa"]

Expand Down

0 comments on commit 3375612

Please sign in to comment.