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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

Author: drakkan

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

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)

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")
+	}
+}