From 57e7401fc5c6af8e9266a721be669a3b70fbfb3f Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 15 Apr 2021 10:00:40 +0100 Subject: [PATCH] Fix some TODO(3.0) occurrences in ssl/t1_lib.c One was related to probing for the combination of signature and hash algorithm together. This is currently not easily possible. The TODO(3.0) is converted to a normal comment and I've raised the problem as issue number #14885 as something to resolve post 3.0. The other TODO was a hard coded limit on the number of groups that could be registered. This has been amended so that there is no limit. Fixes #14333 Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/14886) --- ssl/t1_lib.c | 38 ++++++++++++++++++++-------- test/tls-provider.c | 61 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 31873a3fa2..14c16e355d 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -691,13 +691,13 @@ err: return 0; } -/* TODO(3.0): An arbitrary amount for now. Take another look at this */ -# define MAX_GROUPLIST 40 +# define GROUPLIST_INCREMENT 40 # define GROUP_NAME_BUFFER_LENGTH 64 typedef struct { SSL_CTX *ctx; size_t gidcnt; - uint16_t gid_arr[MAX_GROUPLIST]; + size_t gidmax; + uint16_t *gid_arr; } gid_cb_st; static int gid_cb(const char *elem, int len, void *arg) @@ -709,8 +709,14 @@ static int gid_cb(const char *elem, int len, void *arg) if (elem == NULL) return 0; - if (garg->gidcnt == MAX_GROUPLIST) - return 0; + if (garg->gidcnt == garg->gidmax) { + uint16_t *tmp = + OPENSSL_realloc(garg->gid_arr, garg->gidmax + GROUPLIST_INCREMENT); + if (tmp == NULL) + return 0; + garg->gidmax += GROUPLIST_INCREMENT; + garg->gid_arr = tmp; + } if (len > (int)(sizeof(etmp) - 1)) return 0; memcpy(etmp, elem, len); @@ -732,13 +738,20 @@ int tls1_set_groups_list(SSL_CTX *ctx, uint16_t **pext, size_t *pextlen, { gid_cb_st gcb; uint16_t *tmparr; + int ret = 0; gcb.gidcnt = 0; + gcb.gidmax = GROUPLIST_INCREMENT; + gcb.gid_arr = OPENSSL_malloc(gcb.gidmax * sizeof(*gcb.gid_arr)); + if (gcb.gid_arr == NULL) + return 0; gcb.ctx = ctx; if (!CONF_parse_list(str, ':', 1, gid_cb, &gcb)) - return 0; - if (pext == NULL) - return 1; + goto end; + if (pext == NULL) { + ret = 1; + goto end; + } /* * gid_cb ensurse there are no duplicates so we can just go ahead and set @@ -746,10 +759,13 @@ int tls1_set_groups_list(SSL_CTX *ctx, uint16_t **pext, size_t *pextlen, */ tmparr = OPENSSL_memdup(gcb.gid_arr, gcb.gidcnt * sizeof(*tmparr)); if (tmparr == NULL) - return 0; + goto end; *pext = tmparr; *pextlen = gcb.gidcnt; - return 1; + ret = 1; + end: + OPENSSL_free(gcb.gid_arr); + return ret; } /* Check a group id matches preferences */ @@ -1142,7 +1158,7 @@ int ssl_setup_sig_algs(SSL_CTX *ctx) /* * Check hash is available. - * TODO(3.0): This test is not perfect. A provider could have support + * This test is not perfect. A provider could have support * for a signature scheme, but not a particular hash. However the hash * could be available from some other loaded provider. In that case it * could be that the signature is available, and the hash is available diff --git a/test/tls-provider.c b/test/tls-provider.c index 482c3aa0da..d9d52664b2 100644 --- a/test/tls-provider.c +++ b/test/tls-provider.c @@ -14,6 +14,7 @@ #include /* For TLS1_3_VERSION */ #include +#include static OSSL_FUNC_keymgmt_import_fn xor_import; static OSSL_FUNC_keymgmt_import_types_fn xor_import_types; @@ -167,16 +168,52 @@ static const OSSL_PARAM xor_kemgroup_params[] = { OSSL_PARAM_END }; +#define NUM_DUMMY_GROUPS 50 +static char *dummy_group_names[NUM_DUMMY_GROUPS]; static int tls_prov_get_capabilities(void *provctx, const char *capability, OSSL_CALLBACK *cb, void *arg) { - if (strcmp(capability, "TLS-GROUP") == 0) - return cb(xor_group_params, arg) - && cb(xor_kemgroup_params, arg); + int ret; + int i; + const char *dummy_base = "dummy"; + const size_t dummy_name_max_size = strlen(dummy_base) + 3; + + if (strcmp(capability, "TLS-GROUP") != 0) { + /* We don't support this capability */ + return 0; + } + + /* Register our 2 groups */ + ret = cb(xor_group_params, arg); + ret &= cb(xor_kemgroup_params, arg); + + /* + * Now register some dummy groups > GROUPLIST_INCREMENT (== 40) as defined + * in ssl/t1_lib.c, to make sure we exercise the code paths for registering + * large numbers of groups. + */ + + for (i = 0; i < NUM_DUMMY_GROUPS; i++) { + OSSL_PARAM dummygroup[OSSL_NELEM(xor_group_params)]; + + memcpy(dummygroup, xor_group_params, sizeof(xor_group_params)); - /* We don't support this capability */ - return 0; + /* Give the dummy group a unique name */ + if (dummy_group_names[i] == NULL) { + dummy_group_names[i] = OPENSSL_zalloc(dummy_name_max_size); + if (dummy_group_names[i] == NULL) + return 0; + BIO_snprintf(dummy_group_names[i], + dummy_name_max_size, + "%s%d", dummy_base, i); + } + dummygroup[0].data = dummy_group_names[i]; + dummygroup[0].data_size = strlen(dummy_group_names[i]) + 1; + ret &= cb(dummygroup, arg); + } + + return ret; } /* @@ -743,9 +780,21 @@ static const OSSL_ALGORITHM *tls_prov_query(void *provctx, int operation_id, return NULL; } +static void tls_prov_teardown(void *provctx) +{ + int i; + + OSSL_LIB_CTX_free(provctx); + + for (i = 0; i < NUM_DUMMY_GROUPS; i++) { + OPENSSL_free(dummy_group_names[i]); + dummy_group_names[i] = NULL; + } +} + /* Functions we provide to the core */ static const OSSL_DISPATCH tls_prov_dispatch_table[] = { - { OSSL_FUNC_PROVIDER_TEARDOWN, (void (*)(void))OSSL_LIB_CTX_free }, + { OSSL_FUNC_PROVIDER_TEARDOWN, (void (*)(void))tls_prov_teardown }, { OSSL_FUNC_PROVIDER_QUERY_OPERATION, (void (*)(void))tls_prov_query }, { OSSL_FUNC_PROVIDER_GET_CAPABILITIES, (void (*)(void))tls_prov_get_capabilities }, { 0, NULL } -- 2.39.5