ssh: reject certificate signature keys before recursing

parseCert parsed a certificate's SignatureKey by calling ParsePublicKey
again, and only afterwards rejected a certificate used as a signing key.
For a chain of nested certificates this recursed once per level, so a key
with enough nesting exhausted the goroutine stack and crashed with a fatal
stack overflow that recover cannot catch.

Reject the signature key by its declared algorithm before parsing it, which
bounds parsing.

Fixes golang/go#80414

Change-Id: Id51401aae15bfbec006c03ab97635a3b68bd6058
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/802145
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

ssh/certs.go

@@ -229,15 +229,20 @@ func parseCert(in []byte, privAlgo string) (*Certificate, error) {
 		return nil, err
 	}
 	c.Reserved = g.Reserved
+	// Reject a certificate whose signature key is itself a certificate before
+	// parsing it. Certificates signed by certificates are not supported (see
+	// PROTOCOL.certkeys), and rejecting after ParsePublicKey returns would allow
+	// a chain of nested certificates to recurse once per level, exhausting the
+	// goroutine stack.
+	if sigAlgo, _, ok := parseString(g.SignatureKey); !ok {
+		return nil, errShortRead
+	} else if _, ok := certKeyAlgoNames[string(sigAlgo)]; ok {
+		return nil, fmt.Errorf("ssh: the signature key type %q is invalid for certificates", sigAlgo)
+	}
 	k, err := ParsePublicKey(g.SignatureKey)
 	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 {

ssh/certs_test.go

@@ -15,6 +15,7 @@ import (
 	"math/big"
 	"net"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -45,6 +46,34 @@ func TestParseCert(t *testing.T) {
 	}
 }
 
+func TestParseCertNestedSignatureKey(t *testing.T) {
+	signer, err := NewSignerFromKey(testPrivateKeys["ed25519"])
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cert := &Certificate{
+		Key:         signer.PublicKey(),
+		CertType:    UserCert,
+		ValidBefore: CertTimeInfinity,
+	}
+	if err := cert.SignCert(rand.Reader, signer); err != nil {
+		t.Fatal(err)
+	}
+
+	inner := *cert
+	cert.SignatureKey = &inner
+	blob := cert.Marshal()
+
+	_, err = ParsePublicKey(blob)
+	if err == nil {
+		t.Fatal("ParsePublicKey: expected error for certificate signed by a certificate, got nil")
+	}
+	if !strings.Contains(err.Error(), "invalid for certificates") {
+		t.Errorf("ParsePublicKey: got error %q, want it to mention the signature key is invalid for certificates", err)
+	}
+}
+
 // Cert generated by ssh-keygen OpenSSH_6.8p1 OS X 10.10.3
 // % ssh-keygen -s ca -I testcert -O source-address=192.168.1.0/24 -O force-command=/bin/sleep user.pub
 // user.pub key: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDACh1rt2DXfV3hk6fszSQcQ/rueMId0kVD9U7nl8cfEnFxqOCrNT92g4laQIGl2mn8lsGZfTLg8ksHq3gkvgO3oo/0wHy4v32JeBOHTsN5AL4gfHNEhWeWb50ev47hnTsRIt9P4dxogeUo/hTu7j9+s9lLpEQXCvq6xocXQt0j8MV9qZBBXFLXVT3cWIkSqOdwt/5ZBg+1GSrc7WfCXVWgTk4a20uPMuJPxU4RQwZW6X3+O8Pqo8C3cW0OzZRFP6gUYUKUsTI5WntlS+LAxgw1mZNsozFGdbiOPRnEryE3SRldh9vjDR3tin1fGpA5P7+CEB/bqaXtG3V+F2OkqaMN