Skip to content

Commit 0ae49b8

Browse files
drakkangopherbot
authored andcommitted
ssh: reject certificate keys used as signature keys for SSH certs
As specified in draft-miller-ssh-cert-01, Section 2.1.1: Implementations MUST NOT accept certificate keys as CA keys. Change-Id: I2e559a8a58b7bceccd0d8c6b80803abdbe281067 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/678715 Reviewed-by: Filippo Valsorda <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Nicola Murino <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: David Chase <[email protected]>
1 parent 3bf9d2a commit 0ae49b8

File tree

3 files changed

+38
-1
lines changed

3 files changed

+38
-1
lines changed

ssh/certs.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,12 +447,19 @@ func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
447447
// SignCert signs the certificate with an authority, setting the Nonce,
448448
// SignatureKey, and Signature fields. If the authority implements the
449449
// MultiAlgorithmSigner interface the first algorithm in the list is used. This
450-
// is useful if you want to sign with a specific algorithm.
450+
// is useful if you want to sign with a specific algorithm. As specified in
451+
// [SSH-CERTS], Section 2.1.1, authority can't be a [Certificate].
451452
func (c *Certificate) SignCert(rand io.Reader, authority Signer) error {
452453
c.Nonce = make([]byte, 32)
453454
if _, err := io.ReadFull(rand, c.Nonce); err != nil {
454455
return err
455456
}
457+
// The Type() function is intended to return only certificate key types, but
458+
// we use certKeyAlgoNames anyway for safety, to match [Certificate.Type].
459+
if _, ok := certKeyAlgoNames[authority.PublicKey().Type()]; ok {
460+
return fmt.Errorf("ssh: certificates cannot be used as authority (public key type %q)",
461+
authority.PublicKey().Type())
462+
}
456463
c.SignatureKey = authority.PublicKey()
457464

458465
if v, ok := authority.(MultiAlgorithmSigner); ok {

ssh/certs_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,3 +404,32 @@ func TestCertSignWithMultiAlgorithmSigner(t *testing.T) {
404404
})
405405
}
406406
}
407+
408+
func TestCertSignWithCertificate(t *testing.T) {
409+
cert := &Certificate{
410+
Key: testPublicKeys["rsa"],
411+
ValidBefore: CertTimeInfinity,
412+
CertType: UserCert,
413+
}
414+
if err := cert.SignCert(rand.Reader, testSigners["ecdsa"]); err != nil {
415+
t.Fatalf("SignCert: %v", err)
416+
}
417+
signer, err := NewSignerWithAlgorithms(testSigners["rsa"].(AlgorithmSigner), []string{KeyAlgoRSASHA256})
418+
if err != nil {
419+
t.Fatal(err)
420+
}
421+
certSigner, err := NewCertSigner(cert, signer)
422+
if err != nil {
423+
t.Fatalf("NewCertSigner: %v", err)
424+
}
425+
426+
cert1 := &Certificate{
427+
Key: testPublicKeys["ecdsa"],
428+
ValidBefore: CertTimeInfinity,
429+
CertType: UserCert,
430+
}
431+
432+
if err := cert1.SignCert(rand.Reader, certSigner); err == nil {
433+
t.Fatal("successfully signed a certificate using another certificate, it is expected to fail")
434+
}
435+
}

ssh/doc.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ References:
1616
[PROTOCOL]: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL?rev=HEAD
1717
[PROTOCOL.certkeys]: http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?rev=HEAD
1818
[SSH-PARAMETERS]: http://www.iana.org/assignments/ssh-parameters/ssh-parameters.xml#ssh-parameters-1
19+
[SSH-CERTS]: https://datatracker.ietf.org/doc/html/draft-miller-ssh-cert-01
1920
2021
This package does not fall under the stability promise of the Go language itself,
2122
so its API may be changed when pressing needs arise.

0 commit comments

Comments
 (0)