From c6fce028266aa1271946a7dfde94cd71cf077d5e Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Wed, 4 Jun 2025 12:39:12 +0200 Subject: ssh: refuse to parse certificates that use a certificate as signing key According to draft-miller-ssh-cert-01, Section 2.1.1, certificates with certificate keys as signature keys are invalid Change-Id: I474524ea444deb78f2fa7c2682e47c0fd057f0b8 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/678716 LUCI-TryBot-Result: Go LUCI Reviewed-by: David Chase Auto-Submit: Nicola Murino Reviewed-by: Dmitri Shuralyov Reviewed-by: Filippo Valsorda --- ssh/certs.go | 17 +++++++++-------- ssh/keys.go | 2 +- ssh/keys_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/ssh/certs.go b/ssh/certs.go index 4f535eb..139fa31 100644 --- a/ssh/certs.go +++ b/ssh/certs.go @@ -233,7 +233,11 @@ func parseCert(in []byte, privAlgo string) (*Certificate, error) { if err != nil { return nil, err } - + // The Type() function is intended to return only certificate key types, but + // we use certKeyAlgoNames anyway for safety, to match [Certificate.Type]. + if _, ok := certKeyAlgoNames[k.Type()]; ok { + return nil, fmt.Errorf("ssh: the signature key type %q is invalid for certificates", k.Type()) + } c.SignatureKey = k c.Signature, rest, ok = parseSignatureBody(g.Signature) if !ok || len(rest) > 0 { @@ -301,16 +305,13 @@ type CertChecker struct { SupportedCriticalOptions []string // IsUserAuthority should return true if the key is recognized as an - // authority for the given user certificate. This allows for - // certificates to be signed by other certificates. This must be set - // if this CertChecker will be checking user certificates. + // authority for user certificate. This must be set if this CertChecker + // will be checking user certificates. IsUserAuthority func(auth PublicKey) bool // IsHostAuthority should report whether the key is recognized as - // an authority for this host. This allows for certificates to be - // signed by other keys, and for those other keys to only be valid - // signers for particular hostnames. This must be set if this - // CertChecker will be checking host certificates. + // an authority for this host. This must be set if this CertChecker + // will be checking host certificates. IsHostAuthority func(auth PublicKey, address string) bool // Clock is used for verifying time stamps. If nil, time.Now diff --git a/ssh/keys.go b/ssh/keys.go index 566e09d..a28c0de 100644 --- a/ssh/keys.go +++ b/ssh/keys.go @@ -273,7 +273,7 @@ func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []str return nil, "", nil, nil, errors.New("ssh: no key found") } -// ParsePublicKey parses an SSH public key formatted for use in +// ParsePublicKey parses an SSH public key or certificate formatted for use in // the SSH wire protocol according to RFC 4253, section 6.6. func ParsePublicKey(in []byte) (out PublicKey, err error) { algo, in, ok := parseString(in) diff --git a/ssh/keys_test.go b/ssh/keys_test.go index 7d5b86f..f3eb223 100644 --- a/ssh/keys_test.go +++ b/ssh/keys_test.go @@ -810,3 +810,30 @@ func TestCryptoPublicKey(t *testing.T) { } } } + +func TestParseCertWithCertSignatureKey(t *testing.T) { + certBytes := []byte(`-----BEGIN SSH CERTIFICATE----- +AAAAIHNzaC1lZDI1NTE5LWNlcnQtdjAxQG9wZW5zc2guY29tAAAAIPSp27hvNSB0 +IotJnVhjC4zxNgNS8BHlUCxD0VJi4D/eAAAAIIJMi1e5qfx+IFuKD/p/Ssqcb3os +CpOw/4wBs1pQ53zwAAAAAAAAAAEAAAACAAAAAAAAABMAAAAPZm9vLmV4YW1wbGUu +Y29tAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAT0AAAAgc3NoLWVkMjU1 +MTktY2VydC12MDFAb3BlbnNzaC5jb20AAAAg+sNYhCO35mQT1UBMpmMk8ey+culd +IU8vBlPEl4B07swAAAAggiv+RLnboS4znGCVl/n1jDg2uD0h15tW4s/04eS2mLQA +AAAAAAAAAQAAAAIAAAAAAAAAEwAAAA9mb28uZXhhbXBsZS5jb20AAAAAAAAAAAAA +AAAAAAAAAAAAAAAAAAAAAAAAAAAAMwAAAAtzc2gtZWQyNTUxOQAAACCV2wETgLKL +Kt0bRl3YUnd/ZYSlq0xJMbn4Jj3cdPWykQAAAFMAAAALc3NoLWVkMjU1MTkAAABA +WOdbRGEzyRAhiIK227CLUQD5caXYMV8FvSIB7toEE2M/8HnWdG9H3Rsg/v3unruQ +JrQldnuPJNe7KOP2+zvUDgAAAFMAAAALc3NoLWVkMjU1MTkAAABAm3bIPp85ZpIe +D+izJcUqlcAOri7HO8bULFNHT6LVegvB06xQ5TLwMlrxWUF4cafl1tSe8JQck4a6 +cLYUOHfQDw== +-----END SSH CERTIFICATE----- + `) + block, _ := pem.Decode(certBytes) + if block == nil { + t.Fatal("invalid test certificate") + } + + if _, err := ParsePublicKey(block.Bytes); err == nil { + t.Fatal("parsing an SSH certificate using another certificate as signature key succeeded; expected failure") + } +} -- cgit v1.3