From 855d5046c7899f19e2fad6ac83504a40cd92c6cc Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 21 Nov 2017 07:48:20 -0500 Subject: [PATCH] Unwind legacy SSL_PRIVATE_KEY_METHOD hooks. After much procrastinating, we finally moved Chromium to the new stuff. We can now delete this. This is a breaking change for SSL_PRIVATE_KEY_METHOD consumers, but it should be trivial (remove some unused fields in the struct). I've bumped BORINGSSL_API_VERSION to ease any multi-sided changes that may be needed. Change-Id: I9fe562590ad938bcb4fcf9af0fadeff1d48745fb Reviewed-on: https://boringssl-review.googlesource.com/23224 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Steven Valdez --- include/openssl/base.h | 2 +- include/openssl/ssl.h | 45 -------------------- ssl/ssl_privkey.cc | 90 +-------------------------------------- ssl/test/bssl_shim.cc | 24 ----------- ssl/test/runner/runner.go | 33 ++++---------- ssl/test/test_config.cc | 1 - ssl/test/test_config.h | 1 - 7 files changed, 12 insertions(+), 184 deletions(-) diff --git a/include/openssl/base.h b/include/openssl/base.h index adb5047a..94455560 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -151,7 +151,7 @@ extern "C" { // A consumer may use this symbol in the preprocessor to temporarily build // against multiple revisions of BoringSSL at the same time. It is not // recommended to do so for longer than is necessary. -#define BORINGSSL_API_VERSION 4 +#define BORINGSSL_API_VERSION 5 #if defined(BORINGSSL_SHARED_LIBRARY) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index eba2fa3a..53a8eb51 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1125,16 +1125,7 @@ enum ssl_private_key_result_t { // key hooks. This is used to off-load signing operations to a custom, // potentially asynchronous, backend. Metadata about the key such as the type // and size are parsed out of the certificate. -// -// TODO(davidben): This API has a number of legacy hooks. Remove the last -// consumer of |sign_digest| and trim it. struct ssl_private_key_method_st { - // type is ignored and should be NULL. - int (*type)(SSL *ssl); - - // max_signature_len is ignored and should be NULL. - size_t (*max_signature_len)(SSL *ssl); - // sign signs the message |in| in using the specified signature algorithm. On // success, it returns |ssl_private_key_success| and writes at most |max_out| // bytes of signature data to |out| and sets |*out_len| to the number of bytes @@ -1156,30 +1147,6 @@ struct ssl_private_key_method_st { uint16_t signature_algorithm, const uint8_t *in, size_t in_len); - // sign_digest signs |in_len| bytes of digest from |in|. |md| is the hash - // function used to calculate |in|. On success, it returns - // |ssl_private_key_success| and writes at most |max_out| bytes of signature - // data to |out|. On failure, it returns |ssl_private_key_failure|. If the - // operation has not completed, it returns |ssl_private_key_retry|. |sign| - // should arrange for the high-level operation on |ssl| to be retried when the - // operation is completed. This will result in a call to |complete|. - // - // If the key is an RSA key, implementations must use PKCS#1 padding. |in| is - // the digest itself, so the DigestInfo prefix, if any, must be prepended by - // |sign|. If |md| is |EVP_md5_sha1|, there is no prefix. - // - // It is an error to call |sign_digest| while another private key operation is - // in progress on |ssl|. - // - // This function is deprecated. Implement |sign| instead. - // - // TODO(davidben): Remove this function. - enum ssl_private_key_result_t (*sign_digest)(SSL *ssl, uint8_t *out, - size_t *out_len, size_t max_out, - const EVP_MD *md, - const uint8_t *in, - size_t in_len); - // decrypt decrypts |in_len| bytes of encrypted data from |in|. On success it // returns |ssl_private_key_success|, writes at most |max_out| bytes of // decrypted data to |out| and sets |*out_len| to the actual number of bytes @@ -3978,18 +3945,6 @@ OPENSSL_EXPORT int SSL_set_tmp_ecdh(SSL *ssl, const EC_KEY *ec_key); OPENSSL_EXPORT int SSL_add_dir_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, const char *dir); -// SSL_set_private_key_digest_prefs copies |num_digests| NIDs from |digest_nids| -// into |ssl|. These digests will be used, in decreasing order of preference, -// when signing with |ssl|'s private key. It returns one on success and zero on -// error. -// -// Use |SSL_set_signing_algorithm_prefs| instead. -// -// TODO(davidben): Remove this API when callers have been updated. -OPENSSL_EXPORT int SSL_set_private_key_digest_prefs(SSL *ssl, - const int *digest_nids, - size_t num_digests); - // SSL_set_verify_result calls |abort| unless |result| is |X509_V_OK|. // // TODO(davidben): Remove this function once it has been removed from diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc index e9990aff..134ad561 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -193,33 +193,6 @@ static int setup_ctx(SSL *ssl, EVP_MD_CTX *ctx, EVP_PKEY *pkey, uint16_t sigalg, return 1; } -static int legacy_sign_digest_supported(const SSL_SIGNATURE_ALGORITHM *alg) { - return (alg->pkey_type == EVP_PKEY_EC || alg->pkey_type == EVP_PKEY_RSA) && - !alg->is_rsa_pss; -} - -static enum ssl_private_key_result_t legacy_sign( - SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, uint16_t sigalg, - const uint8_t *in, size_t in_len) { - // TODO(davidben): Remove support for |sign_digest|-only - // |SSL_PRIVATE_KEY_METHOD|s. - const SSL_SIGNATURE_ALGORITHM *alg = get_signature_algorithm(sigalg); - if (alg == NULL || !legacy_sign_digest_supported(alg)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL_FOR_CUSTOM_KEY); - return ssl_private_key_failure; - } - - const EVP_MD *md = alg->digest_func(); - uint8_t hash[EVP_MAX_MD_SIZE]; - unsigned hash_len; - if (!EVP_Digest(in, in_len, hash, &hash_len, md, NULL)) { - return ssl_private_key_failure; - } - - return ssl->cert->key_method->sign_digest(ssl, out, out_len, max_out, md, - hash, hash_len); -} - enum ssl_private_key_result_t ssl_private_key_sign( SSL_HANDSHAKE *hs, uint8_t *out, size_t *out_len, size_t max_out, uint16_t sigalg, Span in) { @@ -229,9 +202,8 @@ enum ssl_private_key_result_t ssl_private_key_sign( if (hs->pending_private_key_op) { ret = ssl->cert->key_method->complete(ssl, out, out_len, max_out); } else { - ret = (ssl->cert->key_method->sign != NULL ? ssl->cert->key_method->sign - : legacy_sign)( - ssl, out, out_len, max_out, sigalg, in.data(), in.size()); + ret = ssl->cert->key_method->sign(ssl, out, out_len, max_out, sigalg, + in.data(), in.size()); } hs->pending_private_key_op = ret == ssl_private_key_retry; return ret; @@ -308,14 +280,6 @@ bool ssl_private_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, return false; } - // Newer algorithms require message-based private keys. - // TODO(davidben): Remove this check when sign_digest is gone. - if (ssl->cert->key_method != NULL && - ssl->cert->key_method->sign == NULL && - !legacy_sign_digest_supported(alg)) { - return false; - } - return true; } @@ -511,7 +475,6 @@ int SSL_CTX_set_signing_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs, prefs, num_prefs); } - int SSL_set_signing_algorithm_prefs(SSL *ssl, const uint16_t *prefs, size_t num_prefs) { return set_algorithm_prefs(&ssl->cert->sigalgs, &ssl->cert->num_sigalgs, @@ -523,52 +486,3 @@ int SSL_CTX_set_verify_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs, return set_algorithm_prefs(&ctx->verify_sigalgs, &ctx->num_verify_sigalgs, prefs, num_prefs); } - -int SSL_set_private_key_digest_prefs(SSL *ssl, const int *digest_nids, - size_t num_digests) { - OPENSSL_free(ssl->cert->sigalgs); - - static_assert(sizeof(int) >= 2 * sizeof(uint16_t), - "sigalgs allocation may overflow"); - - ssl->cert->num_sigalgs = 0; - ssl->cert->sigalgs = - (uint16_t *)OPENSSL_malloc(sizeof(uint16_t) * 2 * num_digests); - if (ssl->cert->sigalgs == NULL) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); - return 0; - } - - // Convert the digest list to a signature algorithms list. - // - // TODO(davidben): Replace this API with one that can express RSA-PSS, etc. - for (size_t i = 0; i < num_digests; i++) { - switch (digest_nids[i]) { - case NID_sha1: - ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA1; - ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] = SSL_SIGN_ECDSA_SHA1; - ssl->cert->num_sigalgs += 2; - break; - case NID_sha256: - ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA256; - ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] = - SSL_SIGN_ECDSA_SECP256R1_SHA256; - ssl->cert->num_sigalgs += 2; - break; - case NID_sha384: - ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA384; - ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] = - SSL_SIGN_ECDSA_SECP384R1_SHA384; - ssl->cert->num_sigalgs += 2; - break; - case NID_sha512: - ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA512; - ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] = - SSL_SIGN_ECDSA_SECP521R1_SHA512; - ssl->cert->num_sigalgs += 2; - break; - } - } - - return 1; -} diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 7eca21d2..8acabd72 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -433,10 +433,7 @@ static ssl_private_key_result_t AsyncPrivateKeyComplete( } static const SSL_PRIVATE_KEY_METHOD g_async_private_key_method = { - nullptr /* type */, - nullptr /* max_signature_len */, AsyncPrivateKeySign, - nullptr /* sign_digest */, AsyncPrivateKeyDecrypt, AsyncPrivateKeyComplete, }; @@ -453,27 +450,6 @@ static bool GetCertificate(SSL *ssl, bssl::UniquePtr *out_x509, bssl::UniquePtr *out_pkey) { const TestConfig *config = GetTestConfig(ssl); - if (!config->digest_prefs.empty()) { - bssl::UniquePtr digest_prefs( - OPENSSL_strdup(config->digest_prefs.c_str())); - std::vector digest_list; - - for (;;) { - char *token = - strtok(digest_list.empty() ? digest_prefs.get() : nullptr, ","); - if (token == nullptr) { - break; - } - - digest_list.push_back(EVP_MD_type(EVP_get_digestbyname(token))); - } - - if (!SSL_set_private_key_digest_prefs(ssl, digest_list.data(), - digest_list.size())) { - return false; - } - } - if (!config->signing_prefs.empty()) { std::vector u16s(config->signing_prefs.begin(), config->signing_prefs.end()); diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 7d02c152..8700af2c 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -8306,8 +8306,8 @@ func addSignatureAlgorithmTests() { expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:", }) - // Test that hash preferences are enforced. BoringSSL does not implement - // MD5 signatures. + // Test that signature preferences are enforced. BoringSSL does not + // implement MD5 signatures. testCases = append(testCases, testCase{ testType: serverTest, name: "ClientAuth-Enforced", @@ -8376,26 +8376,8 @@ func addSignatureAlgorithmTests() { expectedError: ":WRONG_SIGNATURE_TYPE:", }) - // Test that the agreed upon digest respects the client preferences and - // the server digests. - testCases = append(testCases, testCase{ - name: "NoCommonAlgorithms-Digests", - config: Config{ - MaxVersion: VersionTLS12, - ClientAuth: RequireAnyClientCert, - VerifySignatureAlgorithms: []signatureAlgorithm{ - signatureRSAPKCS1WithSHA512, - signatureRSAPKCS1WithSHA1, - }, - }, - flags: []string{ - "-cert-file", path.Join(*resourceDir, rsaCertificateFile), - "-key-file", path.Join(*resourceDir, rsaKeyFile), - "-digest-prefs", "SHA256", - }, - shouldFail: true, - expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:", - }) + // Test that the negotiated signature algorithm respects the client and + // server preferences. testCases = append(testCases, testCase{ name: "NoCommonAlgorithms", config: Config{ @@ -8445,7 +8427,8 @@ func addSignatureAlgorithmTests() { flags: []string{ "-cert-file", path.Join(*resourceDir, rsaCertificateFile), "-key-file", path.Join(*resourceDir, rsaKeyFile), - "-digest-prefs", "SHA256,SHA1", + "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)), + "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA1)), }, expectedPeerSignatureAlgorithm: signatureRSAPKCS1WithSHA256, }) @@ -8461,7 +8444,9 @@ func addSignatureAlgorithmTests() { flags: []string{ "-cert-file", path.Join(*resourceDir, rsaCertificateFile), "-key-file", path.Join(*resourceDir, rsaKeyFile), - "-digest-prefs", "SHA512,SHA256,SHA1", + "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA512)), + "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)), + "-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA1)), }, expectedPeerSignatureAlgorithm: signatureRSAPKCS1WithSHA1, }) diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 2d9f725e..a5ce5a12 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -132,7 +132,6 @@ const Flag kBoolFlags[] = { const Flag kStringFlags[] = { { "-write-settings", &TestConfig::write_settings }, - { "-digest-prefs", &TestConfig::digest_prefs }, { "-key-file", &TestConfig::key_file }, { "-cert-file", &TestConfig::cert_file }, { "-expect-server-name", &TestConfig::expected_server_name }, diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index b742f94a..ea12d344 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -26,7 +26,6 @@ struct TestConfig { int resume_count = 0; std::string write_settings; bool fallback_scsv = false; - std::string digest_prefs; std::vector signing_prefs; std::vector verify_prefs; std::string key_file;