From a232a7159c4e45c2ce8a1311188b6df250d1805f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 30 Mar 2017 15:51:53 -0500 Subject: [PATCH] Deprecate SSL_PRIVATE_KEY_METHOD type and max_signature_len. Instead, extract it from the certificate, which is what everyone was doing anyway. A follow-up change will take advantage of this cleanup to deduplicate code between signing and verifying for which keys are good for which signature algorithms. BUG=188 Change-Id: Ic3f83a6477e8fa53e5e7233f4545f4d2c4b58d01 Reviewed-on: https://boringssl-review.googlesource.com/14565 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 13 ++++++----- ssl/handshake_client.c | 4 ++-- ssl/handshake_server.c | 14 ++++++------ ssl/internal.h | 24 ++++++++++---------- ssl/s3_both.c | 1 + ssl/ssl_cert.c | 19 ++++++++++++++++ ssl/ssl_privkey.c | 51 +++++++++--------------------------------- ssl/t1_lib.c | 22 +++++++++--------- ssl/test/bssl_shim.cc | 21 ++--------------- ssl/tls13_both.c | 2 +- ssl/tls13_client.c | 2 +- 11 files changed, 73 insertions(+), 100 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 6a6cd858..4f02dda4 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1069,15 +1069,16 @@ enum ssl_private_key_result_t { /* ssl_private_key_method_st (aka |SSL_PRIVATE_KEY_METHOD|) describes private * key hooks. This is used to off-load signing operations to a custom, - * potentially asynchronous, backend. */ + * 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 returns the type of the key used by |ssl|. For RSA keys, return - * |NID_rsaEncryption|. For ECDSA keys, return |NID_X9_62_prime256v1|, - * |NID_secp384r1|, or |NID_secp521r1|, depending on the curve. */ + /* type is ignored and should be NULL. */ int (*type)(SSL *ssl); - /* max_signature_len returns the maximum length of a signature signed by the - * key used by |ssl|. This must be a constant value for a given |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 diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index 7eddd359..3b053c79 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -1527,7 +1527,7 @@ static int ssl3_send_client_certificate(SSL_HANDSHAKE *hs) { } } - if (!ssl->ctx->x509_method->ssl_auto_chain_if_needed(ssl) || + if (!ssl_on_certificate_selected(hs) || !ssl3_output_cert_chain(ssl)) { return -1; } @@ -1739,7 +1739,7 @@ static int ssl3_send_cert_verify(SSL_HANDSHAKE *hs) { } /* Set aside space for the signature. */ - const size_t max_sig_len = ssl_private_key_max_signature_len(ssl); + const size_t max_sig_len = EVP_PKEY_size(hs->local_pubkey); uint8_t *ptr; if (!CBB_add_u16_length_prefixed(&body, &child) || !CBB_reserve(&child, &ptr, max_sig_len)) { diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index cc1897fa..f8c97055 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c @@ -695,11 +695,11 @@ static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs, uint32_t mask_a = 0; if (ssl_has_certificate(ssl)) { - int type = ssl_private_key_type(ssl); - if (type == NID_rsaEncryption) { + int type = EVP_PKEY_id(hs->local_pubkey); + if (type == EVP_PKEY_RSA) { mask_k |= SSL_kRSA; mask_a |= SSL_aRSA; - } else if (ssl_is_ecdsa_key_type(type)) { + } else if (type == EVP_PKEY_EC) { mask_a |= SSL_aECDSA; } } @@ -879,7 +879,7 @@ static int ssl3_select_certificate(SSL_HANDSHAKE *hs) { } } - if (!ssl->ctx->x509_method->ssl_auto_chain_if_needed(ssl)) { + if (!ssl_on_certificate_selected(hs)) { return -1; } @@ -1241,7 +1241,7 @@ static int ssl3_send_server_key_exchange(SSL_HANDSHAKE *hs) { } /* Add space for the signature. */ - const size_t max_sig_len = ssl_private_key_max_signature_len(ssl); + const size_t max_sig_len = EVP_PKEY_size(hs->local_pubkey); uint8_t *ptr; if (!CBB_add_u16_length_prefixed(&body, &child) || !CBB_reserve(&child, &ptr, max_sig_len)) { @@ -1573,7 +1573,7 @@ static int ssl3_get_client_key_exchange(SSL_HANDSHAKE *hs) { * |premaster_secret_len|. */ if (alg_k & SSL_kRSA) { /* Allocate a buffer large enough for an RSA decryption. */ - const size_t rsa_size = ssl_private_key_max_signature_len(ssl); + const size_t rsa_size = EVP_PKEY_size(hs->local_pubkey); decrypt_buf = OPENSSL_malloc(rsa_size); if (decrypt_buf == NULL) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); @@ -1584,7 +1584,7 @@ static int ssl3_get_client_key_exchange(SSL_HANDSHAKE *hs) { size_t decrypt_len; if (hs->state == SSL3_ST_SR_KEY_EXCH_A) { if (!ssl_has_private_key(ssl) || - ssl_private_key_type(ssl) != NID_rsaEncryption) { + EVP_PKEY_id(hs->local_pubkey) != EVP_PKEY_RSA) { al = SSL_AD_HANDSHAKE_FAILURE; OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_RSA_CERTIFICATE); goto f_err; diff --git a/ssl/internal.h b/ssl/internal.h index a6375a67..3ebac721 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -542,22 +542,16 @@ enum ssl_open_record_t ssl_process_alert(SSL *ssl, uint8_t *out_alert, /* Private key operations. */ +typedef struct ssl_handshake_st SSL_HANDSHAKE; + /* ssl_has_private_key returns one if |ssl| has a private key * configured and zero otherwise. */ int ssl_has_private_key(const SSL *ssl); -/* ssl_is_ecdsa_key_type returns one if |type| is an ECDSA key type and zero - * otherwise. */ -int ssl_is_ecdsa_key_type(int type); - /* ssl_private_key_* call the corresponding function on the * |SSL_PRIVATE_KEY_METHOD| for |ssl|, if configured. Otherwise, they implement * the operation with |EVP_PKEY|. */ -int ssl_private_key_type(SSL *ssl); - -size_t ssl_private_key_max_signature_len(SSL *ssl); - enum ssl_private_key_result_t ssl_private_key_sign( SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, uint16_t signature_algorithm, const uint8_t *in, size_t in_len); @@ -570,9 +564,9 @@ enum ssl_private_key_result_t ssl_private_key_complete(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out); -/* ssl_private_key_supports_signature_algorithm returns one if |ssl|'s private +/* ssl_private_key_supports_signature_algorithm returns one if |hs|'s private * key supports |signature_algorithm| and zero otherwise. */ -int ssl_private_key_supports_signature_algorithm(SSL *ssl, +int ssl_private_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t signature_algorithm); /* ssl_public_key_verify verifies that the |signature| is valid for the public @@ -585,8 +579,6 @@ int ssl_public_key_verify( /* Custom extensions */ -typedef struct ssl_handshake_st SSL_HANDSHAKE; - /* ssl_custom_extension (a.k.a. SSL_CUSTOM_EXTENSION) is a structure that * contains information about custom-extension callbacks. */ struct ssl_custom_extension { @@ -857,6 +849,11 @@ int ssl_add_client_CA_list(SSL *ssl, CBB *cbb); int ssl_check_leaf_certificate(SSL_HANDSHAKE *hs, EVP_PKEY *pkey, const CRYPTO_BUFFER *leaf); +/* ssl_on_certificate_selected is called once the certificate has been selected. + * It finalizes the certificate and initializes |hs->local_pubkey|. It returns + * one on success and zero on error. */ +int ssl_on_certificate_selected(SSL_HANDSHAKE *hs); + /* TLS 1.3 key derivation. */ @@ -1061,6 +1058,9 @@ struct ssl_handshake_st { /* hostname, on the server, is the value of the SNI extension. */ char *hostname; + /* local_pubkey is the public key we are authenticating as. */ + EVP_PKEY *local_pubkey; + /* peer_pubkey is the public key parsed from the peer's leaf certificate. */ EVP_PKEY *peer_pubkey; diff --git a/ssl/s3_both.c b/ssl/s3_both.c index aff21542..e05a16e1 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -184,6 +184,7 @@ void ssl_handshake_free(SSL_HANDSHAKE *hs) { OPENSSL_free(hs->hostname); EVP_PKEY_free(hs->peer_pubkey); + EVP_PKEY_free(hs->local_pubkey); OPENSSL_free(hs); } diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index 4722a660..87b60e9d 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -861,6 +861,25 @@ int ssl_check_leaf_certificate(SSL_HANDSHAKE *hs, EVP_PKEY *pkey, return 1; } +int ssl_on_certificate_selected(SSL_HANDSHAKE *hs) { + SSL *const ssl = hs->ssl; + if (!ssl_has_certificate(ssl)) { + /* Nothing to do. */ + return 1; + } + + if (!ssl->ctx->x509_method->ssl_auto_chain_if_needed(ssl)) { + return 0; + } + + CBS leaf; + CRYPTO_BUFFER_init_CBS(sk_CRYPTO_BUFFER_value(ssl->cert->chain, 0), &leaf); + + EVP_PKEY_free(hs->local_pubkey); + hs->local_pubkey = ssl_cert_parse_pubkey(&leaf); + return hs->local_pubkey != NULL; +} + static int set_signed_cert_timestamp_list(CERT *cert, const uint8_t *list, size_t list_len) { CBS sct_list; diff --git a/ssl/ssl_privkey.c b/ssl/ssl_privkey.c index 40067590..82842d05 100644 --- a/ssl/ssl_privkey.c +++ b/ssl/ssl_privkey.c @@ -298,40 +298,6 @@ int ssl_has_private_key(const SSL *ssl) { return ssl->cert->privatekey != NULL || ssl->cert->key_method != NULL; } -int ssl_is_ecdsa_key_type(int type) { - switch (type) { - case NID_secp224r1: - case NID_X9_62_prime256v1: - case NID_secp384r1: - case NID_secp521r1: - return 1; - default: - return 0; - } -} - -int ssl_private_key_type(SSL *ssl) { - if (ssl->cert->key_method != NULL) { - return ssl->cert->key_method->type(ssl); - } - switch (EVP_PKEY_id(ssl->cert->privatekey)) { - case EVP_PKEY_RSA: - return NID_rsaEncryption; - case EVP_PKEY_EC: - return EC_GROUP_get_curve_name( - EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(ssl->cert->privatekey))); - default: - return NID_undef; - } -} - -size_t ssl_private_key_max_signature_len(SSL *ssl) { - if (ssl->cert->key_method != NULL) { - return ssl->cert->key_method->max_signature_len(ssl); - } - return EVP_PKEY_size(ssl->cert->privatekey); -} - static int is_rsa_pkcs1(const EVP_MD **out_md, uint16_t sigalg) { switch (sigalg) { case SSL_SIGN_RSA_PKCS1_MD5_SHA1: @@ -523,18 +489,19 @@ enum ssl_private_key_result_t ssl_private_key_complete(SSL *ssl, uint8_t *out, return ssl->cert->key_method->complete(ssl, out, out_len, max_out); } -int ssl_private_key_supports_signature_algorithm(SSL *ssl, +int ssl_private_key_supports_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t signature_algorithm) { + SSL *const ssl = hs->ssl; + int type = EVP_PKEY_id(hs->local_pubkey); const EVP_MD *md; if (is_rsa_pkcs1(&md, signature_algorithm) && ssl3_protocol_version(ssl) < TLS1_3_VERSION) { - return ssl_private_key_type(ssl) == NID_rsaEncryption; + return type == EVP_PKEY_RSA; } int curve; if (is_ecdsa(&curve, &md, signature_algorithm)) { - int type = ssl_private_key_type(ssl); - if (!ssl_is_ecdsa_key_type(type)) { + if (type != EVP_PKEY_EC) { return 0; } @@ -543,11 +510,13 @@ int ssl_private_key_supports_signature_algorithm(SSL *ssl, return 1; } - return curve != NID_undef && type == curve; + return curve != NID_undef && + EC_GROUP_get_curve_name(EC_KEY_get0_group( + EVP_PKEY_get0_EC_KEY(hs->local_pubkey))) == curve; } if (is_rsa_pss(&md, signature_algorithm)) { - if (ssl_private_key_type(ssl) != NID_rsaEncryption) { + if (type != EVP_PKEY_RSA) { return 0; } @@ -557,7 +526,7 @@ int ssl_private_key_supports_signature_algorithm(SSL *ssl, * defined RSASSA-PSS algorithm, but 1024-bit RSA is slightly too large for * SHA-512. 1024-bit RSA is sometimes used for test credentials, so check * the size to fall back to another algorithm. */ - if (ssl_private_key_max_signature_len(ssl) < 2 * EVP_MD_size(md) + 2) { + if ((size_t)EVP_PKEY_size(hs->local_pubkey) < 2 * EVP_MD_size(md) + 2) { return 0; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 2acd4229..e47937d6 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -3313,17 +3313,17 @@ int tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t *out) { /* Before TLS 1.2, the signature algorithm isn't negotiated as part of the * handshake. It is fixed at MD5-SHA1 for RSA and SHA1 for ECDSA. */ if (ssl3_protocol_version(ssl) < TLS1_2_VERSION) { - int type = ssl_private_key_type(ssl); - if (type == NID_rsaEncryption) { - *out = SSL_SIGN_RSA_PKCS1_MD5_SHA1; - return 1; - } - if (ssl_is_ecdsa_key_type(type)) { - *out = SSL_SIGN_ECDSA_SHA1; - return 1; + switch (EVP_PKEY_id(hs->local_pubkey)) { + case EVP_PKEY_RSA: + *out = SSL_SIGN_RSA_PKCS1_MD5_SHA1; + return 1; + case EVP_PKEY_EC: + *out = SSL_SIGN_ECDSA_SHA1; + return 1; + default: + OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMMON_SIGNATURE_ALGORITHMS); + return 0; } - OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMMON_SIGNATURE_ALGORITHMS); - return 0; } const uint16_t *sigalgs = cert->sigalgs; @@ -3350,7 +3350,7 @@ int tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs, uint16_t *out) { /* 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(ssl, sigalgs[i])) { + !ssl_private_key_supports_signature_algorithm(hs, sigalgs[i])) { continue; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index b60c8092..f7bcb0fd 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -286,23 +286,6 @@ static bssl::UniquePtr DecodeHexX509Names( return ret; } -static int AsyncPrivateKeyType(SSL *ssl) { - EVP_PKEY *key = GetTestState(ssl)->private_key.get(); - switch (EVP_PKEY_id(key)) { - case EVP_PKEY_RSA: - return NID_rsaEncryption; - case EVP_PKEY_EC: - return EC_GROUP_get_curve_name( - EC_KEY_get0_group(EVP_PKEY_get0_EC_KEY(key))); - default: - return NID_undef; - } -} - -static size_t AsyncPrivateKeyMaxSignatureLen(SSL *ssl) { - return EVP_PKEY_size(GetTestState(ssl)->private_key.get()); -} - static ssl_private_key_result_t AsyncPrivateKeySign( SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, uint16_t signature_algorithm, const uint8_t *in, size_t in_len) { @@ -436,8 +419,8 @@ static ssl_private_key_result_t AsyncPrivateKeyComplete( } static const SSL_PRIVATE_KEY_METHOD g_async_private_key_method = { - AsyncPrivateKeyType, - AsyncPrivateKeyMaxSignatureLen, + nullptr /* type */, + nullptr /* max_signature_len */, AsyncPrivateKeySign, nullptr /* sign_digest */, AsyncPrivateKeyDecrypt, diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c index ec67cdc1..be3c1b71 100644 --- a/ssl/tls13_both.c +++ b/ssl/tls13_both.c @@ -549,7 +549,7 @@ enum ssl_private_key_result_t tls13_add_certificate_verify(SSL_HANDSHAKE *hs, /* Sign the digest. */ CBB child; - const size_t max_sig_len = ssl_private_key_max_signature_len(ssl); + const size_t max_sig_len = EVP_PKEY_size(hs->local_pubkey); uint8_t *sig; size_t sig_len; if (!CBB_add_u16_length_prefixed(&body, &child) || diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c index 4c711e6c..6de51e5b 100644 --- a/ssl/tls13_client.c +++ b/ssl/tls13_client.c @@ -525,7 +525,7 @@ static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) { } } - if (!ssl->ctx->x509_method->ssl_auto_chain_if_needed(ssl) || + if (!ssl_on_certificate_selected(hs) || !tls13_add_certificate(hs)) { return ssl_hs_error; }