Don't allow the caller to configure invalid signature algorithms.
It should not be possible to make BoringSSL request unknown signature
algorithms, or the special SSL_SIGN_RSA_PKCS1_MD5_SHA1 value, in the
ClientHello or CertificateRequest.
Update-Note: This CL makes unknown values fail
SSL_set_verify_algorithm_prefs, etc. SSL_SIGN_RSA_PKCS1_MD5_SHA1 is
silently dropped from the list, rather than an error because, although
documented as incorrect, this hole in the abstraction seems to be
confusing. I think there's some code in Chromium which accidentally puts
it in the signing prefs (wrong but harmless) and I often need to explain
to folks that it doesn't belowing in verify prefs (puts it in the
ClientHello). This makes us tolerate the value by ignoring it.
This makes the previous pkey_supports_algorithm change moot because we'd
never get that far with SSL_SIGN_RSA_PKCS1_MD5_SHA1, but I think the
check, but I think the check belongs in that function too.
The test also reveals that some of our tests have been accidentally
passing zero into the preference list all this time.
Change-Id: I76d4eb98682515c3b819e0ed8d44f2d708a98975
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55446
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index 13a7d9a..a9f92b6 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -545,9 +545,83 @@
return alg != nullptr && alg->is_rsa_pss;
}
+static int compare_uint16_t(const void *p1, const void *p2) {
+ uint16_t u1 = *((const uint16_t *)p1);
+ uint16_t u2 = *((const uint16_t *)p2);
+ if (u1 < u2) {
+ return -1;
+ } else if (u1 > u2) {
+ return 1;
+ } else {
+ return 0;
+ }
+}
+
+static bool sigalgs_unique(Span<const uint16_t> in_sigalgs) {
+ if (in_sigalgs.size() < 2) {
+ return true;
+ }
+
+ Array<uint16_t> sigalgs;
+ if (!sigalgs.CopyFrom(in_sigalgs)) {
+ return false;
+ }
+
+ qsort(sigalgs.data(), sigalgs.size(), sizeof(uint16_t), compare_uint16_t);
+
+ for (size_t i = 1; i < sigalgs.size(); i++) {
+ if (sigalgs[i - 1] == sigalgs[i]) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_SIGNATURE_ALGORITHM);
+ return false;
+ }
+ }
+
+ return true;
+}
+
+static bool set_sigalg_prefs(Array<uint16_t> *out, Span<const uint16_t> prefs) {
+ if (!sigalgs_unique(prefs)) {
+ return false;
+ }
+
+ // Check for invalid algorithms, and filter out |SSL_SIGN_RSA_PKCS1_MD5_SHA1|.
+ Array<uint16_t> filtered;
+ if (!filtered.Init(prefs.size())) {
+ return false;
+ }
+ size_t added = 0;
+ for (uint16_t pref : prefs) {
+ if (pref == SSL_SIGN_RSA_PKCS1_MD5_SHA1) {
+ // Though not intended to be used with this API, we treat
+ // |SSL_SIGN_RSA_PKCS1_MD5_SHA1| as a real signature algorithm in
+ // |SSL_PRIVATE_KEY_METHOD|. Not accepting it here makes for a confusing
+ // abstraction.
+ continue;
+ }
+ if (get_signature_algorithm(pref) == nullptr) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SIGNATURE_ALGORITHM);
+ return false;
+ }
+ filtered[added] = pref;
+ added++;
+ }
+ filtered.Shrink(added);
+
+ // This can happen if |prefs| contained only |SSL_SIGN_RSA_PKCS1_MD5_SHA1|.
+ // Leaving it empty would revert to the default, so treat this as an error
+ // condition.
+ if (!prefs.empty() && filtered.empty()) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SIGNATURE_ALGORITHM);
+ return false;
+ }
+
+ *out = std::move(filtered);
+ return true;
+}
+
int SSL_CTX_set_signing_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs,
size_t num_prefs) {
- return ctx->cert->sigalgs.CopyFrom(MakeConstSpan(prefs, num_prefs));
+ return set_sigalg_prefs(&ctx->cert->sigalgs, MakeConstSpan(prefs, num_prefs));
}
int SSL_set_signing_algorithm_prefs(SSL *ssl, const uint16_t *prefs,
@@ -555,7 +629,8 @@
if (!ssl->config) {
return 0;
}
- return ssl->config->cert->sigalgs.CopyFrom(MakeConstSpan(prefs, num_prefs));
+ return set_sigalg_prefs(&ssl->config->cert->sigalgs,
+ MakeConstSpan(prefs, num_prefs));
}
static constexpr struct {
@@ -611,50 +686,16 @@
return true;
}
-static int compare_uint16_t(const void *p1, const void *p2) {
- uint16_t u1 = *((const uint16_t *)p1);
- uint16_t u2 = *((const uint16_t *)p2);
- if (u1 < u2) {
- return -1;
- } else if (u1 > u2) {
- return 1;
- } else {
- return 0;
- }
-}
-
-static bool sigalgs_unique(Span<const uint16_t> in_sigalgs) {
- if (in_sigalgs.size() < 2) {
- return true;
- }
-
- Array<uint16_t> sigalgs;
- if (!sigalgs.CopyFrom(in_sigalgs)) {
- return false;
- }
-
- qsort(sigalgs.data(), sigalgs.size(), sizeof(uint16_t), compare_uint16_t);
-
- for (size_t i = 1; i < sigalgs.size(); i++) {
- if (sigalgs[i - 1] == sigalgs[i]) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DUPLICATE_SIGNATURE_ALGORITHM);
- return false;
- }
- }
-
- return true;
-}
-
int SSL_CTX_set1_sigalgs(SSL_CTX *ctx, const int *values, size_t num_values) {
Array<uint16_t> sigalgs;
- if (!parse_sigalg_pairs(&sigalgs, values, num_values) ||
- !sigalgs_unique(sigalgs)) {
+ if (!parse_sigalg_pairs(&sigalgs, values, num_values)) {
return 0;
}
if (!SSL_CTX_set_signing_algorithm_prefs(ctx, sigalgs.data(),
sigalgs.size()) ||
- !ctx->verify_sigalgs.CopyFrom(sigalgs)) {
+ !SSL_CTX_set_verify_algorithm_prefs(ctx, sigalgs.data(),
+ sigalgs.size())) {
return 0;
}
@@ -668,13 +709,12 @@
}
Array<uint16_t> sigalgs;
- if (!parse_sigalg_pairs(&sigalgs, values, num_values) ||
- !sigalgs_unique(sigalgs)) {
+ if (!parse_sigalg_pairs(&sigalgs, values, num_values)) {
return 0;
}
if (!SSL_set_signing_algorithm_prefs(ssl, sigalgs.data(), sigalgs.size()) ||
- !ssl->config->verify_sigalgs.CopyFrom(sigalgs)) {
+ !SSL_set_verify_algorithm_prefs(ssl, sigalgs.data(), sigalgs.size())) {
return 0;
}
@@ -837,8 +877,7 @@
int SSL_CTX_set1_sigalgs_list(SSL_CTX *ctx, const char *str) {
Array<uint16_t> sigalgs;
- if (!parse_sigalgs_list(&sigalgs, str) ||
- !sigalgs_unique(sigalgs)) {
+ if (!parse_sigalgs_list(&sigalgs, str)) {
return 0;
}
@@ -859,8 +898,7 @@
}
Array<uint16_t> sigalgs;
- if (!parse_sigalgs_list(&sigalgs, str) ||
- !sigalgs_unique(sigalgs)) {
+ if (!parse_sigalgs_list(&sigalgs, str)) {
return 0;
}
@@ -874,7 +912,8 @@
int SSL_CTX_set_verify_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs,
size_t num_prefs) {
- return ctx->verify_sigalgs.CopyFrom(MakeConstSpan(prefs, num_prefs));
+ return set_sigalg_prefs(&ctx->verify_sigalgs,
+ MakeConstSpan(prefs, num_prefs));
}
int SSL_set_verify_algorithm_prefs(SSL *ssl, const uint16_t *prefs,
@@ -884,5 +923,6 @@
return 0;
}
- return ssl->config->verify_sigalgs.CopyFrom(MakeConstSpan(prefs, num_prefs));
+ return set_sigalg_prefs(&ssl->config->verify_sigalgs,
+ MakeConstSpan(prefs, num_prefs));
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 51366a4..8e2e5e7 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -8498,5 +8498,23 @@
EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_ZERO_RETURN);
}
+TEST(SSLTest, InvalidSignatureAlgorithm) {
+ bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(ctx);
+
+ static const uint16_t kInvalidPrefs[] = {1234};
+ EXPECT_FALSE(SSL_CTX_set_signing_algorithm_prefs(
+ ctx.get(), kInvalidPrefs, OPENSSL_ARRAY_SIZE(kInvalidPrefs)));
+ EXPECT_FALSE(SSL_CTX_set_verify_algorithm_prefs(
+ ctx.get(), kInvalidPrefs, OPENSSL_ARRAY_SIZE(kInvalidPrefs)));
+
+ static const uint16_t kDuplicatePrefs[] = {SSL_SIGN_RSA_PKCS1_SHA256,
+ SSL_SIGN_RSA_PKCS1_SHA256};
+ EXPECT_FALSE(SSL_CTX_set_signing_algorithm_prefs(
+ ctx.get(), kDuplicatePrefs, OPENSSL_ARRAY_SIZE(kDuplicatePrefs)));
+ EXPECT_FALSE(SSL_CTX_set_verify_algorithm_prefs(
+ ctx.get(), kDuplicatePrefs, OPENSSL_ARRAY_SIZE(kDuplicatePrefs)));
+}
+
} // namespace
BSSL_NAMESPACE_END
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index fbfbdd9..b253b0c 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -841,6 +841,12 @@
if !sigAlgs.readU16(&v) {
return false
}
+ if signatureAlgorithm(v) == signatureRSAPKCS1WithMD5AndSHA1 {
+ // signatureRSAPKCS1WithMD5AndSHA1 is an internal value BoringSSL
+ // uses to represent the TLS 1.0 MD5/SHA-1 concatenation. It should
+ // never appear on the wire.
+ return false
+ }
*out = append(*out, signatureAlgorithm(v))
}
return true
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index e3c58aa..b66ae52 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -9852,7 +9852,6 @@
[]string{
"-cert-file", path.Join(*resourceDir, getShimCertificate(alg.cert)),
"-key-file", path.Join(*resourceDir, getShimKey(alg.cert)),
- "-signing-prefs", strconv.Itoa(int(alg.id)),
},
flagInts("-curves", shimConfig.AllCurves)...,
),
@@ -9860,6 +9859,9 @@
peerSignatureAlgorithm: alg.id,
},
}
+ if alg.id != 0 {
+ negotiateTest.flags = append(negotiateTest.flags, "-signing-prefs", strconv.Itoa(int(alg.id)))
+ }
if testType == serverTest {
// TLS 1.2 servers only sign on some cipher suites.
@@ -9892,14 +9894,7 @@
IgnorePeerSignatureAlgorithmPreferences: shouldFail,
},
},
- flags: append(
- []string{
- "-expect-peer-signature-algorithm", strconv.Itoa(int(alg.id)),
- // The algorithm may be disabled by default, so explicitly enable it.
- "-verify-prefs", strconv.Itoa(int(alg.id)),
- },
- flagInts("-curves", shimConfig.AllCurves)...,
- ),
+ flags: flagInts("-curves", shimConfig.AllCurves),
// Resume the session to assert the peer signature
// algorithm is reported on both handshakes.
resumeSession: !shouldFail,
@@ -9907,6 +9902,11 @@
expectedError: verifyError,
expectedLocalError: verifyLocalError,
}
+ if alg.id != 0 {
+ verifyTest.flags = append(verifyTest.flags, "-expect-peer-signature-algorithm", strconv.Itoa(int(alg.id)))
+ // The algorithm may be disabled by default, so explicitly enable it.
+ verifyTest.flags = append(verifyTest.flags, "-verify-prefs", strconv.Itoa(int(alg.id)))
+ }
// Test whether the shim expects the algorithm enabled by default.
defaultTest := testCase{
@@ -9951,14 +9951,14 @@
InvalidSignature: true,
},
},
- flags: append(
- // The algorithm may be disabled by default, so explicitly enable it.
- []string{"-verify-prefs", strconv.Itoa(int(alg.id))},
- flagInts("-curves", shimConfig.AllCurves)...,
- ),
+ flags: flagInts("-curves", shimConfig.AllCurves),
shouldFail: true,
expectedError: ":BAD_SIGNATURE:",
}
+ if alg.id != 0 {
+ // The algorithm may be disabled by default, so explicitly enable it.
+ invalidTest.flags = append(invalidTest.flags, "-verify-prefs", strconv.Itoa(int(alg.id)))
+ }
if testType == serverTest {
// TLS 1.2 servers only verify when they request client certificates.
@@ -10477,10 +10477,8 @@
flags: []string{
"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
"-key-file", path.Join(*resourceDir, rsaKeyFile),
- "-signing-prefs", strconv.Itoa(int(fakeSigAlg1)),
"-signing-prefs", strconv.Itoa(int(signatureECDSAWithP256AndSHA256)),
"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)),
- "-signing-prefs", strconv.Itoa(int(fakeSigAlg2)),
},
expectations: connectionExpectations{
peerSignatureAlgorithm: signatureRSAPKCS1WithSHA256,
@@ -10791,6 +10789,9 @@
"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
"-key-file", path.Join(*resourceDir, rsaKeyFile),
"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithMD5AndSHA1)),
+ // Include a valid algorithm as well, to avoid an empty list
+ // if filtered out.
+ "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)),
},
shouldFail: true,
expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:",
@@ -10814,6 +10815,9 @@
"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
"-key-file", path.Join(*resourceDir, rsaKeyFile),
"-verify-prefs", strconv.Itoa(int(signatureRSAPKCS1WithMD5AndSHA1)),
+ // Include a valid algorithm as well, to avoid an empty list
+ // if filtered out.
+ "-verify-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)),
"-require-any-client-certificate",
},
shouldFail: true,
