From 3a1b7306ac18110024f1b716c2aa61f17732e02a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 29 Nov 2022 18:44:46 -0500 Subject: [PATCH] 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 Reviewed-by: Bob Beck --- ssl/ssl_privkey.cc | 136 +++++++++++++++++--------- ssl/ssl_test.cc | 18 ++++ ssl/test/runner/handshake_messages.go | 6 ++ ssl/test/runner/runner.go | 36 ++++--- 4 files changed, 132 insertions(+), 64 deletions(-) diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index 13a7d9a7c..a9f92b61b 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -545,9 +545,83 @@ int SSL_is_signature_algorithm_rsa_pss(uint16_t sigalg) { 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 in_sigalgs) { + if (in_sigalgs.size() < 2) { + return true; + } + + Array 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 *out, Span prefs) { + if (!sigalgs_unique(prefs)) { + return false; + } + + // Check for invalid algorithms, and filter out |SSL_SIGN_RSA_PKCS1_MD5_SHA1|. + Array 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 @@ int SSL_set_signing_algorithm_prefs(SSL *ssl, const uint16_t *prefs, 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 @@ static bool parse_sigalg_pairs(Array *out, const int *values, 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 in_sigalgs) { - if (in_sigalgs.size() < 2) { - return true; - } - - Array 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 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 @@ int SSL_set1_sigalgs(SSL *ssl, const int *values, size_t num_values) { } Array 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 @@ static bool parse_sigalgs_list(Array *out, const char *str) { int SSL_CTX_set1_sigalgs_list(SSL_CTX *ctx, const char *str) { Array sigalgs; - if (!parse_sigalgs_list(&sigalgs, str) || - !sigalgs_unique(sigalgs)) { + if (!parse_sigalgs_list(&sigalgs, str)) { return 0; } @@ -859,8 +898,7 @@ int SSL_set1_sigalgs_list(SSL *ssl, const char *str) { } Array sigalgs; - if (!parse_sigalgs_list(&sigalgs, str) || - !sigalgs_unique(sigalgs)) { + if (!parse_sigalgs_list(&sigalgs, str)) { return 0; } @@ -874,7 +912,8 @@ int SSL_set1_sigalgs_list(SSL *ssl, const char *str) { 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 @@ int SSL_set_verify_algorithm_prefs(SSL *ssl, const uint16_t *prefs, 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 51366a4fd..8e2e5e74d 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -8498,5 +8498,23 @@ TEST(SSLTest, QuietShutdown) { EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_ZERO_RETURN); } +TEST(SSLTest, InvalidSignatureAlgorithm) { + bssl::UniquePtr 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 fbfbdd956..b253b0c84 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -841,6 +841,12 @@ func parseSignatureAlgorithms(reader *byteReader, out *[]signatureAlgorithm, all 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 e3c58aa20..b66ae525d 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -9852,7 +9852,6 @@ func addSignatureAlgorithmTests() { []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 @@ func addSignatureAlgorithmTests() { 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 @@ func addSignatureAlgorithmTests() { 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 @@ func addSignatureAlgorithmTests() { 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 @@ func addSignatureAlgorithmTests() { 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 @@ func addSignatureAlgorithmTests() { 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 @@ func addSignatureAlgorithmTests() { "-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 @@ func addSignatureAlgorithmTests() { "-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,