Never accidentally use SSL_SIGN_RSA_PKCS1_MD5_SHA1 at TLS 1.2.

SSL_SIGN_RSA_PKCS1_MD5_SHA1 does not really exist, but is a private use
value we allocated to internally represent the TLS 1.0/1.1 RSA signature
algorithm. (Unlike the TLS 1.0/1.1 ECDSA signature algorithm, which is
the same as SSL_SIGN_ECDSA_SHA1, the RSA one is a bespoke MD5+SHA1
concatenation which never appears in TLS 1.2 and up.)

Although documented that you're not to use it with
SSL_CTX_set_verify_algorithm_prefs and
SSL_CTX_set_signing_algorithm_prefs (it only exists for
SSL_PRIVATE_KEY_METHOD), there's nothing stopping a caller from passing
it in.

Were you to do so anyway, we'd get confused and sign or verify it at TLS
1.2. This CL is the first half of a fix: since we already have
pkey_supports_algorithm that checks a (version, sigalg, key) tuple, that
function should just know this is not a 1.2-compatible algorithm.

A subsequent CL will also fix those APIs to not accept invalid values
from the caller, since these invalid calls will still, e.g., dump
garbage values on the wire.

Change-Id: I119503f9742a17952ed08e5815fb3d1419fd4a12
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55445
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

ssl/extensions.cc

@@ -4102,10 +4102,7 @@ bool tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t *out) {
   Span<const uint16_t> peer_sigalgs = tls1_get_peer_verify_algorithms(hs);
 
   for (uint16_t sigalg : sigalgs) {
-    // SSL_SIGN_RSA_PKCS1_MD5_SHA1 is an internal value and should never be
-    // negotiated.
-    if (sigalg == SSL_SIGN_RSA_PKCS1_MD5_SHA1 ||
-        !ssl_private_key_supports_signature_algorithm(hs, sigalg)) {
+    if (!ssl_private_key_supports_signature_algorithm(hs, sigalg)) {
       continue;
     }
 

ssl/ssl_privkey.cc

@@ -151,6 +151,20 @@ static bool pkey_supports_algorithm(const SSL *ssl, EVP_PKEY *pkey,
     return false;
   }
 
+  if (ssl_protocol_version(ssl) < TLS1_2_VERSION) {
+    // TLS 1.0 and 1.1 do not negotiate algorithms and always sign one of two
+    // hardcoded algorithms.
+    return sigalg == SSL_SIGN_RSA_PKCS1_MD5_SHA1 ||
+           sigalg == SSL_SIGN_ECDSA_SHA1;
+  }
+
+  // |SSL_SIGN_RSA_PKCS1_MD5_SHA1| is not a real SignatureScheme for TLS 1.2 and
+  // higher. It is an internal value we use to represent TLS 1.0/1.1's MD5/SHA1
+  // concatenation.
+  if (sigalg == SSL_SIGN_RSA_PKCS1_MD5_SHA1) {
+    return false;
+  }
+
   if (ssl_protocol_version(ssl) >= TLS1_3_VERSION) {
     // RSA keys may only be used with RSA-PSS.
     if (alg->pkey_type == EVP_PKEY_RSA && !alg->is_rsa_pss) {

ssl/test/runner/common.go

@@ -211,6 +211,11 @@ const (
 	// EdDSA algorithms
 	signatureEd25519 signatureAlgorithm = 0x0807
 	signatureEd448   signatureAlgorithm = 0x0808
+
+	// signatureRSAPKCS1WithMD5AndSHA1 is the internal value BoringSSL uses to
+	// represent the TLS 1.0/1.1 RSA MD5/SHA1 concatenation. We define the
+	// constant here to test that this doesn't leak into the protocol.
+	signatureRSAPKCS1WithMD5AndSHA1 signatureAlgorithm = 0xff01
 )
 
 // supportedSignatureAlgorithms contains the default supported signature
@@ -1792,10 +1797,15 @@ type ProtocolBugs struct {
 	// reject CertificateRequest with the CertificateAuthorities extension.
 	ExpectNoCertificateAuthoritiesExtension bool
 
-	// UseLegacySigningAlgorithm, if non-zero, is the signature algorithm
+	// SigningAlgorithmForLegacyVersions, if non-zero, is the signature algorithm
 	// to use when signing in TLS 1.1 and earlier where algorithms are not
 	// negotiated.
-	UseLegacySigningAlgorithm signatureAlgorithm
+	SigningAlgorithmForLegacyVersions signatureAlgorithm
+
+	// AlwaysSignAsLegacyVersion, if true, causes all TLS versions to sign as if
+	// they were TLS 1.1 and earlier. This can be paired with
+	// SendSignatureAlgorithm to send a given signature algorithm enum.
+	AlwaysSignAsLegacyVersion bool
 
 	// RejectUnsolicitedKeyUpdate, if true, causes all unsolicited
 	// KeyUpdates from the peer to be rejected.

ssl/test/runner/runner.go

@@ -10616,7 +10616,7 @@ func addSignatureAlgorithmTests() {
 			Certificates: []Certificate{ed25519Certificate},
 			Bugs: ProtocolBugs{
 				// Sign with Ed25519 even though it is TLS 1.1.
-				UseLegacySigningAlgorithm: signatureEd25519,
+				SigningAlgorithmForLegacyVersions: signatureEd25519,
 			},
 		},
 		flags:         []string{"-verify-prefs", strconv.Itoa(int(signatureEd25519))},
@@ -10644,7 +10644,7 @@ func addSignatureAlgorithmTests() {
 			Certificates: []Certificate{ed25519Certificate},
 			Bugs: ProtocolBugs{
 				// Sign with Ed25519 even though it is TLS 1.1.
-				UseLegacySigningAlgorithm: signatureEd25519,
+				SigningAlgorithmForLegacyVersions: signatureEd25519,
 			},
 		},
 		flags: []string{
@@ -10764,6 +10764,63 @@ func addSignatureAlgorithmTests() {
 			"-verify-prefs", strconv.Itoa(int(signatureEd25519)),
 		},
 	})
+
+	for _, testType := range []testType{clientTest, serverTest} {
+		for _, ver := range tlsVersions {
+			if ver.version < VersionTLS12 {
+				continue
+			}
+
+			prefix := "Client-" + ver.name + "-"
+			if testType == serverTest {
+				prefix = "Server-" + ver.name + "-"
+			}
+
+			// Test that the shim will not sign MD5/SHA1 with RSA at TLS 1.2,
+			// even if specified in signing preferences.
+			testCases = append(testCases, testCase{
+				testType: testType,
+				name:     prefix + "NoSign-RSA_PKCS1_MD5_SHA1",
+				config: Config{
+					MaxVersion:                ver.version,
+					CipherSuites:              signingCiphers,
+					ClientAuth:                RequireAnyClientCert,
+					VerifySignatureAlgorithms: []signatureAlgorithm{signatureRSAPKCS1WithMD5AndSHA1},
+				},
+				flags: []string{
+					"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
+					"-key-file", path.Join(*resourceDir, rsaKeyFile),
+					"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithMD5AndSHA1)),
+				},
+				shouldFail:    true,
+				expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:",
+			})
+
+			// Test that the shim will not accept MD5/SHA1 with RSA at TLS 1.2,
+			// even if specified in verify preferences.
+			testCases = append(testCases, testCase{
+				testType: testType,
+				name:     prefix + "NoVerify-RSA_PKCS1_MD5_SHA1",
+				config: Config{
+					MaxVersion:   ver.version,
+					Certificates: []Certificate{rsaCertificate},
+					Bugs: ProtocolBugs{
+						IgnorePeerSignatureAlgorithmPreferences: true,
+						AlwaysSignAsLegacyVersion:               true,
+						SendSignatureAlgorithm:                  signatureRSAPKCS1WithMD5AndSHA1,
+					},
+				},
+				flags: []string{
+					"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
+					"-key-file", path.Join(*resourceDir, rsaKeyFile),
+					"-verify-prefs", strconv.Itoa(int(signatureRSAPKCS1WithMD5AndSHA1)),
+					"-require-any-client-certificate",
+				},
+				shouldFail:    true,
+				expectedError: ":WRONG_SIGNATURE_TYPE:",
+			})
+		}
+	}
 }
 
 // timeouts is the retransmit schedule for BoringSSL. It doubles and

ssl/test/runner/sign.go

@@ -274,8 +274,8 @@ func (e *ed25519Signer) verifyMessage(key crypto.PublicKey, msg, sig []byte) err
 
 func getSigner(version uint16, key interface{}, config *Config, sigAlg signatureAlgorithm, isVerify bool) (signer, error) {
 	// TLS 1.1 and below use legacy signature algorithms.
-	if version < VersionTLS12 {
-		if config.Bugs.UseLegacySigningAlgorithm == 0 || isVerify {
+	if version < VersionTLS12 || (!isVerify && config.Bugs.AlwaysSignAsLegacyVersion) {
+		if config.Bugs.SigningAlgorithmForLegacyVersions == 0 || isVerify {
 			switch key.(type) {
 			case *rsa.PrivateKey, *rsa.PublicKey:
 				return &rsaPKCS1Signer{crypto.MD5SHA1}, nil
@@ -287,7 +287,7 @@ func getSigner(version uint16, key interface{}, config *Config, sigAlg signature
 		}
 
 		// Fall through, forcing a particular algorithm.
-		sigAlg = config.Bugs.UseLegacySigningAlgorithm
+		sigAlg = config.Bugs.SigningAlgorithmForLegacyVersions
 	}
 
 	switch sigAlg {