From d95381ec7a50c6c31a3524a4ca1f9efc8a655dc0 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 15 Nov 2021 14:39:22 +0100 Subject: [PATCH 1/4] tls-socket: Handle sending fatal errors better In particular as server, the previous code might cause it to hang in recv() if this case wasn't triggered by a close notify (followed by a shutdown of the socket) but it e.g. failed processing a ServerHello and responded with a fatal alert. Fixes: 09fbaad6bd71 ("tls-socket: Don't fail reading if sending data failed") --- src/libtls/tls_socket.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libtls/tls_socket.c b/src/libtls/tls_socket.c index 75f146929..e15030ee7 100644 --- a/src/libtls/tls_socket.c +++ b/src/libtls/tls_socket.c @@ -193,11 +193,13 @@ static bool exchange(private_tls_socket_t *this, bool wr, bool block) case SUCCESS: return TRUE; default: - if (wr) - { - return FALSE; + if (!wr && this->app.in_done > 0) + { /* return data after proper termination via fatal close + * notify to which we responded with one */ + this->eof = TRUE; + return TRUE; } - break; + return FALSE; } break; } From 88e7654c6cd43e6bf67b222297c779b7fabea4cf Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Mon, 15 Nov 2021 14:01:44 +0100 Subject: [PATCH 2/4] libtls: Shutdown server socket in test teardown function If a test fails and the server thread is blocked reading on the socket, it would stay stuck. --- src/libtls/tests/suites/test_socket.c | 83 ++++++++++++--------------- 1 file changed, 36 insertions(+), 47 deletions(-) diff --git a/src/libtls/tests/suites/test_socket.c b/src/libtls/tests/suites/test_socket.c index 9e26e91e4..2cd1607d9 100644 --- a/src/libtls/tests/suites/test_socket.c +++ b/src/libtls/tests/suites/test_socket.c @@ -366,14 +366,6 @@ START_SETUP(setup_all_creds) } END_SETUP -START_TEARDOWN(teardown_creds) -{ - lib->credmgr->remove_set(lib->credmgr, &creds->set); - creds->destroy(creds); - creds = NULL; -} -END_TEARDOWN - /** * Configuration for an echo server */ @@ -386,6 +378,27 @@ typedef struct { bool cauth; } echo_server_config_t; +/** + * Global server config for current test + */ +static echo_server_config_t *server_config; + +START_TEARDOWN(teardown_creds) +{ + lib->credmgr->remove_set(lib->credmgr, &creds->set); + creds->destroy(creds); + creds = NULL; + + if (server_config) + { + shutdown(server_config->fd, SHUT_RDWR); + close(server_config->fd); + free(server_config); + server_config = NULL; + } +} +END_TEARDOWN + /** * Run an echo server */ @@ -545,28 +558,22 @@ static echo_server_config_t *create_config(tls_version_t version, uint16_t port, */ static void test_tls(tls_version_t version, uint16_t port, bool cauth, u_int i) { - echo_server_config_t *config; tls_cipher_suite_t *suites; char suite[128]; int count; - config = create_config(version, port, cauth); + server_config = create_config(version, port, cauth); - start_echo_server(config); + start_echo_server(server_config); count = tls_crypto_get_supported_suites(TRUE, version, &suites); ck_assert(i < count); snprintf(suite, sizeof(suite), "%N", tls_cipher_suite_names, suites[i]); lib->settings->set_str(lib->settings, "%s.tls.suites", suite, lib->ns); - run_echo_client(config); + run_echo_client(server_config); free(suites); - - shutdown(config->fd, SHUT_RDWR); - close(config->fd); - - free(config); } /** @@ -575,14 +582,13 @@ static void test_tls(tls_version_t version, uint16_t port, bool cauth, u_int i) static void test_tls_ke_groups(tls_version_t version, uint16_t port, bool cauth, u_int i) { - echo_server_config_t *config; diffie_hellman_group_t *groups; char curve[128]; int count; - config = create_config(version, port, cauth); + server_config = create_config(version, port, cauth); - start_echo_server(config); + start_echo_server(server_config); count = tls_crypto_get_supported_groups(&groups); ck_assert(i < count); @@ -590,14 +596,9 @@ static void test_tls_ke_groups(tls_version_t version, uint16_t port, bool cauth, groups[i]); lib->settings->set_str(lib->settings, "%s.tls.ke_group", curve, lib->ns); - run_echo_client(config); + run_echo_client(server_config); free(groups); - - shutdown(config->fd, SHUT_RDWR); - close(config->fd); - - free(config); } /** @@ -606,14 +607,13 @@ static void test_tls_ke_groups(tls_version_t version, uint16_t port, bool cauth, static void test_tls_signature_schemes(tls_version_t version, uint16_t port, bool cauth, u_int i) { - echo_server_config_t *config; tls_signature_scheme_t *schemes; char signature[128]; int count; - config = create_config(version, port, cauth); + server_config = create_config(version, port, cauth); - start_echo_server(config); + start_echo_server(server_config); count = tls_crypto_get_supported_signatures(version, &schemes); ck_assert(i < count); @@ -621,14 +621,9 @@ static void test_tls_signature_schemes(tls_version_t version, uint16_t port, schemes[i]); lib->settings->set_str(lib->settings, "%s.tls.signature", signature, lib->ns); - run_echo_client(config); + run_echo_client(server_config); free(schemes); - - shutdown(config->fd, SHUT_RDWR); - close(config->fd); - - free(config); } /** @@ -637,22 +632,19 @@ static void test_tls_signature_schemes(tls_version_t version, uint16_t port, static void test_tls_server(tls_version_t version, uint16_t port, bool cauth, u_int i) { - echo_server_config_t *client, *server; + echo_server_config_t *client; + server_config = create_config(version, port, cauth); client = create_config(i, port, cauth); - server = create_config(version, port, cauth); - start_echo_server(server); + start_echo_server(server_config); run_echo_client(client); shutdown(client->fd, SHUT_RDWR); close(client->fd); - shutdown(server->fd, SHUT_RDWR); - close(server->fd); free(client); - free(server); } /** @@ -661,22 +653,19 @@ static void test_tls_server(tls_version_t version, uint16_t port, bool cauth, static void test_tls_client(tls_version_t version, uint16_t port, bool cauth, u_int i) { - echo_server_config_t *client, *server; + echo_server_config_t *client; + server_config = create_config(i, port, cauth); client = create_config(version, port, cauth); - server = create_config(i, port, cauth); - start_echo_server(server); + start_echo_server(server_config); run_echo_client(client); shutdown(client->fd, SHUT_RDWR); close(client->fd); - shutdown(server->fd, SHUT_RDWR); - close(server->fd); free(client); - free(server); } START_TEST(test_tls_12_server) From 03520a0d54b3aefa5914dba349cc03432bc39d32 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 16 Nov 2021 14:33:09 +0100 Subject: [PATCH 3/4] openssl: Add helper to map ECDH groups to curve NIDs --- .../openssl/openssl_ec_diffie_hellman.c | 74 ++++++++++--------- .../openssl/openssl_ec_diffie_hellman.h | 9 ++- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.c b/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.c index de0d94a3e..d591b0517 100644 --- a/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.c +++ b/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.c @@ -328,49 +328,52 @@ METHOD(diffie_hellman_t, destroy, void, } /* - * Described in header. + * Described in header + */ +int openssl_ecdh_group_to_nid(diffie_hellman_group_t group) +{ + switch (group) + { + case ECP_192_BIT: + return NID_X9_62_prime192v1; + case ECP_224_BIT: + return NID_secp224r1; + case ECP_256_BIT: + return NID_X9_62_prime256v1; + case ECP_384_BIT: + return NID_secp384r1; + case ECP_521_BIT: + return NID_secp521r1; +/* added with 1.0.2 */ +#if OPENSSL_VERSION_NUMBER >= 0x10002000L + case ECP_224_BP: + return NID_brainpoolP224r1; + case ECP_256_BP: + return NID_brainpoolP256r1; + case ECP_384_BP: + return NID_brainpoolP384r1; + case ECP_512_BP: + return NID_brainpoolP512r1; +#endif + default: + return 0; + } +} + +/* + * Described in header */ openssl_ec_diffie_hellman_t *openssl_ec_diffie_hellman_create(diffie_hellman_group_t group) { private_openssl_ec_diffie_hellman_t *this; EC_KEY *key = NULL; + int curve; - switch (group) + curve = openssl_ecdh_group_to_nid(group); + if (curve) { - case ECP_192_BIT: - key = EC_KEY_new_by_curve_name(NID_X9_62_prime192v1); - break; - case ECP_224_BIT: - key = EC_KEY_new_by_curve_name(NID_secp224r1); - break; - case ECP_256_BIT: - key = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1); - break; - case ECP_384_BIT: - key = EC_KEY_new_by_curve_name(NID_secp384r1); - break; - case ECP_521_BIT: - key = EC_KEY_new_by_curve_name(NID_secp521r1); - break; -/* added with 1.0.2 */ -#if OPENSSL_VERSION_NUMBER >= 0x10002000L - case ECP_224_BP: - key = EC_KEY_new_by_curve_name(NID_brainpoolP224r1); - break; - case ECP_256_BP: - key = EC_KEY_new_by_curve_name(NID_brainpoolP256r1); - break; - case ECP_384_BP: - key = EC_KEY_new_by_curve_name(NID_brainpoolP384r1); - break; - case ECP_512_BP: - key = EC_KEY_new_by_curve_name(NID_brainpoolP512r1); - break; -#endif - default: - break; + key = EC_KEY_new_by_curve_name(curve); } - if (!key) { return NULL; @@ -408,4 +411,5 @@ openssl_ec_diffie_hellman_t *openssl_ec_diffie_hellman_create(diffie_hellman_gro } return &this->public; } + #endif /* OPENSSL_NO_EC */ diff --git a/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.h b/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.h index 2f58c976d..12563c6da 100644 --- a/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.h +++ b/src/libstrongswan/plugins/openssl/openssl_ec_diffie_hellman.h @@ -44,5 +44,12 @@ struct openssl_ec_diffie_hellman_t { */ openssl_ec_diffie_hellman_t *openssl_ec_diffie_hellman_create(diffie_hellman_group_t group); -#endif /** OPENSSL_EC_DIFFIE_HELLMAN_H_ @}*/ +/** + * Map ECDH groups to OpenSSL NIDs for the ECC curve. + * + * @param group ECDH group + * @return NID for the curve + */ +int openssl_ecdh_group_to_nid(diffie_hellman_group_t group); +#endif /** OPENSSL_EC_DIFFIE_HELLMAN_H_ @}*/ From 46a6b062822c08f2d63690dd66caa52da734ee8d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 16 Nov 2021 14:34:03 +0100 Subject: [PATCH 4/4] openssl: Only announce ECDH groups actually supported by OpenSSL Determined by whether the library provides curves for it or not. For instance, in the OpenSSL 3 FIPS provider the Brainpool curves are not included. And in the Fedora package several weak curves are explicitly patched out and the Brainpool curves are omitted even in non-FIPS mode. --- .../plugins/openssl/openssl_plugin.c | 92 +++++++++++++++---- 1 file changed, 76 insertions(+), 16 deletions(-) diff --git a/src/libstrongswan/plugins/openssl/openssl_plugin.c b/src/libstrongswan/plugins/openssl/openssl_plugin.c index a989b1da6..49b77f8ac 100644 --- a/src/libstrongswan/plugins/openssl/openssl_plugin.c +++ b/src/libstrongswan/plugins/openssl/openssl_plugin.c @@ -28,6 +28,9 @@ #ifndef OPENSSL_NO_ENGINE #include #endif +#ifndef OPENSSL_NO_ECDH +#include +#endif #include "openssl_plugin.h" #include "openssl_util.h" @@ -486,10 +489,55 @@ METHOD(plugin_t, get_name, char*, return "openssl"; } +#ifndef OPENSSL_NO_ECDH +/** + * Check if the given DH group is in the list of supported curves. + */ +static bool ecdh_group_supported(EC_builtin_curve *curves, size_t num_curves, + diffie_hellman_group_t group) +{ + int j; + + for (j = 0; j < num_curves; j++) + { + if (curves[j].nid == openssl_ecdh_group_to_nid(group)) + { + return TRUE; + } + } + return FALSE; +} + +/** + * Only add features for ECDH groups that are actually supported. + */ +static void add_ecdh_features(plugin_feature_t *features, + plugin_feature_t *to_add, int count, int *pos) +{ + size_t num_curves; + int i; + + num_curves = EC_get_builtin_curves(NULL, 0); + + EC_builtin_curve curves[num_curves]; + + num_curves = EC_get_builtin_curves(curves, num_curves); + + for (i = 0; i < count; i++) + { + if (to_add[i].kind != FEATURE_PROVIDE || + ecdh_group_supported(curves, num_curves, to_add[i].arg.dh_group)) + { + features[(*pos)++] = to_add[i]; + } + } +} +#endif /* OPENSSL_NO_ECDH */ + METHOD(plugin_t, get_features, int, private_openssl_plugin_t *this, plugin_feature_t *features[]) { - static plugin_feature_t f[] = { + static plugin_feature_t f_base[] = { /* we provide OpenSSL threading callbacks */ PLUGIN_PROVIDE(CUSTOM, "openssl-threading"), /* crypters */ @@ -635,21 +683,6 @@ METHOD(plugin_t, get_features, int, PLUGIN_PROVIDE(AEAD, ENCR_CHACHA20_POLY1305, 32), #endif /* OPENSSL_NO_CHACHA */ #endif /* OPENSSL_VERSION_NUMBER */ -#ifndef OPENSSL_NO_ECDH - /* EC DH groups */ - PLUGIN_REGISTER(DH, openssl_ec_diffie_hellman_create), - PLUGIN_PROVIDE(DH, ECP_256_BIT), - PLUGIN_PROVIDE(DH, ECP_384_BIT), - PLUGIN_PROVIDE(DH, ECP_521_BIT), - PLUGIN_PROVIDE(DH, ECP_224_BIT), - PLUGIN_PROVIDE(DH, ECP_192_BIT), -#if OPENSSL_VERSION_NUMBER >= 0x10002000L - PLUGIN_PROVIDE(DH, ECP_256_BP), - PLUGIN_PROVIDE(DH, ECP_384_BP), - PLUGIN_PROVIDE(DH, ECP_512_BP), - PLUGIN_PROVIDE(DH, ECP_224_BP), -#endif /* OPENSSL_VERSION_NUMBER */ -#endif /* OPENSSL_NO_ECDH */ #ifndef OPENSSL_NO_DH /* MODP DH groups */ PLUGIN_REGISTER(DH, openssl_diffie_hellman_create), @@ -809,6 +842,33 @@ METHOD(plugin_t, get_features, int, PLUGIN_PROVIDE(RNG, RNG_STRONG), PLUGIN_PROVIDE(RNG, RNG_WEAK), }; + static plugin_feature_t f_ecdh[] = { +#ifndef OPENSSL_NO_ECDH + /* EC DH groups */ + PLUGIN_REGISTER(DH, openssl_ec_diffie_hellman_create), + PLUGIN_PROVIDE(DH, ECP_256_BIT), + PLUGIN_PROVIDE(DH, ECP_384_BIT), + PLUGIN_PROVIDE(DH, ECP_521_BIT), + PLUGIN_PROVIDE(DH, ECP_224_BIT), + PLUGIN_PROVIDE(DH, ECP_192_BIT), +#if OPENSSL_VERSION_NUMBER >= 0x10002000L + PLUGIN_PROVIDE(DH, ECP_256_BP), + PLUGIN_PROVIDE(DH, ECP_384_BP), + PLUGIN_PROVIDE(DH, ECP_512_BP), + PLUGIN_PROVIDE(DH, ECP_224_BP), +#endif /* OPENSSL_VERSION_NUMBER */ +#endif /* OPENSSL_NO_ECDH */ + }; + static plugin_feature_t f[countof(f_base) + countof(f_ecdh)] = {}; + static int count = 0; + + if (!count) + { + plugin_features_add(f, f_base, countof(f_base), &count); +#ifndef OPENSSL_NO_ECDH + add_ecdh_features(f, f_ecdh, countof(f_ecdh), &count); +#endif + } *features = f; return countof(f); }