From 495ad3d82aff125f237f370a70882b97843edb6f Mon Sep 17 00:00:00 2001 From: Alexander Sosedkin Date: Mon, 14 Feb 2022 12:44:57 +0100 Subject: [PATCH 1/3] lib/priority: split up update_system_wide_priority_string This is done in preparation for deferring priority string evaluation. Signed-off-by: Alexander Sosedkin --- lib/priority.c | 77 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/lib/priority.c b/lib/priority.c index e7698ba7eb..755729da18 100644 --- a/lib/priority.c +++ b/lib/priority.c @@ -1735,110 +1735,127 @@ static int cfg_ini_handler(void *_ctx, const char *section, const char *name, co return 1; } -static int -update_system_wide_priority_string(void) +static int /* not locking system_wide_config */ +construct_system_wide_priority_string(gnutls_buffer_st* buf) { - gnutls_buffer_st buf; int ret; size_t i; - _gnutls_buffer_init(&buf); + _gnutls_buffer_init(buf); - ret = _gnutls_buffer_append_str(&buf, "NONE"); + ret = _gnutls_buffer_append_str(buf, "NONE"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } for (i = 0; system_wide_config.kxs[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+"); + ret = _gnutls_buffer_append_str(buf, ":+"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_kx_get_name(system_wide_config.kxs[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } for (i = 0; system_wide_config.groups[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+GROUP-"); + ret = _gnutls_buffer_append_str(buf, ":+GROUP-"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_group_get_name(system_wide_config.groups[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } for (i = 0; system_wide_config.ciphers[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+"); + ret = _gnutls_buffer_append_str(buf, ":+"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_cipher_get_name(system_wide_config.ciphers[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } for (i = 0; system_wide_config.macs[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+"); + ret = _gnutls_buffer_append_str(buf, ":+"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_mac_get_name(system_wide_config.macs[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } for (i = 0; system_wide_config.sigs[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+SIGN-"); + ret = _gnutls_buffer_append_str(buf, ":+SIGN-"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_sign_get_name(system_wide_config.sigs[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } for (i = 0; system_wide_config.versions[i] != 0; i++) { - ret = _gnutls_buffer_append_str(&buf, ":+VERS-"); + ret = _gnutls_buffer_append_str(buf, ":+VERS-"); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } - ret = _gnutls_buffer_append_str(&buf, + ret = _gnutls_buffer_append_str(buf, gnutls_protocol_get_name(system_wide_config.versions[i])); if (ret < 0) { - _gnutls_buffer_clear(&buf); + _gnutls_buffer_clear(buf); return ret; } } + return 0; +} + +static int /* not locking system_wide_config */ +update_system_wide_priority_string(void) +{ + /* doesn't do locking, _gnutls_update_system_priorities does */ + gnutls_buffer_st buf; + int ret; + + ret = construct_system_wide_priority_string(&buf); + if (ret < 0) { + _gnutls_debug_log("cfg: unable to construct " + "system-wide priority string: %s", + gnutls_strerror(ret)); + _gnutls_buffer_clear(&buf); + return ret; + } gnutls_free(system_wide_config.priority_string); system_wide_config.priority_string = gnutls_strdup((char *)buf.data); -- 2.34.1 From a74633975e97e491e1e1cdf12416ce160699b703 Mon Sep 17 00:00:00 2001 From: Alexander Sosedkin Date: Mon, 14 Feb 2022 13:48:37 +0100 Subject: [PATCH 2/3] lib/priority: defer setting system-wide priority string Signed-off-by: Alexander Sosedkin --- lib/global.c | 2 +- lib/global.h | 2 +- lib/priority.c | 112 ++++++++++++++++++++++++++++--------------------- 3 files changed, 66 insertions(+), 50 deletions(-) diff --git a/lib/global.c b/lib/global.c index 65c0b81709..faa7f0afb2 100644 --- a/lib/global.c +++ b/lib/global.c @@ -365,7 +365,7 @@ static int _gnutls_global_init(unsigned constructor) _gnutls_fips_mode_reset_zombie(); } #endif - _gnutls_load_system_priorities(); + _gnutls_prepare_to_load_system_priorities(); _gnutls_switch_lib_state(LIB_STATE_OPERATIONAL); ret = 0; diff --git a/lib/global.h b/lib/global.h index e30187e7ad..16fde08b5c 100644 --- a/lib/global.h +++ b/lib/global.h @@ -46,7 +46,7 @@ extern void gnutls_crypto_deinit(void); extern void _gnutls_tpm_global_deinit(void); extern void _gnutls_nss_keylog_deinit(void); -extern void _gnutls_load_system_priorities(void); +extern void _gnutls_prepare_to_load_system_priorities(void); extern void _gnutls_unload_system_priorities(void); #endif /* GNUTLS_LIB_GLOBAL_H */ diff --git a/lib/priority.c b/lib/priority.c index 755729da18..4faf96fabf 100644 --- a/lib/priority.c +++ b/lib/priority.c @@ -1864,11 +1864,12 @@ update_system_wide_priority_string(void) return 0; } -static int _gnutls_update_system_priorities(void) +static int _gnutls_update_system_priorities(bool defer_system_wide) { int ret, err = 0; struct stat sb; FILE *fp; + gnutls_buffer_st buf; struct ini_ctx ctx; ret = gnutls_rwlock_rdlock(&system_wide_config_rwlock); @@ -1883,10 +1884,12 @@ static int _gnutls_update_system_priorities(void) } if (system_priority_file_loaded && - sb.st_mtime == system_priority_last_mod) { + system_priority_last_mod == sb.st_mtime) { _gnutls_debug_log("cfg: system priority %s has not changed\n", system_priority_file); - goto out; + if (system_wide_config.priority_string) { + goto out; /* nothing to do */ + } } (void)gnutls_rwlock_unlock(&system_wide_config_rwlock); @@ -1896,54 +1899,71 @@ static int _gnutls_update_system_priorities(void) return gnutls_assert_val(ret); } - /* Another thread has successfully updated the system wide config (with - * the same modification time as checked above), while upgrading to - * write lock; no need to reload. + /* Another thread could have successfully re-read system-wide config, + * skip re-reading if the mtime it has used is exactly the same. */ - if (system_priority_file_loaded && - system_priority_last_mod == sb.st_mtime) { - goto out; + if (system_priority_file_loaded) { + system_priority_file_loaded = + (system_priority_last_mod == sb.st_mtime); } - system_priority_file_loaded = 0; - _name_val_array_clear(&system_wide_config.priority_strings); + if (!system_priority_file_loaded) { + _name_val_array_clear(&system_wide_config.priority_strings); - gnutls_free(system_wide_config.priority_string); - system_wide_config.priority_string = NULL; + gnutls_free(system_wide_config.priority_string); + system_wide_config.priority_string = NULL; - fp = fopen(system_priority_file, "re"); - if (fp == NULL) { - _gnutls_debug_log("cfg: unable to open: %s: %d\n", - system_priority_file, errno); - goto out; - } - /* Parsing the configuration file needs to be done in 2 phases: first - * parsing the [global] section and then the other sections, because the - * [global] section modifies the parsing behavior. - */ - memset(&ctx, 0, sizeof(ctx)); - err = ini_parse_file(fp, global_ini_handler, &ctx); - if (!err) { - if (fseek(fp, 0L, SEEK_SET) < 0) { - _gnutls_debug_log("cfg: unable to rewind: %s\n", - system_priority_file); - if (fail_on_invalid_config) - exit(1); + fp = fopen(system_priority_file, "re"); + if (fp == NULL) { + _gnutls_debug_log("cfg: unable to open: %s: %d\n", + system_priority_file, errno); + goto out; } - err = ini_parse_file(fp, cfg_ini_handler, &ctx); - } - fclose(fp); - if (err) { + /* Parsing the configuration file needs to be done in 2 phases: + * first parsing the [global] section + * and then the other sections, + * because the [global] section modifies the parsing behavior. + */ + memset(&ctx, 0, sizeof(ctx)); + err = ini_parse_file(fp, global_ini_handler, &ctx); + if (!err) { + if (fseek(fp, 0L, SEEK_SET) < 0) { + _gnutls_debug_log("cfg: unable to rewind: %s\n", + system_priority_file); + if (fail_on_invalid_config) + exit(1); + } + err = ini_parse_file(fp, cfg_ini_handler, &ctx); + } + fclose(fp); + if (err) { + ini_ctx_deinit(&ctx); + _gnutls_debug_log("cfg: unable to parse: %s: %d\n", + system_priority_file, err); + goto out; + } + cfg_apply(&system_wide_config, &ctx); ini_ctx_deinit(&ctx); - _gnutls_debug_log("cfg: unable to parse: %s: %d\n", - system_priority_file, err); - goto out; + _gnutls_debug_log("cfg: loaded system config %s mtime %lld\n", + system_priority_file, + (unsigned long long)sb.st_mtime); + } - cfg_apply(&system_wide_config, &ctx); - ini_ctx_deinit(&ctx); if (system_wide_config.allowlisting) { - ret = update_system_wide_priority_string(); + if (defer_system_wide) { + /* try constructing a priority string, + * but don't apply it yet, at this point + * we're only interested in whether we can */ + ret = construct_system_wide_priority_string(&buf); + _gnutls_buffer_clear(&buf); + _gnutls_debug_log("cfg: deferred setting " + "system-wide priority string\n"); + } else { + ret = update_system_wide_priority_string(); + _gnutls_debug_log("cfg: finalized " + "system-wide priority string\n"); + } if (ret < 0) { _gnutls_debug_log("cfg: unable to build priority string: %s\n", gnutls_strerror(ret)); @@ -1953,10 +1973,6 @@ static int _gnutls_update_system_priorities(void) } } - _gnutls_debug_log("cfg: loaded system priority %s mtime %lld\n", - system_priority_file, - (unsigned long long)sb.st_mtime); - system_priority_file_loaded = 1; system_priority_last_mod = sb.st_mtime; @@ -1970,7 +1986,7 @@ static int _gnutls_update_system_priorities(void) return ret; } -void _gnutls_load_system_priorities(void) +void _gnutls_prepare_to_load_system_priorities(void) { const char *p; int ret; @@ -1983,7 +1999,7 @@ void _gnutls_load_system_priorities(void) if (p != NULL && p[0] == '1' && p[1] == 0) fail_on_invalid_config = 1; - ret = _gnutls_update_system_priorities(); + ret = _gnutls_update_system_priorities(true /* defer_system_wide */); if (ret < 0) { _gnutls_debug_log("failed to update system priorities: %s\n", gnutls_strerror(ret)); @@ -2050,7 +2066,7 @@ char *_gnutls_resolve_priorities(const char* priorities) /* Always try to refresh the cached data, to allow it to be * updated without restarting all applications. */ - ret = _gnutls_update_system_priorities(); + ret = _gnutls_update_system_priorities(false /* defer_system_wide */); if (ret < 0) { _gnutls_debug_log("failed to update system priorities: %s\n", gnutls_strerror(ret)); -- 2.34.1 From 52d5c8627165ff9bc0e99564b772f178ad1f80dd Mon Sep 17 00:00:00 2001 From: Alexander Sosedkin Date: Mon, 21 Feb 2022 18:19:25 +0100 Subject: [PATCH 3/3] lib/algorithms: add UB warnings on late allowlisting API invocations Signed-off-by: Alexander Sosedkin --- lib/algorithms/ecc.c | 3 +++ lib/algorithms/mac.c | 3 +++ lib/algorithms/protocols.c | 3 +++ lib/algorithms/sign.c | 6 ++++++ 4 files changed, 15 insertions(+) diff --git a/lib/algorithms/ecc.c b/lib/algorithms/ecc.c index 736e5dd07f..52ae1db0e4 100644 --- a/lib/algorithms/ecc.c +++ b/lib/algorithms/ecc.c @@ -389,6 +389,9 @@ void _gnutls_ecc_curve_mark_disabled_all(void) * enabled through the allowlisting mode in the configuration file, or * when the setting is modified with a prior call to this function. * + * This function must be called prior to any session priority setting functions; + * otherwise the behavior is undefined. + * * Returns: 0 on success or negative error code otherwise. * * Since: 3.7.3 diff --git a/lib/algorithms/mac.c b/lib/algorithms/mac.c index a2c66e76bb..166d51d552 100644 --- a/lib/algorithms/mac.c +++ b/lib/algorithms/mac.c @@ -332,6 +332,9 @@ void _gnutls_digest_mark_insecure_all(void) * through the allowlisting mode in the configuration file, or when * the setting is modified with a prior call to this function. * + * This function must be called prior to any session priority setting functions; + * otherwise the behavior is undefined. + * * Since: 3.7.3 */ int diff --git a/lib/algorithms/protocols.c b/lib/algorithms/protocols.c index b0f3e0bc30..64c86eba3c 100644 --- a/lib/algorithms/protocols.c +++ b/lib/algorithms/protocols.c @@ -237,6 +237,9 @@ void _gnutls_version_mark_revertible_all(void) * enabled through the allowlisting mode in the configuration file, or * when the setting is modified with a prior call to this function. * + * This function must be called prior to any session priority setting functions; + * otherwise the behavior is undefined. + * * Returns: 0 on success or negative error code otherwise. * * Since: 3.7.3 diff --git a/lib/algorithms/sign.c b/lib/algorithms/sign.c index 543bd19bb5..06abdb4cf8 100644 --- a/lib/algorithms/sign.c +++ b/lib/algorithms/sign.c @@ -516,6 +516,9 @@ void _gnutls_sign_mark_insecure_all(hash_security_level_t level) * use in certificates. Use gnutls_sign_set_secure_for_certs() to * mark it secure as well for certificates. * + * This function must be called prior to any session priority setting functions; + * otherwise the behavior is undefined. + * * Since: 3.7.3 */ int @@ -560,6 +563,9 @@ gnutls_sign_set_secure(gnutls_sign_algorithm_t sign, * for the use in certificates. Use gnutls_sign_set_secure() to mark * it insecure for any uses. * + * This function must be called prior to any session priority setting functions; + * otherwise the behavior is undefined. + * * Since: 3.7.3 */ int -- 2.34.1