From 48b276db3d2f05ccbcd47c08bf2f99d55d257515 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 12 Apr 2018 17:37:32 -0400 Subject: [PATCH] Give ssl_cipher_preference_list_st a destructor. Change-Id: I578a284c6a8cae773a97d3d30ad8a5cd13f56164 Reviewed-on: https://boringssl-review.googlesource.com/27491 Commit-Queue: Steven Valdez CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Steven Valdez --- ssl/handshake_server.cc | 14 ++--- ssl/internal.h | 103 ++++++++++++++++---------------- ssl/s3_lib.cc | 3 +- ssl/ssl_cipher.cc | 129 ++++++++++++++++++++-------------------- ssl/ssl_lib.cc | 27 ++------- 5 files changed, 129 insertions(+), 147 deletions(-) diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 84004de5..6404cc9e 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -332,14 +332,14 @@ static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs, static const SSL_CIPHER *ssl3_choose_cipher( SSL_HANDSHAKE *hs, const SSL_CLIENT_HELLO *client_hello, - const struct ssl_cipher_preference_list_st *server_pref) { + const SSLCipherPreferenceList *server_pref) { SSL *const ssl = hs->ssl; const STACK_OF(SSL_CIPHER) *prio, *allow; // in_group_flags will either be NULL, or will point to an array of bytes // which indicate equal-preference groups in the |prio| stack. See the - // comment about |in_group_flags| in the |ssl_cipher_preference_list_st| + // comment about |in_group_flags| in the |SSLCipherPreferenceList| // struct. - const uint8_t *in_group_flags; + const bool *in_group_flags; // group_min contains the minimal index so far found in a group, or -1 if no // such value exists yet. int group_min = -1; @@ -351,13 +351,13 @@ static const SSL_CIPHER *ssl3_choose_cipher( } if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) { - prio = server_pref->ciphers; + prio = server_pref->ciphers.get(); in_group_flags = server_pref->in_group_flags; allow = client_pref.get(); } else { prio = client_pref.get(); in_group_flags = NULL; - allow = server_pref->ciphers; + allow = server_pref->ciphers.get(); } uint32_t mask_k, mask_a; @@ -375,7 +375,7 @@ static const SSL_CIPHER *ssl3_choose_cipher( (c->algorithm_auth & mask_a) && // Check the cipher is in the |allow| list. sk_SSL_CIPHER_find(allow, &cipher_index, c)) { - if (in_group_flags != NULL && in_group_flags[i] == 1) { + if (in_group_flags != NULL && in_group_flags[i]) { // This element of |prio| is in a group. Update the minimum index found // so far and continue looking. if (group_min == -1 || (size_t)group_min > cipher_index) { @@ -389,7 +389,7 @@ static const SSL_CIPHER *ssl3_choose_cipher( } } - if (in_group_flags != NULL && in_group_flags[i] == 0 && group_min != -1) { + if (in_group_flags != NULL && !in_group_flags[i] && group_min != -1) { // We are about to leave a group, but we found a match in it, so that's // our answer. return sk_SSL_CIPHER_value(allow, group_min); diff --git a/ssl/internal.h b/ssl/internal.h index f1fc63fa..b258589b 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -457,9 +457,48 @@ namespace bssl { #define SSL_HANDSHAKE_MAC_SHA256 0x2 #define SSL_HANDSHAKE_MAC_SHA384 0x4 -// SSL_MAX_DIGEST is the number of digest types which exist. When adding a new -// one, update the table in ssl_cipher.c. -#define SSL_MAX_DIGEST 4 +// An SSLCipherPreferenceList contains a list of SSL_CIPHERs with equal- +// preference groups. For TLS clients, the groups are moot because the server +// picks the cipher and groups cannot be expressed on the wire. However, for +// servers, the equal-preference groups allow the client's preferences to be +// partially respected. (This only has an effect with +// SSL_OP_CIPHER_SERVER_PREFERENCE). +// +// The equal-preference groups are expressed by grouping SSL_CIPHERs together. +// All elements of a group have the same priority: no ordering is expressed +// within a group. +// +// The values in |ciphers| are in one-to-one correspondence with +// |in_group_flags|. (That is, sk_SSL_CIPHER_num(ciphers) is the number of +// bytes in |in_group_flags|.) The bytes in |in_group_flags| are either 1, to +// indicate that the corresponding SSL_CIPHER is not the last element of a +// group, or 0 to indicate that it is. +// +// For example, if |in_group_flags| contains all zeros then that indicates a +// traditional, fully-ordered preference. Every SSL_CIPHER is the last element +// of the group (i.e. they are all in a one-element group). +// +// For a more complex example, consider: +// ciphers: A B C D E F +// in_group_flags: 1 1 0 0 1 0 +// +// That would express the following, order: +// +// A E +// B -> D -> F +// C +struct SSLCipherPreferenceList { + static constexpr bool kAllowUniquePtr = true; + + SSLCipherPreferenceList() = default; + ~SSLCipherPreferenceList(); + + bool Init(UniquePtr ciphers, + Span in_group_flags); + + UniquePtr ciphers; + bool *in_group_flags = nullptr; +}; // ssl_cipher_get_evp_aead sets |*out_aead| to point to the correct EVP_AEAD // object for |cipher| protocol version |version|. It sets |*out_mac_secret_len| @@ -477,13 +516,12 @@ const EVP_MD *ssl_get_handshake_digest(uint16_t version, const SSL_CIPHER *cipher); // ssl_create_cipher_list evaluates |rule_str|. It sets |*out_cipher_list| to a -// newly-allocated |ssl_cipher_preference_list_st| containing the result. It -// returns true on success and false on failure. If |strict| is true, nonsense -// will be rejected. If false, nonsense will be silently ignored. An empty -// result is considered an error regardless of |strict|. -bool ssl_create_cipher_list( - struct ssl_cipher_preference_list_st **out_cipher_list, - const char *rule_str, bool strict); +// newly-allocated |SSLCipherPreferenceList| containing the result. It returns +// true on success and false on failure. If |strict| is true, nonsense will be +// rejected. If false, nonsense will be silently ignored. An empty result is +// considered an error regardless of |strict|. +bool ssl_create_cipher_list(SSLCipherPreferenceList **out_cipher_list, + const char *rule_str, bool strict); // ssl_cipher_get_value returns the cipher suite id of |cipher|. uint16_t ssl_cipher_get_value(const SSL_CIPHER *cipher); @@ -1921,41 +1959,6 @@ extern const SSL_X509_METHOD ssl_crypto_x509_method; // crypto/x509. extern const SSL_X509_METHOD ssl_noop_x509_method; -// ssl_cipher_preference_list_st contains a list of SSL_CIPHERs with -// equal-preference groups. For TLS clients, the groups are moot because the -// server picks the cipher and groups cannot be expressed on the wire. However, -// for servers, the equal-preference groups allow the client's preferences to -// be partially respected. (This only has an effect with -// SSL_OP_CIPHER_SERVER_PREFERENCE). -// -// The equal-preference groups are expressed by grouping SSL_CIPHERs together. -// All elements of a group have the same priority: no ordering is expressed -// within a group. -// -// The values in |ciphers| are in one-to-one correspondence with -// |in_group_flags|. (That is, sk_SSL_CIPHER_num(ciphers) is the number of -// bytes in |in_group_flags|.) The bytes in |in_group_flags| are either 1, to -// indicate that the corresponding SSL_CIPHER is not the last element of a -// group, or 0 to indicate that it is. -// -// For example, if |in_group_flags| contains all zeros then that indicates a -// traditional, fully-ordered preference. Every SSL_CIPHER is the last element -// of the group (i.e. they are all in a one-element group). -// -// For a more complex example, consider: -// ciphers: A B C D E F -// in_group_flags: 1 1 0 0 1 0 -// -// That would express the following, order: -// -// A E -// B -> D -> F -// C -struct ssl_cipher_preference_list_st { - STACK_OF(SSL_CIPHER) *ciphers; - uint8_t *in_group_flags; -}; - struct tlsext_ticket_key { static constexpr bool kAllowUniquePtr = true; @@ -1998,7 +2001,7 @@ struct SSLContext { // configuration. enum tls13_variant_t tls13_variant; - struct ssl_cipher_preference_list_st *cipher_list; + SSLCipherPreferenceList *cipher_list; X509_STORE *cert_store; LHASH_OF(SSL_SESSION) *sessions; @@ -2621,7 +2624,7 @@ struct SSLConnection { X509_VERIFY_PARAM *param; // crypto - struct ssl_cipher_preference_list_st *cipher_list; + SSLCipherPreferenceList *cipher_list; // session info @@ -2853,13 +2856,9 @@ void ssl_session_rebase_time(SSL *ssl, SSL_SESSION *session); void ssl_session_renew_timeout(SSL *ssl, SSL_SESSION *session, uint32_t timeout); -void ssl_cipher_preference_list_free( - struct ssl_cipher_preference_list_st *cipher_list); - // ssl_get_cipher_preferences returns the cipher preference list for TLS 1.2 and // below. -const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences( - const SSL *ssl); +const SSLCipherPreferenceList *ssl_get_cipher_preferences(const SSL *ssl); void ssl_update_cache(SSL_HANDSHAKE *hs, int mode); diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index baa5a174..b1fc5fbf 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc @@ -215,8 +215,7 @@ void ssl3_free(SSL *ssl) { ssl->s3 = NULL; } -const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences( - const SSL *ssl) { +const SSLCipherPreferenceList *ssl_get_cipher_preferences(const SSL *ssl) { if (ssl->cipher_list != NULL) { return ssl->cipher_list; } diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc index 32e6c2cd..f02fa8a8 100644 --- a/ssl/ssl_cipher.cc +++ b/ssl/ssl_cipher.cc @@ -811,9 +811,14 @@ static void ll_append_head(CIPHER_ORDER **head, CIPHER_ORDER *curr, *head = curr; } -static void ssl_cipher_collect_ciphers(CIPHER_ORDER *co_list, - CIPHER_ORDER **head_p, - CIPHER_ORDER **tail_p) { +static bool ssl_cipher_collect_ciphers(Array *out_co_list, + CIPHER_ORDER **out_head, + CIPHER_ORDER **out_tail) { + Array co_list; + if (!co_list.Init(kCiphersLen)) { + return false; + } + size_t co_list_num = 0; for (const SSL_CIPHER &cipher : kCiphers) { // TLS 1.3 ciphers do not participate in this mechanism. @@ -844,9 +849,35 @@ static void ssl_cipher_collect_ciphers(CIPHER_ORDER *co_list, co_list[co_list_num - 1].next = NULL; - *head_p = &co_list[0]; - *tail_p = &co_list[co_list_num - 1]; + *out_head = &co_list[0]; + *out_tail = &co_list[co_list_num - 1]; + } else { + *out_head = nullptr; + *out_tail = nullptr; } + *out_co_list = std::move(co_list); + return true; +} + +SSLCipherPreferenceList::~SSLCipherPreferenceList() { + OPENSSL_free(in_group_flags); +} + +bool SSLCipherPreferenceList::Init(UniquePtr ciphers_arg, + Span in_group_flags_arg) { + if (sk_SSL_CIPHER_num(ciphers_arg.get()) != in_group_flags_arg.size()) { + OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); + return false; + } + + Array copy; + if (!copy.CopyFrom(in_group_flags_arg)) { + return false; + } + ciphers = std::move(ciphers_arg); + size_t unused_len; + copy.Release(&in_group_flags, &unused_len); + return true; } // ssl_cipher_apply_rule applies the rule type |rule| to ciphers matching its @@ -1201,15 +1232,8 @@ static bool ssl_cipher_process_rulestr(const char *rule_str, return true; } -bool ssl_create_cipher_list( - struct ssl_cipher_preference_list_st **out_cipher_list, - const char *rule_str, bool strict) { - STACK_OF(SSL_CIPHER) *cipherstack = NULL; - CIPHER_ORDER *co_list = NULL, *head = NULL, *tail = NULL, *curr; - uint8_t *in_group_flags = NULL; - unsigned int num_in_group_flags = 0; - struct ssl_cipher_preference_list_st *pref_list = NULL; - +bool ssl_create_cipher_list(SSLCipherPreferenceList **out_cipher_list, + const char *rule_str, bool strict) { // Return with error if nothing to do. if (rule_str == NULL || out_cipher_list == NULL) { return false; @@ -1218,14 +1242,12 @@ bool ssl_create_cipher_list( // Now we have to collect the available ciphers from the compiled in ciphers. // We cannot get more than the number compiled in, so it is used for // allocation. - co_list = (CIPHER_ORDER *)OPENSSL_malloc(sizeof(CIPHER_ORDER) * kCiphersLen); - if (co_list == NULL) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); + Array co_list; + CIPHER_ORDER *head = nullptr, *tail = nullptr; + if (!ssl_cipher_collect_ciphers(&co_list, &head, &tail)) { return false; } - ssl_cipher_collect_ciphers(co_list, &head, &tail); - // Now arrange all ciphers by preference: // TODO(davidben): Compute this order once and copy it. @@ -1285,7 +1307,7 @@ bool ssl_create_cipher_list( if (strncmp(rule_str, "DEFAULT", 7) == 0) { if (!ssl_cipher_process_rulestr(SSL_DEFAULT_CIPHER_LIST, &head, &tail, strict)) { - goto err; + return false; } rule_p += 7; if (*rule_p == ':') { @@ -1295,75 +1317,52 @@ bool ssl_create_cipher_list( if (*rule_p != '\0' && !ssl_cipher_process_rulestr(rule_p, &head, &tail, strict)) { - goto err; + return false; } // Allocate new "cipherstack" for the result, return with error // if we cannot get one. - cipherstack = sk_SSL_CIPHER_new_null(); - if (cipherstack == NULL) { - goto err; - } - - in_group_flags = (uint8_t *)OPENSSL_malloc(kCiphersLen); - if (!in_group_flags) { - goto err; + UniquePtr cipherstack(sk_SSL_CIPHER_new_null()); + Array in_group_flags; + if (cipherstack == nullptr || + !in_group_flags.Init(kCiphersLen)) { + return false; } // The cipher selection for the list is done. The ciphers are added // to the resulting precedence to the STACK_OF(SSL_CIPHER). - for (curr = head; curr != NULL; curr = curr->next) { + size_t num_in_group_flags = 0; + for (CIPHER_ORDER *curr = head; curr != NULL; curr = curr->next) { if (curr->active) { - if (!sk_SSL_CIPHER_push(cipherstack, curr->cipher)) { - goto err; + if (!sk_SSL_CIPHER_push(cipherstack.get(), curr->cipher)) { + return false; } in_group_flags[num_in_group_flags++] = curr->in_group; } } - OPENSSL_free(co_list); // Not needed any longer - co_list = NULL; - pref_list = (ssl_cipher_preference_list_st *)OPENSSL_malloc( - sizeof(struct ssl_cipher_preference_list_st)); - if (!pref_list) { - goto err; - } - pref_list->ciphers = cipherstack; - pref_list->in_group_flags = NULL; - if (num_in_group_flags) { - pref_list->in_group_flags = (uint8_t *)OPENSSL_malloc(num_in_group_flags); - if (!pref_list->in_group_flags) { - goto err; - } - OPENSSL_memcpy(pref_list->in_group_flags, in_group_flags, - num_in_group_flags); + UniquePtr pref_list = + MakeUnique(); + if (!pref_list || + !pref_list->Init( + std::move(cipherstack), + MakeConstSpan(in_group_flags).subspan(0, num_in_group_flags))) { + return false; } - OPENSSL_free(in_group_flags); - in_group_flags = NULL; - if (*out_cipher_list != NULL) { - ssl_cipher_preference_list_free(*out_cipher_list); + + if (*out_cipher_list) { + Delete(*out_cipher_list); } - *out_cipher_list = pref_list; - pref_list = NULL; + *out_cipher_list = pref_list.release(); // Configuring an empty cipher list is an error but still updates the // output. - if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers) == 0) { + if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH); return false; } return true; - -err: - OPENSSL_free(co_list); - OPENSSL_free(in_group_flags); - sk_SSL_CIPHER_free(cipherstack); - if (pref_list) { - OPENSSL_free(pref_list->in_group_flags); - } - OPENSSL_free(pref_list); - return false; } uint16_t ssl_cipher_get_value(const SSL_CIPHER *cipher) { diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 8f1de028..78a1860d 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -272,16 +272,6 @@ ssl_open_record_t ssl_open_app_data(SSL *ssl, Span *out, return ret; } -void ssl_cipher_preference_list_free( - struct ssl_cipher_preference_list_st *cipher_list) { - if (cipher_list == NULL) { - return; - } - sk_SSL_CIPHER_free(cipher_list->ciphers); - OPENSSL_free(cipher_list->in_group_flags); - OPENSSL_free(cipher_list); -} - void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) { SSL *const ssl = hs->ssl; SSL_CTX *ctx = ssl->session_ctx; @@ -622,7 +612,7 @@ void SSL_CTX_free(SSL_CTX *ctx) { CRYPTO_MUTEX_cleanup(&ctx->lock); lh_SSL_SESSION_free(ctx->sessions); - ssl_cipher_preference_list_free(ctx->cipher_list); + Delete(ctx->cipher_list); Delete(ctx->cert); sk_SSL_CUSTOM_EXTENSION_pop_free(ctx->client_custom_extensions, SSL_CUSTOM_EXTENSION_free); @@ -765,7 +755,7 @@ void SSL_free(SSL *ssl) { BIO_free_all(ssl->wbio); // add extra stuff - ssl_cipher_preference_list_free(ssl->cipher_list); + Delete(ssl->cipher_list); SSL_SESSION_free(ssl->session); @@ -1820,11 +1810,11 @@ int SSL_set_tmp_dh(SSL *ssl, const DH *dh) { } STACK_OF(SSL_CIPHER) *SSL_CTX_get_ciphers(const SSL_CTX *ctx) { - return ctx->cipher_list->ciphers; + return ctx->cipher_list->ciphers.get(); } int SSL_CTX_cipher_in_group(const SSL_CTX *ctx, size_t i) { - if (i >= sk_SSL_CIPHER_num(ctx->cipher_list->ciphers)) { + if (i >= sk_SSL_CIPHER_num(ctx->cipher_list->ciphers.get())) { return 0; } return ctx->cipher_list->in_group_flags[i]; @@ -1835,13 +1825,8 @@ STACK_OF(SSL_CIPHER) *SSL_get_ciphers(const SSL *ssl) { return NULL; } - const struct ssl_cipher_preference_list_st *prefs = - ssl_get_cipher_preferences(ssl); - if (prefs == NULL) { - return NULL; - } - - return prefs->ciphers; + const SSLCipherPreferenceList *prefs = ssl_get_cipher_preferences(ssl); + return prefs == nullptr ? nullptr : prefs->ciphers.get(); } const char *SSL_get_cipher_list(const SSL *ssl, int n) {