crypto/tls: enable signature algorithm BoGo tests (and fix two bugs)

The two bugs are very minor:

- We were trying to set the ConnectionState CurveID field even if the
  RSA key exchange was in use

- We were sending the wrong alert from TLS 1.2 clients if none of the
  certificate signature algorithms were supported

Change-Id: I6a6a46564f5a9f1a5d44e54fc59a650118ad67d5
Reviewed-on: https://go-review.googlesource.com/c/go/+/675918
Auto-Submit: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

Author: FiloSottile

auth.go

@@ -149,20 +149,18 @@ func legacyTypeAndHashFromPublicKey(pub crypto.PublicKey) (sigType uint8, hash c
 var rsaSignatureSchemes = []struct {
 	scheme          SignatureScheme
 	minModulusBytes int
-	maxVersion      uint16
 }{
 	// RSA-PSS is used with PSSSaltLengthEqualsHash, and requires
 	//    emLen >= hLen + sLen + 2
-	{PSSWithSHA256, crypto.SHA256.Size()*2 + 2, VersionTLS13},
-	{PSSWithSHA384, crypto.SHA384.Size()*2 + 2, VersionTLS13},
-	{PSSWithSHA512, crypto.SHA512.Size()*2 + 2, VersionTLS13},
+	{PSSWithSHA256, crypto.SHA256.Size()*2 + 2},
+	{PSSWithSHA384, crypto.SHA384.Size()*2 + 2},
+	{PSSWithSHA512, crypto.SHA512.Size()*2 + 2},
 	// PKCS #1 v1.5 uses prefixes from hashPrefixes in crypto/rsa, and requires
 	//    emLen >= len(prefix) + hLen + 11
-	// TLS 1.3 dropped support for PKCS #1 v1.5 in favor of RSA-PSS.
-	{PKCS1WithSHA256, 19 + crypto.SHA256.Size() + 11, VersionTLS12},
-	{PKCS1WithSHA384, 19 + crypto.SHA384.Size() + 11, VersionTLS12},
-	{PKCS1WithSHA512, 19 + crypto.SHA512.Size() + 11, VersionTLS12},
-	{PKCS1WithSHA1, 15 + crypto.SHA1.Size() + 11, VersionTLS12},
+	{PKCS1WithSHA256, 19 + crypto.SHA256.Size() + 11},
+	{PKCS1WithSHA384, 19 + crypto.SHA384.Size() + 11},
+	{PKCS1WithSHA512, 19 + crypto.SHA512.Size() + 11},
+	{PKCS1WithSHA1, 15 + crypto.SHA1.Size() + 11},
 }
 
 // signatureSchemesForCertificate returns the list of supported SignatureSchemes
@@ -202,7 +200,7 @@ func signatureSchemesForCertificate(version uint16, cert *Certificate) []Signatu
 		size := pub.Size()
 		sigAlgs = make([]SignatureScheme, 0, len(rsaSignatureSchemes))
 		for _, candidate := range rsaSignatureSchemes {
-			if size >= candidate.minModulusBytes && version <= candidate.maxVersion {
+			if size >= candidate.minModulusBytes {
 				sigAlgs = append(sigAlgs, candidate.scheme)
 			}
 		}
@@ -220,9 +218,8 @@ func signatureSchemesForCertificate(version uint16, cert *Certificate) []Signatu
 
 	// Filter out any unsupported signature algorithms, for example due to
 	// FIPS 140-3 policy, or any downstream changes to defaults.go.
-	supportedAlgs := supportedSignatureAlgorithms(version)
 	sigAlgs = slices.DeleteFunc(sigAlgs, func(sigAlg SignatureScheme) bool {
-		return !isSupportedSignatureAlgorithm(sigAlg, supportedAlgs)
+		return isDisabledSignatureAlgorithm(version, sigAlg, false)
 	})
 
 	return sigAlgs

common.go

@@ -317,6 +317,10 @@ type ConnectionState struct {
 
 	// testingOnlyDidHRR is true if a HelloRetryRequest was sent/received.
 	testingOnlyDidHRR bool
+
+	// testingOnlyPeerSignatureAlgorithm is the signature algorithm used by the
+	// peer to sign the handshake. It is not set for resumed connections.
+	testingOnlyPeerSignatureAlgorithm SignatureScheme
 }
 
 // ExportKeyingMaterial returns length bytes of exported key material in a new
@@ -1757,35 +1761,64 @@ func unexpectedMessageError(wanted, got any) error {
 	return fmt.Errorf("tls: received unexpected handshake message of type %T when waiting for %T", got, wanted)
 }
 
+var testingOnlySupportedSignatureAlgorithms []SignatureScheme
+
 // supportedSignatureAlgorithms returns the supported signature algorithms for
 // the given minimum TLS version, to advertise in ClientHello and
 // CertificateRequest messages.
 func supportedSignatureAlgorithms(minVers uint16) []SignatureScheme {
 	sigAlgs := defaultSupportedSignatureAlgorithms()
-	if fips140tls.Required() {
-		sigAlgs = slices.DeleteFunc(sigAlgs, func(s SignatureScheme) bool {
-			return !slices.Contains(allowedSignatureAlgorithmsFIPS, s)
-		})
+	if testingOnlySupportedSignatureAlgorithms != nil {
+		sigAlgs = slices.Clone(testingOnlySupportedSignatureAlgorithms)
 	}
-	if minVers > VersionTLS12 {
-		sigAlgs = slices.DeleteFunc(sigAlgs, func(s SignatureScheme) bool {
-			sigType, sigHash, _ := typeAndHashFromSignatureScheme(s)
-			return sigType == signaturePKCS1v15 || sigHash == crypto.SHA1
-		})
+	return slices.DeleteFunc(sigAlgs, func(s SignatureScheme) bool {
+		return isDisabledSignatureAlgorithm(minVers, s, false)
+	})
+}
+
+// var tlssha1 = godebug.New("tlssha1") [uTLS]
+
+func isDisabledSignatureAlgorithm(version uint16, s SignatureScheme, isCert bool) bool {
+	if fips140tls.Required() && !slices.Contains(allowedSignatureAlgorithmsFIPS, s) {
+		return true
+	}
+
+	// For the _cert extension we include all algorithms, including SHA-1 and
+	// PKCS#1 v1.5, because it's more likely that something on our side will be
+	// willing to accept a *-with-SHA1 certificate (e.g. with a custom
+	// VerifyConnection or by a direct match with the CertPool), than that the
+	// peer would have a better certificate but is just choosing not to send it.
+	// crypto/x509 will refuse to verify important SHA-1 signatures anyway.
+	if isCert {
+		return false
 	}
-	return sigAlgs
+
+	// TLS 1.3 removed support for PKCS#1 v1.5 and SHA-1 signatures,
+	// and Go 1.25 removed support for SHA-1 signatures in TLS 1.2.
+	if version > VersionTLS12 {
+		sigType, sigHash, _ := typeAndHashFromSignatureScheme(s)
+		if sigType == signaturePKCS1v15 || sigHash == crypto.SHA1 {
+			return true
+		}
+	}
+	// [uTLS] SECTION BEGIN
+	//} else if tlssha1.Value() != "1" {
+	//	_, sigHash, _ := typeAndHashFromSignatureScheme(s)
+	//	if sigHash == crypto.SHA1 {
+	//		return true
+	//	}
+	// [uTLS] SECTION END
+
+	return false
 }
 
 // supportedSignatureAlgorithmsCert returns the supported algorithms for
 // signatures in certificates.
 func supportedSignatureAlgorithmsCert() []SignatureScheme {
-	sigAlgs := defaultSupportedSignatureAlgorithmsCert()
-	if fips140tls.Required() {
-		sigAlgs = slices.DeleteFunc(sigAlgs, func(s SignatureScheme) bool {
-			return !slices.Contains(allowedSignatureAlgorithmsFIPS, s)
-		})
-	}
-	return sigAlgs
+	sigAlgs := defaultSupportedSignatureAlgorithms()
+	return slices.DeleteFunc(sigAlgs, func(s SignatureScheme) bool {
+		return isDisabledSignatureAlgorithm(0, s, true)
+	})
 }
 
 func isSupportedSignatureAlgorithm(sigAlg SignatureScheme, supportedSignatureAlgorithms []SignatureScheme) bool {

conn.go

@@ -50,6 +50,7 @@ type Conn struct {
 	didHRR           bool // whether a HelloRetryRequest was sent/received
 	cipherSuite      uint16
 	curveID          CurveID
+	peerSigAlg       SignatureScheme
 	ocspResponse     []byte   // stapled OCSP response
 	scts             [][]byte // signed certificate timestamps from server
 	peerCertificates []*x509.Certificate
@@ -1643,6 +1644,7 @@ func (c *Conn) connectionStateLocked() ConnectionState {
 	state.NegotiatedProtocol = c.clientProtocol
 	state.DidResume = c.didResume
 	state.testingOnlyDidHRR = c.didHRR
+	state.testingOnlyPeerSignatureAlgorithm = c.peerSigAlg
 	state.CurveID = c.curveID
 	state.NegotiatedProtocolIsMutual = true
 	state.ServerName = c.serverName

handshake_client.go

@@ -23,7 +23,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/refraction-networking/utls/internal/byteorder"
 	"github.com/refraction-networking/utls/internal/fips140tls"
 	"github.com/refraction-networking/utls/internal/hpke"
 	"github.com/refraction-networking/utls/internal/tls13"
@@ -43,8 +42,6 @@ type clientHandshakeState struct {
 	uconn *UConn // [uTLS]
 }
 
-var testingOnlyForceClientHelloSignatureAlgorithms []SignatureScheme
-
 func (c *Conn) makeClientHello() (*clientHelloMsg, *keySharePrivateKeys, *echClientContext, error) {
 	config := c.config
 
@@ -130,9 +127,6 @@ func (c *Conn) makeClientHello() (*clientHelloMsg, *keySharePrivateKeys, *echCli
 		hello.supportedSignatureAlgorithms = supportedSignatureAlgorithms(minVersion)
 		hello.supportedSignatureAlgorithmsCert = supportedSignatureAlgorithmsCert()
 	}
-	if testingOnlyForceClientHelloSignatureAlgorithms != nil {
-		hello.supportedSignatureAlgorithms = testingOnlyForceClientHelloSignatureAlgorithms
-	}
 
 	var keyShareKeys *keySharePrivateKeys
 	if maxVersion >= VersionTLS13 {
@@ -758,8 +752,9 @@ func (hs *clientHandshakeState) doFullHandshake() error {
 			c.sendAlert(alertIllegalParameter)
 			return err
 		}
-		if len(skx.key) >= 3 && skx.key[0] == 3 /* named curve */ {
-			c.curveID = CurveID(byteorder.BEUint16(skx.key[1:]))
+		if keyAgreement, ok := keyAgreement.(*ecdheKeyAgreement); ok {
+			c.curveID = keyAgreement.curveID
+			c.peerSigAlg = keyAgreement.signatureAlgorithm
 		}
 
 		msg, err = c.readHandshake(&hs.finishedHash)
@@ -841,7 +836,7 @@ func (hs *clientHandshakeState) doFullHandshake() error {
 		if c.vers >= VersionTLS12 {
 			signatureAlgorithm, err := selectSignatureScheme(c.vers, chainToSend, certReq.supportedSignatureAlgorithms)
 			if err != nil {
-				c.sendAlert(alertIllegalParameter)
+				c.sendAlert(alertHandshakeFailure)
 				return err
 			}
 			sigType, sigHash, err = typeAndHashFromSignatureScheme(signatureAlgorithm)

handshake_client_tls13.go

@@ -871,6 +871,7 @@ func (hs *clientHandshakeStateTLS13) readServerCertificate() error {
 		c.sendAlert(alertDecryptError)
 		return errors.New("tls: invalid signature by the server certificate: " + err.Error())
 	}
+	c.peerSigAlg = certVerify.signatureAlgorithm
 
 	if err := transcriptMsg(certVerify, hs.transcript); err != nil {
 		return err

handshake_server.go

@@ -17,8 +17,6 @@ import (
 	"hash"
 	"io"
 	"time"
-
-	"github.com/refraction-networking/utls/internal/byteorder"
 )
 
 // serverHandshakeState contains details of a server handshake in progress.
@@ -635,8 +633,9 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 		return err
 	}
 	if skx != nil {
-		if len(skx.key) >= 3 && skx.key[0] == 3 /* named curve */ {
-			c.curveID = CurveID(byteorder.BEUint16(skx.key[1:]))
+		if keyAgreement, ok := keyAgreement.(*ecdheKeyAgreement); ok {
+			c.curveID = keyAgreement.curveID
+			c.peerSigAlg = keyAgreement.signatureAlgorithm
 		}
 		if _, err := hs.c.writeHandshakeRecord(skx, &hs.finishedHash); err != nil {
 			return err
@@ -784,6 +783,7 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 			c.sendAlert(alertDecryptError)
 			return errors.New("tls: invalid signature by the client certificate: " + err.Error())
 		}
+		c.peerSigAlg = certVerify.signatureAlgorithm
 
 		if err := transcriptMsg(certVerify, &hs.finishedHash); err != nil {
 			return err

handshake_server_tls13.go

@@ -1121,6 +1121,7 @@ func (hs *serverHandshakeStateTLS13) readClientCertificate() error {
 			c.sendAlert(alertDecryptError)
 			return errors.New("tls: invalid signature by the client certificate: " + err.Error())
 		}
+		c.peerSigAlg = certVerify.signatureAlgorithm
 
 		if err := transcriptMsg(certVerify, hs.transcript); err != nil {
 			return err

key_agreement.go

@@ -165,25 +165,29 @@ type ecdheKeyAgreement struct {
 	// and returned in generateClientKeyExchange.
 	ckx             *clientKeyExchangeMsg
 	preMasterSecret []byte
+
+	// curveID and signatureAlgorithm are set by processServerKeyExchange and
+	// generateServerKeyExchange.
+	curveID            CurveID
+	signatureAlgorithm SignatureScheme
 }
 
 func (ka *ecdheKeyAgreement) generateServerKeyExchange(config *Config, cert *Certificate, clientHello *clientHelloMsg, hello *serverHelloMsg) (*serverKeyExchangeMsg, error) {
-	var curveID CurveID
 	for _, c := range clientHello.supportedCurves {
 		if config.supportsCurve(ka.version, c) {
-			curveID = c
+			ka.curveID = c
 			break
 		}
 	}
 
-	if curveID == 0 {
+	if ka.curveID == 0 {
 		return nil, errors.New("tls: no supported elliptic curves offered")
 	}
-	if _, ok := curveForCurveID(curveID); !ok {
+	if _, ok := curveForCurveID(ka.curveID); !ok {
 		return nil, errors.New("tls: CurvePreferences includes unsupported curve")
 	}
 
-	key, err := generateECDHEKey(config.rand(), curveID)
+	key, err := generateECDHEKey(config.rand(), ka.curveID)
 	if err != nil {
 		return nil, err
 	}
@@ -193,8 +197,8 @@ func (ka *ecdheKeyAgreement) generateServerKeyExchange(config *Config, cert *Cer
 	ecdhePublic := key.PublicKey().Bytes()
 	serverECDHEParams := make([]byte, 1+2+1+len(ecdhePublic))
 	serverECDHEParams[0] = 3 // named curve
-	serverECDHEParams[1] = byte(curveID >> 8)
-	serverECDHEParams[2] = byte(curveID)
+	serverECDHEParams[1] = byte(ka.curveID >> 8)
+	serverECDHEParams[2] = byte(ka.curveID)
 	serverECDHEParams[3] = byte(len(ecdhePublic))
 	copy(serverECDHEParams[4:], ecdhePublic)
 
@@ -203,15 +207,14 @@ func (ka *ecdheKeyAgreement) generateServerKeyExchange(config *Config, cert *Cer
 		return nil, fmt.Errorf("tls: certificate private key of type %T does not implement crypto.Signer", cert.PrivateKey)
 	}
 
-	var signatureAlgorithm SignatureScheme
 	var sigType uint8
 	var sigHash crypto.Hash
 	if ka.version >= VersionTLS12 {
-		signatureAlgorithm, err = selectSignatureScheme(ka.version, cert, clientHello.supportedSignatureAlgorithms)
+		ka.signatureAlgorithm, err = selectSignatureScheme(ka.version, cert, clientHello.supportedSignatureAlgorithms)
 		if err != nil {
 			return nil, err
 		}
-		sigType, sigHash, err = typeAndHashFromSignatureScheme(signatureAlgorithm)
+		sigType, sigHash, err = typeAndHashFromSignatureScheme(ka.signatureAlgorithm)
 		if err != nil {
 			return nil, err
 		}
@@ -245,8 +248,8 @@ func (ka *ecdheKeyAgreement) generateServerKeyExchange(config *Config, cert *Cer
 	copy(skx.key, serverECDHEParams)
 	k := skx.key[len(serverECDHEParams):]
 	if ka.version >= VersionTLS12 {
-		k[0] = byte(signatureAlgorithm >> 8)
-		k[1] = byte(signatureAlgorithm)
+		k[0] = byte(ka.signatureAlgorithm >> 8)
+		k[1] = byte(ka.signatureAlgorithm)
 		k = k[2:]
 	}
 	k[0] = byte(len(sig) >> 8)
@@ -280,7 +283,7 @@ func (ka *ecdheKeyAgreement) processServerKeyExchange(config *Config, clientHell
 	if skx.key[0] != 3 { // named curve
 		return errors.New("tls: server selected unsupported curve")
 	}
-	curveID := CurveID(skx.key[1])<<8 | CurveID(skx.key[2])
+	ka.curveID = CurveID(skx.key[1])<<8 | CurveID(skx.key[2])
 
 	publicLen := int(skx.key[3])
 	if publicLen+4 > len(skx.key) {
@@ -294,15 +297,15 @@ func (ka *ecdheKeyAgreement) processServerKeyExchange(config *Config, clientHell
 		return errServerKeyExchange
 	}
 
-	if !slices.Contains(clientHello.supportedCurves, curveID) {
+	if !slices.Contains(clientHello.supportedCurves, ka.curveID) {
 		return errors.New("tls: server selected unoffered curve")
 	}
 
-	if _, ok := curveForCurveID(curveID); !ok {
+	if _, ok := curveForCurveID(ka.curveID); !ok {
 		return errors.New("tls: server selected unsupported curve")
 	}
 
-	key, err := generateECDHEKey(config.rand(), curveID)
+	key, err := generateECDHEKey(config.rand(), ka.curveID)
 	if err != nil {
 		return err
 	}
@@ -326,16 +329,16 @@ func (ka *ecdheKeyAgreement) processServerKeyExchange(config *Config, clientHell
 	var sigType uint8
 	var sigHash crypto.Hash
 	if ka.version >= VersionTLS12 {
-		signatureAlgorithm := SignatureScheme(sig[0])<<8 | SignatureScheme(sig[1])
+		ka.signatureAlgorithm = SignatureScheme(sig[0])<<8 | SignatureScheme(sig[1])
 		sig = sig[2:]
 		if len(sig) < 2 {
 			return errServerKeyExchange
 		}
 
-		if !isSupportedSignatureAlgorithm(signatureAlgorithm, clientHello.supportedSignatureAlgorithms) {
+		if !isSupportedSignatureAlgorithm(ka.signatureAlgorithm, clientHello.supportedSignatureAlgorithms) {
 			return errors.New("tls: certificate used with invalid signature algorithm")
 		}
-		sigType, sigHash, err = typeAndHashFromSignatureScheme(signatureAlgorithm)
+		sigType, sigHash, err = typeAndHashFromSignatureScheme(ka.signatureAlgorithm)
 		if err != nil {
 			return err
 		}

u_handshake_client.go

@@ -262,9 +262,7 @@ func (c *Conn) makeClientHelloForApplyPreset() (*clientHelloMsg, *keySharePrivat
 
 	if maxVersion >= VersionTLS12 {
 		hello.supportedSignatureAlgorithms = supportedSignatureAlgorithms(minVersion)
-	}
-	if testingOnlyForceClientHelloSignatureAlgorithms != nil {
-		hello.supportedSignatureAlgorithms = testingOnlyForceClientHelloSignatureAlgorithms
+		hello.supportedSignatureAlgorithmsCert = supportedSignatureAlgorithmsCert()
 	}
 
 	var keyShareKeys *keySharePrivateKeys