From 4a4e2b7bb3c9d7bde989cd3d812863370ecaf55c Mon Sep 17 00:00:00 2001 From: yuhan6665 <1588741+yuhan6665@users.noreply.github.com> Date: Sun, 20 Jul 2025 22:16:23 -0400 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Reviewed-by: David Chase Reviewed-by: Daniel McCarney Reviewed-by: Michael Knyszek --- auth.go | 23 ++++++-------- common.go | 65 +++++++++++++++++++++++++++++---------- conn.go | 2 ++ defaults.go | 26 ---------------- handshake_client.go | 13 +++----- handshake_client_tls13.go | 1 + handshake_server.go | 7 +++-- handshake_server_tls13.go | 1 + key_agreement.go | 41 ++++++++++++------------ 9 files changed, 92 insertions(+), 87 deletions(-) diff --git a/auth.go b/auth.go index c6421ce..37e2667 100644 --- a/auth.go +++ b/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) } } @@ -219,10 +217,9 @@ func signatureSchemesForCertificate(version uint16, cert *Certificate) []Signatu } // Filter out any unsupported signature algorithms, for example due to - // FIPS 140-3 policy, tlssha1=0, or any downstream changes to defaults.go. - supportedAlgs := supportedSignatureAlgorithms(version) + // FIPS 140-3 policy, tlssha1=0, or protocol version. sigAlgs = slices.DeleteFunc(sigAlgs, func(sigAlg SignatureScheme) bool { - return !isSupportedSignatureAlgorithm(sigAlg, supportedAlgs) + return isDisabledSignatureAlgorithm(version, sigAlg, false) }) return sigAlgs diff --git a/common.go b/common.go index 8b19ccf..b60c9f4 100644 --- a/common.go +++ b/common.go @@ -309,6 +309,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 @@ -1718,35 +1722,62 @@ 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") + +func isDisabledSignatureAlgorithm(version uint16, s SignatureScheme, isCert bool) bool { + if fips140tls.Required() && !slices.Contains(allowedSignatureAlgorithmsFIPS, s) { + return true } - return sigAlgs + + // 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 + } + + // 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 + } + } else { //if tlssha1.Value() != "1" { + _, sigHash, _ := typeAndHashFromSignatureScheme(s) + if sigHash == crypto.SHA1 { + return true + } + } + + 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 { diff --git a/conn.go b/conn.go index a408e76..ec1f1ec 100644 --- a/conn.go +++ b/conn.go @@ -55,6 +55,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 @@ -1691,6 +1692,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 diff --git a/defaults.go b/defaults.go index 617235b..508d92b 100644 --- a/defaults.go +++ b/defaults.go @@ -23,37 +23,11 @@ func defaultCurvePreferences() []CurveID { return []CurveID{X25519MLKEM768, X25519, CurveP256, CurveP384, CurveP521} } -//var tlssha1 = godebug.New("tlssha1") - // defaultSupportedSignatureAlgorithms returns the signature and hash algorithms that // the code advertises and supports in a TLS 1.2+ ClientHello and in a TLS 1.2+ // CertificateRequest. The two fields are merged to match with TLS 1.3. // Note that in TLS 1.2, the ECDSA algorithms are not constrained to P-256, etc. func defaultSupportedSignatureAlgorithms() []SignatureScheme { - return []SignatureScheme{ - PSSWithSHA256, - ECDSAWithP256AndSHA256, - Ed25519, - PSSWithSHA384, - PSSWithSHA512, - PKCS1WithSHA256, - PKCS1WithSHA384, - PKCS1WithSHA512, - ECDSAWithP384AndSHA384, - ECDSAWithP521AndSHA512, - } -} - -// defaultSupportedSignatureAlgorithmsCert returns the signature algorithms that -// the code advertises as supported for signatures in certificates. -// -// 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. -func defaultSupportedSignatureAlgorithmsCert() []SignatureScheme { return []SignatureScheme{ PSSWithSHA256, ECDSAWithP256AndSHA256, diff --git a/handshake_client.go b/handshake_client.go index 029a167..5ccc7b8 100644 --- a/handshake_client.go +++ b/handshake_client.go @@ -14,7 +14,6 @@ import ( "crypto/rsa" "crypto/subtle" "crypto/x509" - "encoding/binary" "errors" "fmt" "hash" @@ -41,8 +40,6 @@ type clientHandshakeState struct { ticket []byte // a fresh ticket received during this handshake } -var testingOnlyForceClientHelloSignatureAlgorithms []SignatureScheme - func (c *Conn) makeClientHello() (*clientHelloMsg, *keySharePrivateKeys, *echClientContext, error) { config := c.config if len(config.ServerName) == 0 && !config.InsecureSkipVerify { @@ -126,9 +123,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 { @@ -725,8 +719,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(binary.BigEndian.Uint16(skx.key[1:])) + if keyAgreement, ok := keyAgreement.(*ecdheKeyAgreement); ok { + c.curveID = keyAgreement.curveID + c.peerSigAlg = keyAgreement.signatureAlgorithm } msg, err = c.readHandshake(&hs.finishedHash) @@ -812,7 +807,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) diff --git a/handshake_client_tls13.go b/handshake_client_tls13.go index 186c6e7..3dc31dd 100644 --- a/handshake_client_tls13.go +++ b/handshake_client_tls13.go @@ -694,6 +694,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 diff --git a/handshake_server.go b/handshake_server.go index 5bf57bc..cb895da 100644 --- a/handshake_server.go +++ b/handshake_server.go @@ -12,7 +12,6 @@ import ( "crypto/rsa" "crypto/subtle" "crypto/x509" - "encoding/binary" "errors" "fmt" "hash" @@ -619,8 +618,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(binary.BigEndian.Uint16(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 @@ -772,6 +772,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 diff --git a/handshake_server_tls13.go b/handshake_server_tls13.go index 2f55176..b08bd3b 100644 --- a/handshake_server_tls13.go +++ b/handshake_server_tls13.go @@ -1213,6 +1213,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 diff --git a/key_agreement.go b/key_agreement.go index 118f259..b0b468d 100644 --- a/key_agreement.go +++ b/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 }