ssh: don't advertise rsa-sha2 algorithms if we can't use them

The server implementation looks at the HostKeys to advertise and
negotiate host key signature algorithms. A fundamental issue of the
Signer and AlgorithmSigner interfaces is that they don't expose the
supported signature algorithms, so really the server has to guess.

Currently, it would guess exclusively based on the PublicKey.Type,
regardless of whether the host key implemented AlgorithmSigner. This
means that a legacy Signer that only supports ssh-rsa still led the
server to negotiate rsa-sha2 algorithms. The server would then fail to
find a suitable host key to make the signature and crash.

This won't happen if only Signers from this package are used, but if a
custom Signer that doesn't support SignWithAlgorithm() but returns
"ssh-rsa" from PublicKey().Type() is used as a HostKey, the server is
vulnerable to DoS.

The only workable rules to determine what to advertise seems to be:

   1. a pure Signer will always Sign with the PublicKey.Type

   2. an AlgorithmSigner supports all algorithms associated with the
      PublicKey.Type

Rule number two means that we can't add new supported algorithms in the
future, which is not great, but it's too late to fix that.

rsaSigner was breaking rule number one, and although it would have been
fine where it's used, I didn't want to break our own interface contract.

It's unclear why we had separate test key entries for rsa-sha2
algorithms, since we can use the ssh-rsa key for those. The only test
that used them, TestCertTypes, seemed broken: the init was actually
failing at making the corresponding signers rsaSigners, and indeed the
test for the SHA-256 signer expected and checked a SHA-512 signature.

Pending CVE
For golang/go#49952

Change-Id: Ie658eefcadd87906e63fc7faae8249376aa96c79
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/392355
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

ssh/certs.go

@@ -440,10 +440,14 @@ func (c *Certificate) SignCert(rand io.Reader, authority Signer) error {
 	}
 	c.SignatureKey = authority.PublicKey()
 
-	if v, ok := authority.(AlgorithmSigner); ok {
-		if v.PublicKey().Type() == KeyAlgoRSA {
-			authority = &rsaSigner{v, KeyAlgoRSASHA512}
+	// Default to KeyAlgoRSASHA512 for ssh-rsa signers.
+	if v, ok := authority.(AlgorithmSigner); ok && v.PublicKey().Type() == KeyAlgoRSA {
+		sig, err := v.SignWithAlgorithm(rand, c.bytesForSigning(), KeyAlgoRSASHA512)
+		if err != nil {
+			return err
 		}
+		c.Signature = sig
+		return nil
 	}
 
 	sig, err := authority.Sign(rand, c.bytesForSigning())
@@ -454,30 +458,29 @@ func (c *Certificate) SignCert(rand io.Reader, authority Signer) error {
 	return nil
 }
 
-// certAlgoNames includes a mapping from signature algorithms to the
-// corresponding certificate signature algorithm.
-var certAlgoNames = map[string]string{
-	KeyAlgoRSA:        CertAlgoRSAv01,
-	KeyAlgoRSASHA256:  CertAlgoRSASHA256v01,
-	KeyAlgoRSASHA512:  CertAlgoRSASHA512v01,
-	KeyAlgoDSA:        CertAlgoDSAv01,
-	KeyAlgoECDSA256:   CertAlgoECDSA256v01,
-	KeyAlgoECDSA384:   CertAlgoECDSA384v01,
-	KeyAlgoECDSA521:   CertAlgoECDSA521v01,
-	KeyAlgoSKECDSA256: CertAlgoSKECDSA256v01,
-	KeyAlgoED25519:    CertAlgoED25519v01,
-	KeyAlgoSKED25519:  CertAlgoSKED25519v01,
+// certKeyAlgoNames is a mapping from known certificate algorithm names to the
+// corresponding public key signature algorithm.
+var certKeyAlgoNames = map[string]string{
+	CertAlgoRSAv01:        KeyAlgoRSA,
+	CertAlgoRSASHA256v01:  KeyAlgoRSASHA256,
+	CertAlgoRSASHA512v01:  KeyAlgoRSASHA512,
+	CertAlgoDSAv01:        KeyAlgoDSA,
+	CertAlgoECDSA256v01:   KeyAlgoECDSA256,
+	CertAlgoECDSA384v01:   KeyAlgoECDSA384,
+	CertAlgoECDSA521v01:   KeyAlgoECDSA521,
+	CertAlgoSKECDSA256v01: KeyAlgoSKECDSA256,
+	CertAlgoED25519v01:    KeyAlgoED25519,
+	CertAlgoSKED25519v01:  KeyAlgoSKED25519,
 }
 
-// certToPrivAlgo returns the underlying algorithm for a certificate algorithm.
-// Panics if a non-certificate algorithm is passed.
-func certToPrivAlgo(algo string) string {
-	for privAlgo, pubAlgo := range certAlgoNames {
-		if pubAlgo == algo {
-			return privAlgo
-		}
+// underlyingAlgo returns the signature algorithm associated with algo (which is
+// an advertised or negotiated public key or host key algorithm). These are
+// usually the same, except for certificate algorithms.
+func underlyingAlgo(algo string) string {
+	if a, ok := certKeyAlgoNames[algo]; ok {
+		return a
 	}
-	panic("unknown cert algorithm")
+	return algo
 }
 
 func (cert *Certificate) bytesForSigning() []byte {
@@ -523,11 +526,13 @@ func (c *Certificate) Marshal() []byte {
 
 // Type returns the certificate algorithm name. It is part of the PublicKey interface.
 func (c *Certificate) Type() string {
-	algo, ok := certAlgoNames[c.Key.Type()]
-	if !ok {
-		panic("unknown cert key type " + c.Key.Type())
+	keyType := c.Key.Type()
+	for certName, keyName := range certKeyAlgoNames {
+		if keyName == keyType {
+			return certName
+		}
 	}
-	return algo
+	panic("unknown certificate type for key type " + keyType)
 }
 
 // Verify verifies a signature against the certificate's public

ssh/certs_test.go

@@ -216,12 +216,12 @@ func TestHostKeyCert(t *testing.T) {
 		_, _, _, err = NewClientConn(c2, test.addr, config)
 
 		if (err == nil) != test.succeed {
-			t.Fatalf("NewClientConn(%q): %v", test.addr, err)
+			t.Errorf("NewClientConn(%q): %v", test.addr, err)
 		}
 
 		err = <-errc
 		if (err == nil) != test.succeed {
-			t.Fatalf("NewServerConn(%q): %v", test.addr, err)
+			t.Errorf("NewServerConn(%q): %v", test.addr, err)
 		}
 	}
 }
@@ -249,9 +249,7 @@ func TestCertTypes(t *testing.T) {
 		{CertAlgoECDSA521v01, testSigners["ecdsap521"], ""},
 		{CertAlgoED25519v01, testSigners["ed25519"], ""},
 		{CertAlgoRSAv01, testSigners["rsa"], KeyAlgoRSASHA512},
-		{CertAlgoRSAv01, &legacyRSASigner{testSigners["rsa"]}, KeyAlgoRSA},
-		{CertAlgoRSAv01, testSigners["rsa-sha2-256"], KeyAlgoRSASHA512},
-		{CertAlgoRSAv01, testSigners["rsa-sha2-512"], KeyAlgoRSASHA512},
+		{"legacyRSASigner", &legacyRSASigner{testSigners["rsa"]}, KeyAlgoRSA},
 		{CertAlgoDSAv01, testSigners["dsa"], ""},
 	}
 

ssh/client.go

@@ -113,25 +113,16 @@ func (c *connection) clientHandshake(dialAddress string, config *ClientConfig) e
 	return c.clientAuthenticate(config)
 }
 
-// verifyHostKeySignature verifies the host key obtained in the key
-// exchange.
+// verifyHostKeySignature verifies the host key obtained in the key exchange.
+// algo is the negotiated algorithm, and may be a certificate type.
 func verifyHostKeySignature(hostKey PublicKey, algo string, result *kexResult) error {
 	sig, rest, ok := parseSignatureBody(result.Signature)
 	if len(rest) > 0 || !ok {
 		return errors.New("ssh: signature parse error")
 	}
 
-	// For keys, underlyingAlgo is exactly algo. For certificates,
-	// we have to look up the underlying key algorithm that SSH
-	// uses to evaluate signatures.
-	underlyingAlgo := algo
-	for sigAlgo, certAlgo := range certAlgoNames {
-		if certAlgo == algo {
-			underlyingAlgo = sigAlgo
-		}
-	}
-	if sig.Format != underlyingAlgo {
-		return fmt.Errorf("ssh: invalid signature algorithm %q, expected %q", sig.Format, underlyingAlgo)
+	if a := underlyingAlgo(algo); sig.Format != a {
+		return fmt.Errorf("ssh: invalid signature algorithm %q, expected %q", sig.Format, a)
 	}
 
 	return hostKey.Verify(result.H, sig)

ssh/common.go

@@ -89,23 +89,33 @@ var supportedMACs = []string{
 
 var supportedCompressions = []string{compressionNone}
 
-// hashFuncs keeps the mapping of supported algorithms to their respective
-// hashes needed for signature verification.
+// hashFuncs keeps the mapping of supported signature algorithms to their
+// respective hashes needed for signing and verification.
 var hashFuncs = map[string]crypto.Hash{
-	KeyAlgoRSA:           crypto.SHA1,
-	KeyAlgoRSASHA256:     crypto.SHA256,
-	KeyAlgoRSASHA512:     crypto.SHA512,
-	KeyAlgoDSA:           crypto.SHA1,
-	KeyAlgoECDSA256:      crypto.SHA256,
-	KeyAlgoECDSA384:      crypto.SHA384,
-	KeyAlgoECDSA521:      crypto.SHA512,
-	CertAlgoRSAv01:       crypto.SHA1,
-	CertAlgoRSASHA256v01: crypto.SHA256,
-	CertAlgoRSASHA512v01: crypto.SHA512,
-	CertAlgoDSAv01:       crypto.SHA1,
-	CertAlgoECDSA256v01:  crypto.SHA256,
-	CertAlgoECDSA384v01:  crypto.SHA384,
-	CertAlgoECDSA521v01:  crypto.SHA512,
+	KeyAlgoRSA:       crypto.SHA1,
+	KeyAlgoRSASHA256: crypto.SHA256,
+	KeyAlgoRSASHA512: crypto.SHA512,
+	KeyAlgoDSA:       crypto.SHA1,
+	KeyAlgoECDSA256:  crypto.SHA256,
+	KeyAlgoECDSA384:  crypto.SHA384,
+	KeyAlgoECDSA521:  crypto.SHA512,
+	// KeyAlgoED25519 doesn't pre-hash.
+	KeyAlgoSKECDSA256: crypto.SHA256,
+	KeyAlgoSKED25519:  crypto.SHA256,
+}
+
+// algorithmsForKeyFormat returns the supported signature algorithms for a given
+// public key format (PublicKey.Type), in order of preference. See RFC 8332,
+// Section 2. See also the note in sendKexInit on backwards compatibility.
+func algorithmsForKeyFormat(keyFormat string) []string {
+	switch keyFormat {
+	case KeyAlgoRSA:
+		return []string{KeyAlgoRSASHA256, KeyAlgoRSASHA512, KeyAlgoRSA}
+	case CertAlgoRSAv01:
+		return []string{CertAlgoRSASHA256v01, CertAlgoRSASHA512v01, CertAlgoRSAv01}
+	default:
+		return []string{keyFormat}
+	}
 }
 
 // unexpectedMessageError results when the SSH message that we received didn't

ssh/handshake.go

@@ -455,21 +455,29 @@ func (t *handshakeTransport) sendKexInit() error {
 	}
 	io.ReadFull(rand.Reader, msg.Cookie[:])
 
-	if len(t.hostKeys) > 0 {
+	isServer := len(t.hostKeys) > 0
+	if isServer {
 		for _, k := range t.hostKeys {
-			algo := k.PublicKey().Type()
-			switch algo {
-			case KeyAlgoRSA:
-				msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, []string{KeyAlgoRSASHA512, KeyAlgoRSASHA256, KeyAlgoRSA}...)
-			case CertAlgoRSAv01:
-				msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, []string{CertAlgoRSASHA512v01, CertAlgoRSASHA256v01, CertAlgoRSAv01}...)
-			default:
-				msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, algo)
+			// If k is an AlgorithmSigner, presume it supports all signature algorithms
+			// associated with the key format. (Ideally AlgorithmSigner would have a
+			// method to advertise supported algorithms, but it doesn't. This means that
+			// adding support for a new algorithm is a breaking change, as we will
+			// immediately negotiate it even if existing implementations don't support
+			// it. If that ever happens, we'll have to figure something out.)
+			// If k is not an AlgorithmSigner, we can only assume it only supports the
+			// algorithms that matches the key format. (This means that Sign can't pick
+			// a different default.)
+			keyFormat := k.PublicKey().Type()
+			if _, ok := k.(AlgorithmSigner); ok {
+				msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, algorithmsForKeyFormat(keyFormat)...)
+			} else {
+				msg.ServerHostKeyAlgos = append(msg.ServerHostKeyAlgos, keyFormat)
 			}
 		}
 	} else {
 		msg.ServerHostKeyAlgos = t.hostKeyAlgorithms
 	}
+
 	packet := Marshal(msg)
 
 	// writePacket destroys the contents, so save a copy.
@@ -589,9 +597,9 @@ func (t *handshakeTransport) enterKeyExchange(otherInitPacket []byte) error {
 
 	var result *kexResult
 	if len(t.hostKeys) > 0 {
-		result, err = t.server(kex, t.algorithms, &magics)
+		result, err = t.server(kex, &magics)
 	} else {
-		result, err = t.client(kex, t.algorithms, &magics)
+		result, err = t.client(kex, &magics)
 	}
 
 	if err != nil {
@@ -618,33 +626,52 @@ func (t *handshakeTransport) enterKeyExchange(otherInitPacket []byte) error {
 	return nil
 }
 
-func (t *handshakeTransport) server(kex kexAlgorithm, algs *algorithms, magics *handshakeMagics) (*kexResult, error) {
-	var hostKey Signer
-	for _, k := range t.hostKeys {
-		kt := k.PublicKey().Type()
-		if kt == algs.hostKey {
-			hostKey = k
-		} else if signer, ok := k.(AlgorithmSigner); ok {
-			// Some signature algorithms don't show up as key types
-			// so we have to manually check for a compatible host key.
-			switch kt {
-			case KeyAlgoRSA:
-				if algs.hostKey == KeyAlgoRSASHA256 || algs.hostKey == KeyAlgoRSASHA512 {
-					hostKey = &rsaSigner{signer, algs.hostKey}
-				}
-			case CertAlgoRSAv01:
-				if algs.hostKey == CertAlgoRSASHA256v01 || algs.hostKey == CertAlgoRSASHA512v01 {
-					hostKey = &rsaSigner{signer, certToPrivAlgo(algs.hostKey)}
-				}
+// algorithmSignerWrapper is an AlgorithmSigner that only supports the default
+// key format algorithm.
+//
+// This is technically a violation of the AlgorithmSigner interface, but it
+// should be unreachable given where we use this. Anyway, at least it returns an
+// error instead of panicing or producing an incorrect signature.
+type algorithmSignerWrapper struct {
+	Signer
+}
+
+func (a algorithmSignerWrapper) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error) {
+	if algorithm != underlyingAlgo(a.PublicKey().Type()) {
+		return nil, errors.New("ssh: internal error: algorithmSignerWrapper invoked with non-default algorithm")
+	}
+	return a.Sign(rand, data)
+}
+
+func pickHostKey(hostKeys []Signer, algo string) AlgorithmSigner {
+	for _, k := range hostKeys {
+		if algo == k.PublicKey().Type() {
+			return algorithmSignerWrapper{k}
+		}
+		k, ok := k.(AlgorithmSigner)
+		if !ok {
+			continue
+		}
+		for _, a := range algorithmsForKeyFormat(k.PublicKey().Type()) {
+			if algo == a {
+				return k
 			}
 		}
 	}
+	return nil
+}
+
+func (t *handshakeTransport) server(kex kexAlgorithm, magics *handshakeMagics) (*kexResult, error) {
+	hostKey := pickHostKey(t.hostKeys, t.algorithms.hostKey)
+	if hostKey == nil {
+		return nil, errors.New("ssh: internal error: negotiated unsupported signature type")
+	}
 
-	r, err := kex.Server(t.conn, t.config.Rand, magics, hostKey)
+	r, err := kex.Server(t.conn, t.config.Rand, magics, hostKey, t.algorithms.hostKey)
 	return r, err
 }
 
-func (t *handshakeTransport) client(kex kexAlgorithm, algs *algorithms, magics *handshakeMagics) (*kexResult, error) {
+func (t *handshakeTransport) client(kex kexAlgorithm, magics *handshakeMagics) (*kexResult, error) {
 	result, err := kex.Client(t.conn, t.config.Rand, magics)
 	if err != nil {
 		return nil, err
@@ -655,7 +682,7 @@ func (t *handshakeTransport) client(kex kexAlgorithm, algs *algorithms, magics *
 		return nil, err
 	}
 
-	if err := verifyHostKeySignature(hostKey, algs.hostKey, result); err != nil {
+	if err := verifyHostKeySignature(hostKey, t.algorithms.hostKey, result); err != nil {
 		return nil, err
 	}
 

ssh/handshake_test.go

@@ -583,3 +583,38 @@ func TestHandshakeAEADCipherNoMAC(t *testing.T) {
 		<-checker.called
 	}
 }
+
+// TestNoSHA2Support tests a host key Signer that is not an AlgorithmSigner and
+// therefore can't do SHA-2 signatures. Ensures the server does not advertise
+// support for them in this case.
+func TestNoSHA2Support(t *testing.T) {
+	c1, c2, err := netPipe()
+	if err != nil {
+		t.Fatalf("netPipe: %v", err)
+	}
+	defer c1.Close()
+	defer c2.Close()
+
+	serverConf := &ServerConfig{
+		PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) {
+			return &Permissions{}, nil
+		},
+	}
+	serverConf.AddHostKey(&legacyRSASigner{testSigners["rsa"]})
+	go func() {
+		_, _, _, err := NewServerConn(c1, serverConf)
+		if err != nil {
+			t.Error(err)
+		}
+	}()
+
+	clientConf := &ClientConfig{
+		User:            "test",
+		Auth:            []AuthMethod{Password("testpw")},
+		HostKeyCallback: FixedHostKey(testSigners["rsa"].PublicKey()),
+	}
+
+	if _, _, _, err := NewClientConn(c2, "", clientConf); err != nil {
+		t.Fatal(err)
+	}
+}