From a53b5be6a056e998fb119dbf035d1df68083a951 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 3 Apr 2018 15:31:38 +0100 Subject: [PATCH] Fix configuration of TLSv1.3 ciphersuites Configuration of TLSv1.3 ciphersuites wasn't working in some cases. Fixes #5740 Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/5855) --- ssl/ssl_ciph.c | 136 +++++++++++++++++++++++++++++++++++++++--- ssl/ssl_lib.c | 93 ----------------------------- ssl/ssl_locl.h | 1 + test/sslcorrupttest.c | 6 +- 4 files changed, 131 insertions(+), 105 deletions(-) diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c index be728dfbe8..9011e42fa8 100644 --- a/ssl/ssl_ciph.c +++ b/ssl/ssl_ciph.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "internal/nelem.h" #include "ssl_locl.h" #include "internal/thread_once.h" @@ -1274,6 +1275,131 @@ static int check_suiteb_cipher_list(const SSL_METHOD *meth, CERT *c, } #endif +static int ciphersuite_cb(const char *elem, int len, void *arg) +{ + STACK_OF(SSL_CIPHER) *ciphersuites = (STACK_OF(SSL_CIPHER) *)arg; + const SSL_CIPHER *cipher; + /* Arbitrary sized temp buffer for the cipher name. Should be big enough */ + char name[80]; + + if (len > (int)(sizeof(name) - 1)) { + SSLerr(SSL_F_CIPHERSUITE_CB, SSL_R_NO_CIPHER_MATCH); + return 0; + } + + memcpy(name, elem, len); + name[len] = '\0'; + + cipher = ssl3_get_cipher_by_std_name(name); + if (cipher == NULL) { + SSLerr(SSL_F_CIPHERSUITE_CB, SSL_R_NO_CIPHER_MATCH); + return 0; + } + + if (!sk_SSL_CIPHER_push(ciphersuites, cipher)) { + SSLerr(SSL_F_CIPHERSUITE_CB, ERR_R_INTERNAL_ERROR); + return 0; + } + + return 1; +} + +int set_ciphersuites(STACK_OF(SSL_CIPHER) **currciphers, const char *str) +{ + STACK_OF(SSL_CIPHER) *newciphers = sk_SSL_CIPHER_new_null(); + + if (newciphers == NULL) + return 0; + + /* Parse the list. We explicitly allow an empty list */ + if (*str != '\0' + && !CONF_parse_list(str, ':', 1, ciphersuite_cb, newciphers)) { + sk_SSL_CIPHER_free(newciphers); + return 0; + } + sk_SSL_CIPHER_free(*currciphers); + *currciphers = newciphers; + + return 1; +} + +static int update_cipher_list_by_id(STACK_OF(SSL_CIPHER) **cipher_list_by_id, + STACK_OF(SSL_CIPHER) *cipherstack) +{ + STACK_OF(SSL_CIPHER) *tmp_cipher_list = sk_SSL_CIPHER_dup(cipherstack); + + if (tmp_cipher_list == NULL) { + return 0; + } + + sk_SSL_CIPHER_free(*cipher_list_by_id); + *cipher_list_by_id = tmp_cipher_list; + + (void)sk_SSL_CIPHER_set_cmp_func(*cipher_list_by_id, ssl_cipher_ptr_id_cmp); + sk_SSL_CIPHER_sort(*cipher_list_by_id); + + return 1; +} + +static int update_cipher_list(STACK_OF(SSL_CIPHER) **cipher_list, + STACK_OF(SSL_CIPHER) **cipher_list_by_id, + STACK_OF(SSL_CIPHER) *tls13_ciphersuites) +{ + int i; + STACK_OF(SSL_CIPHER) *tmp_cipher_list = sk_SSL_CIPHER_dup(*cipher_list); + + if (tmp_cipher_list == NULL) + return 0; + + /* + * Delete any existing TLSv1.3 ciphersuites. These are always first in the + * list. + */ + while (sk_SSL_CIPHER_num(tmp_cipher_list) > 0 + && sk_SSL_CIPHER_value(tmp_cipher_list, 0)->min_tls + == TLS1_3_VERSION) + sk_SSL_CIPHER_delete(tmp_cipher_list, 0); + + /* Insert the new TLSv1.3 ciphersuites */ + for (i = 0; i < sk_SSL_CIPHER_num(tls13_ciphersuites); i++) + sk_SSL_CIPHER_insert(tmp_cipher_list, + sk_SSL_CIPHER_value(tls13_ciphersuites, i), i); + + if (!update_cipher_list_by_id(cipher_list_by_id, tmp_cipher_list)) + return 0; + + sk_SSL_CIPHER_free(*cipher_list); + *cipher_list = tmp_cipher_list; + + return 1; +} + +int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str) +{ + int ret = set_ciphersuites(&(ctx->tls13_ciphersuites), str); + + if (ret && ctx->cipher_list != NULL) { + /* We already have a cipher_list, so we need to update it */ + return update_cipher_list(&ctx->cipher_list, &ctx->cipher_list_by_id, + ctx->tls13_ciphersuites); + } + + return ret; +} + +int SSL_set_ciphersuites(SSL *s, const char *str) +{ + int ret = set_ciphersuites(&(s->tls13_ciphersuites), str); + + if (ret && s->cipher_list != NULL) { + /* We already have a cipher_list, so we need to update it */ + return update_cipher_list(&s->cipher_list, &s->cipher_list_by_id, + s->tls13_ciphersuites); + } + + return ret; +} + STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method, STACK_OF(SSL_CIPHER) *tls13_ciphersuites, STACK_OF(SSL_CIPHER) **cipher_list, @@ -1283,7 +1409,7 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method, { int ok, num_of_ciphers, num_of_alias_max, num_of_group_aliases, i; uint32_t disabled_mkey, disabled_auth, disabled_enc, disabled_mac; - STACK_OF(SSL_CIPHER) *cipherstack, *tmp_cipher_list; + STACK_OF(SSL_CIPHER) *cipherstack; const char *rule_p; CIPHER_ORDER *co_list = NULL, *head = NULL, *tail = NULL, *curr; const SSL_CIPHER **ca_list = NULL; @@ -1498,19 +1624,13 @@ STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method, } OPENSSL_free(co_list); /* Not needed any longer */ - tmp_cipher_list = sk_SSL_CIPHER_dup(cipherstack); - if (tmp_cipher_list == NULL) { + if (!update_cipher_list_by_id(cipher_list_by_id, cipherstack)) { sk_SSL_CIPHER_free(cipherstack); return NULL; } sk_SSL_CIPHER_free(*cipher_list); *cipher_list = cipherstack; - if (*cipher_list_by_id != NULL) - sk_SSL_CIPHER_free(*cipher_list_by_id); - *cipher_list_by_id = tmp_cipher_list; - (void)sk_SSL_CIPHER_set_cmp_func(*cipher_list_by_id, ssl_cipher_ptr_id_cmp); - sk_SSL_CIPHER_sort(*cipher_list_by_id); return cipherstack; } diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index ae157300ae..b1d78dc35e 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2549,99 +2549,6 @@ int SSL_set_cipher_list(SSL *s, const char *str) return 1; } -static int ciphersuite_cb(const char *elem, int len, void *arg) -{ - STACK_OF(SSL_CIPHER) *ciphersuites = (STACK_OF(SSL_CIPHER) *)arg; - const SSL_CIPHER *cipher; - /* Arbitrary sized temp buffer for the cipher name. Should be big enough */ - char name[80]; - - if (len > (int)(sizeof(name) - 1)) { - SSLerr(SSL_F_CIPHERSUITE_CB, SSL_R_NO_CIPHER_MATCH); - return 0; - } - - memcpy(name, elem, len); - name[len] = '\0'; - - cipher = ssl3_get_cipher_by_std_name(name); - if (cipher == NULL) { - SSLerr(SSL_F_CIPHERSUITE_CB, SSL_R_NO_CIPHER_MATCH); - return 0; - } - - if (!sk_SSL_CIPHER_push(ciphersuites, cipher)) { - SSLerr(SSL_F_CIPHERSUITE_CB, ERR_R_INTERNAL_ERROR); - return 0; - } - - return 1; -} - -static int set_ciphersuites(STACK_OF(SSL_CIPHER) **currciphers, const char *str) -{ - STACK_OF(SSL_CIPHER) *newciphers = sk_SSL_CIPHER_new_null(); - - if (newciphers == NULL) - return 0; - - /* Parse the list. We explicitly allow an empty list */ - if (*str != '\0' - && !CONF_parse_list(str, ':', 1, ciphersuite_cb, newciphers)) { - sk_SSL_CIPHER_free(newciphers); - return 0; - } - sk_SSL_CIPHER_free(*currciphers); - *currciphers = newciphers; - - return 1; -} - -static int update_cipher_list(STACK_OF(SSL_CIPHER) *cipher_list, - STACK_OF(SSL_CIPHER) *tls13_ciphersuites) -{ - int i; - - /* - * Delete any existing TLSv1.3 ciphersuites. These are always first in the - * list. - */ - while (sk_SSL_CIPHER_num(cipher_list) > 0 - && sk_SSL_CIPHER_value(cipher_list, 0)->min_tls == TLS1_3_VERSION) - sk_SSL_CIPHER_delete(cipher_list, 0); - - /* Insert the new TLSv1.3 ciphersuites */ - for (i = 0; i < sk_SSL_CIPHER_num(tls13_ciphersuites); i++) - sk_SSL_CIPHER_insert(cipher_list, - sk_SSL_CIPHER_value(tls13_ciphersuites, i), i); - - return 1; -} - -int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str) -{ - int ret = set_ciphersuites(&(ctx->tls13_ciphersuites), str); - - if (ret && ctx->cipher_list != NULL) { - /* We already have a cipher_list, so we need to update it */ - return update_cipher_list(ctx->cipher_list, ctx->tls13_ciphersuites); - } - - return ret; -} - -int SSL_set_ciphersuites(SSL *s, const char *str) -{ - int ret = set_ciphersuites(&(s->tls13_ciphersuites), str); - - if (ret && s->cipher_list != NULL) { - /* We already have a cipher_list, so we need to update it */ - return update_cipher_list(s->cipher_list, s->tls13_ciphersuites); - } - - return ret; -} - char *SSL_get_shared_ciphers(const SSL *s, char *buf, int len) { char *p; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index a9ef6c363a..b1d6e40ed1 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2210,6 +2210,7 @@ __owur int ssl_cipher_id_cmp(const SSL_CIPHER *a, const SSL_CIPHER *b); DECLARE_OBJ_BSEARCH_GLOBAL_CMP_FN(SSL_CIPHER, SSL_CIPHER, ssl_cipher_id); __owur int ssl_cipher_ptr_id_cmp(const SSL_CIPHER *const *ap, const SSL_CIPHER *const *bp); +__owur int set_ciphersuites(STACK_OF(SSL_CIPHER) **currciphers, const char *str); __owur STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *ssl_method, STACK_OF(SSL_CIPHER) *tls13_ciphersuites, STACK_OF(SSL_CIPHER) **cipher_list, diff --git a/test/sslcorrupttest.c b/test/sslcorrupttest.c index 0219559ef6..30d3d3d947 100644 --- a/test/sslcorrupttest.c +++ b/test/sslcorrupttest.c @@ -198,11 +198,9 @@ static int test_ssl_corrupt(int testidx) &sctx, &cctx, cert, privkey))) return 0; - if (!TEST_true(SSL_CTX_set_cipher_list(cctx, cipher_list[testidx]))) - goto end; - - if (!TEST_ptr(ciphers = SSL_CTX_get_ciphers(cctx)) + if (!TEST_true(SSL_CTX_set_cipher_list(cctx, cipher_list[testidx])) || !TEST_true(SSL_CTX_set_ciphersuites(cctx, "")) + || !TEST_ptr(ciphers = SSL_CTX_get_ciphers(cctx)) || !TEST_int_eq(sk_SSL_CIPHER_num(ciphers), 1) || !TEST_ptr(currcipher = sk_SSL_CIPHER_value(ciphers, 0))) goto end; -- 2.25.1