ssh: support rsa-sha2-256/512 for client authentication

CL 220037 had implemented support for host authentication using
rsa-sha2-256/512, but not client public key authentication. OpenSSH
disabled the SHA-1 based ssh-rsa by default in version 8.8 (after
pre-announcing it in versions 8.2, 8.3, 8.4, 8.5, 8.6, and 8.7) although
some distributions re-enable it. GitHub will start rejecting ssh-rsa for
keys uploaded before November 2, 2021 on March 15, 2022.

https://github.blog/2021-09-01-improving-git-protocol-security-github/

The server side already worked, as long as the client selected one of
the SHA-2 algorithms, because the signature flowed freely to Verify.
There was however nothing verifying that the signature algorithm matched
the advertised one. The comment suggested the check was being performed,
but it got lost back in CL 86190043. Not a security issue because the
signature had to pass the callback's Verify method regardless, and both
values were checked to be acceptable.

Tested with OpenSSH 8.8 configured with "PubkeyAcceptedKeyTypes -ssh-rsa"
and no application-side changes.

The Signers returned by ssh/agent (when backed by an agent client)
didn't actually implement AlgorithmSigner but ParameterizedSigner, an
interface defined in an earlier version of CL 123955.

Changed the type in TestClientAuthErrorList just so we wouldn't get 3
errors for one key.

Fixes golang/go#39885
For golang/go#49952

Change-Id: I13b41db8041f1112a70f106c55f077b904b12cb8

ssh/agent/client.go

@@ -25,7 +25,6 @@ import (
 	"math/big"
 	"sync"
 
-	"crypto"
 	"golang.org/x/crypto/ed25519"
 	"golang.org/x/crypto/ssh"
 )
@@ -771,19 +770,26 @@ func (s *agentKeyringSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature,
 	return s.agent.Sign(s.pub, data)
 }
 
-func (s *agentKeyringSigner) SignWithOpts(rand io.Reader, data []byte, opts crypto.SignerOpts) (*ssh.Signature, error) {
+func (s *agentKeyringSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) {
+	if algorithm == "" || algorithm == s.pub.Type() {
+		return s.Sign(rand, data)
+	}
+
 	var flags SignatureFlags
-	if opts != nil {
-		switch opts.HashFunc() {
-		case crypto.SHA256:
-			flags = SignatureFlagRsaSha256
-		case crypto.SHA512:
-			flags = SignatureFlagRsaSha512
-		}
+	switch algorithm {
+	case ssh.KeyAlgoRSASHA256:
+		flags = SignatureFlagRsaSha256
+	case ssh.KeyAlgoRSASHA512:
+		flags = SignatureFlagRsaSha512
+	default:
+		return nil, fmt.Errorf("agent: unsupported algorithm %q", algorithm)
 	}
+
 	return s.agent.SignWithFlags(s.pub, data, flags)
 }
 
+var _ ssh.AlgorithmSigner = &agentKeyringSigner{}
+
 // Calls an extension method. It is up to the agent implementation as to whether or not
 // any particular extension is supported and may always return an error. Because the
 // type of the response is up to the implementation, this returns the bytes of the

ssh/client_auth.go

@@ -201,54 +201,78 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
 	}
 	var methods []string
 	for _, signer := range signers {
-		ok, err := validateKey(signer.PublicKey(), user, c)
-		if err != nil {
-			return authFailure, nil, err
-		}
-		if !ok {
-			continue
-		}
-
 		pub := signer.PublicKey()
-		pubKey := pub.Marshal()
-		sign, err := signer.Sign(rand, buildDataSignedForAuth(session, userAuthRequestMsg{
-			User:    user,
-			Service: serviceSSH,
-			Method:  cb.method(),
-		}, []byte(pub.Type()), pubKey))
-		if err != nil {
-			return authFailure, nil, err
-		}
 
-		// manually wrap the serialized signature in a string
-		s := Marshal(sign)
-		sig := make([]byte, stringLength(len(s)))
-		marshalString(sig, s)
-		msg := publickeyAuthMsg{
-			User:     user,
-			Service:  serviceSSH,
-			Method:   cb.method(),
-			HasSig:   true,
-			Algoname: pub.Type(),
-			PubKey:   pubKey,
-			Sig:      sig,
-		}
-		p := Marshal(&msg)
-		if err := c.writePacket(p); err != nil {
-			return authFailure, nil, err
-		}
-		var success authResult
-		success, methods, err = handleAuthResponse(c)
-		if err != nil {
-			return authFailure, nil, err
-		}
+		// Like in sendKexInit, if the public key implements AlgorithmSigner we
+		// assume it supports all algorithms, otherwise only the key format one.
+		var algorithms []string
+		as, ok := signer.(AlgorithmSigner)
+		if ok {
+			algorithms = algorithmsForKeyFormat(pub.Type())
+		} else {
+			as = algorithmSignerWrapper{signer}
+			algorithms = []string{pub.Type()}
+		}
+
+		// We don't implement RFC 8308 extensions, somewhat intentionally
+		// (because they involve a speculative read), which means we can't use
+		// the "server-sig-algs" extension to learn about the server's supported
+		// algorithms. The concern with trial and error, besides the
+		// round-trips, is that the server might react poorly upon seeing an
+		// unsupported algorithm. OpenSSH has supported the only "additional"
+		// algorithms we support (the rsa-sha2 ones) since 2016. Feels like it's
+		// fine to default to them. If an application doesn't want that, they
+		// can wrap the HostKey in a Signer without SignWithAlgorithm.
+		for _, algo := range algorithms {
+			ok, err := validateKey(pub, algo, user, c)
+			if err != nil {
+				return authFailure, nil, err
+			}
+			if !ok {
+				continue
+			}
+
+			pubKey := pub.Marshal()
+			data := buildDataSignedForAuth(session, userAuthRequestMsg{
+				User:    user,
+				Service: serviceSSH,
+				Method:  cb.method(),
+			}, algo, pubKey)
+			sign, err := as.SignWithAlgorithm(rand, data, underlyingAlgo(algo))
+			if err != nil {
+				return authFailure, nil, err
+			}
 
-		// If authentication succeeds or the list of available methods does not
-		// contain the "publickey" method, do not attempt to authenticate with any
-		// other keys.  According to RFC 4252 Section 7, the latter can occur when
-		// additional authentication methods are required.
-		if success == authSuccess || !containsMethod(methods, cb.method()) {
-			return success, methods, err
+			// manually wrap the serialized signature in a string
+			s := Marshal(sign)
+			sig := make([]byte, stringLength(len(s)))
+			marshalString(sig, s)
+			msg := publickeyAuthMsg{
+				User:     user,
+				Service:  serviceSSH,
+				Method:   cb.method(),
+				HasSig:   true,
+				Algoname: algo,
+				PubKey:   pubKey,
+				Sig:      sig,
+			}
+			p := Marshal(&msg)
+			if err := c.writePacket(p); err != nil {
+				return authFailure, nil, err
+			}
+			var success authResult
+			success, methods, err = handleAuthResponse(c)
+			if err != nil {
+				return authFailure, nil, err
+			}
+
+			// If authentication succeeds or the list of available methods does not
+			// contain the "publickey" method, do not attempt to authenticate with any
+			// other keys.  According to RFC 4252 Section 7, the latter can occur when
+			// additional authentication methods are required.
+			if success == authSuccess || !containsMethod(methods, cb.method()) {
+				return success, methods, err
+			}
 		}
 	}
 
@@ -266,26 +290,25 @@ func containsMethod(methods []string, method string) bool {
 }
 
 // validateKey validates the key provided is acceptable to the server.
-func validateKey(key PublicKey, user string, c packetConn) (bool, error) {
+func validateKey(key PublicKey, algo string, user string, c packetConn) (bool, error) {
 	pubKey := key.Marshal()
 	msg := publickeyAuthMsg{
 		User:     user,
 		Service:  serviceSSH,
 		Method:   "publickey",
 		HasSig:   false,
-		Algoname: key.Type(),
+		Algoname: algo,
 		PubKey:   pubKey,
 	}
 	if err := c.writePacket(Marshal(&msg)); err != nil {
 		return false, err
 	}
 
-	return confirmKeyAck(key, c)
+	return confirmKeyAck(key, algo, c)
 }
 
-func confirmKeyAck(key PublicKey, c packetConn) (bool, error) {
+func confirmKeyAck(key PublicKey, algo string, c packetConn) (bool, error) {
 	pubKey := key.Marshal()
-	algoname := key.Type()
 
 	for {
 		packet, err := c.readPacket()
@@ -302,7 +325,7 @@ func confirmKeyAck(key PublicKey, c packetConn) (bool, error) {
 			if err := Unmarshal(packet, &msg); err != nil {
 				return false, err
 			}
-			if msg.Algo != algoname || !bytes.Equal(msg.PubKey, pubKey) {
+			if msg.Algo != algo || !bytes.Equal(msg.PubKey, pubKey) {
 				return false, nil
 			}
 			return true, nil

ssh/client_auth_test.go

@@ -105,11 +105,44 @@ func tryAuthBothSides(t *testing.T, config *ClientConfig, gssAPIWithMICConfig *G
 	return err, serverAuthErrors
 }
 
+type loggingAlgorithmSigner struct {
+	used []string
+	AlgorithmSigner
+}
+
+func (l *loggingAlgorithmSigner) Sign(rand io.Reader, data []byte) (*Signature, error) {
+	l.used = append(l.used, "[Sign]")
+	return l.AlgorithmSigner.Sign(rand, data)
+}
+
+func (l *loggingAlgorithmSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*Signature, error) {
+	l.used = append(l.used, algorithm)
+	return l.AlgorithmSigner.SignWithAlgorithm(rand, data, algorithm)
+}
+
 func TestClientAuthPublicKey(t *testing.T) {
+	signer := &loggingAlgorithmSigner{AlgorithmSigner: testSigners["rsa"].(AlgorithmSigner)}
 	config := &ClientConfig{
 		User: "testuser",
 		Auth: []AuthMethod{
-			PublicKeys(testSigners["rsa"]),
+			PublicKeys(signer),
+		},
+		HostKeyCallback: InsecureIgnoreHostKey(),
+	}
+	if err := tryAuth(t, config); err != nil {
+		t.Fatalf("unable to dial remote side: %s", err)
+	}
+	if len(signer.used) != 1 || signer.used[0] != KeyAlgoRSASHA256 {
+		t.Errorf("unexpected Sign/SignWithAlgorithm calls: %q", signer.used)
+	}
+}
+
+// TestClientAuthNoSHA2 tests a ssh-rsa Signer that doesn't implement AlgorithmSigner.
+func TestClientAuthNoSHA2(t *testing.T) {
+	config := &ClientConfig{
+		User: "testuser",
+		Auth: []AuthMethod{
+			PublicKeys(&legacyRSASigner{testSigners["rsa"]}),
 		},
 		HostKeyCallback: InsecureIgnoreHostKey(),
 	}
@@ -659,7 +692,7 @@ func TestClientAuthErrorList(t *testing.T) {
 
 	clientConfig := &ClientConfig{
 		Auth: []AuthMethod{
-			PublicKeys(testSigners["rsa"]),
+			PublicKeys(testSigners["ecdsa"]),
 		},
 		HostKeyCallback: InsecureIgnoreHostKey(),
 	}

ssh/common.go

@@ -297,16 +297,17 @@ func (c *Config) SetDefaults() {
 }
 
 // buildDataSignedForAuth returns the data that is signed in order to prove
-// possession of a private key. See RFC 4252, section 7.
-func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo, pubKey []byte) []byte {
+// possession of a private key. See RFC 4252, section 7. algo is the advertised
+// algorithm, and may be a certificate type.
+func buildDataSignedForAuth(sessionID []byte, req userAuthRequestMsg, algo string, pubKey []byte) []byte {
 	data := struct {
 		Session []byte
 		Type    byte
 		User    string
 		Service string
 		Method  string
 		Sign    bool
-		Algo    []byte
+		Algo    string
 		PubKey  []byte
 	}{
 		sessionID,

ssh/server.go

@@ -554,6 +554,7 @@ userAuthLoop:
 				if !ok || len(payload) > 0 {
 					return nil, parseError(msgUserAuthRequest)
 				}
+
 				// Ensure the public key algo and signature algo
 				// are supported.  Compare the private key
 				// algorithm name that corresponds to algo with
@@ -563,7 +564,12 @@ userAuthLoop:
 					authErr = fmt.Errorf("ssh: algorithm %q not accepted", sig.Format)
 					break
 				}
-				signedData := buildDataSignedForAuth(sessionID, userAuthReq, algoBytes, pubKeyData)
+				if underlyingAlgo(algo) != sig.Format {
+					authErr = fmt.Errorf("ssh: signature %q not compatible with selected algorithm %q", sig.Format, algo)
+					break
+				}
+
+				signedData := buildDataSignedForAuth(sessionID, userAuthReq, algo, pubKeyData)
 
 				if err := pubKey.Verify(signedData, sig); err != nil {
 					return nil, err