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>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 863aff7..a126b46 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -4102,10 +4102,7 @@
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;
}
diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index 0843e0b..13a7d9a 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -151,6 +151,20 @@
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) {
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 07562df..3854283 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -211,6 +211,11 @@
// 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 @@
// 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.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 8c756a6..e3c58aa 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -10616,7 +10616,7 @@
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 @@
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 @@
"-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
diff --git a/ssl/test/runner/sign.go b/ssl/test/runner/sign.go
index d57cd60..da6452a 100644
--- a/ssl/test/runner/sign.go
+++ b/ssl/test/runner/sign.go
@@ -274,8 +274,8 @@
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 @@
}
// Fall through, forcing a particular algorithm.
- sigAlg = config.Bugs.UseLegacySigningAlgorithm
+ sigAlg = config.Bugs.SigningAlgorithmForLegacyVersions
}
switch sigAlg {
