diff --git a/crypto/conf/conf_err.c b/crypto/conf/conf_err.c index 68ee90b970..9f1309c507 100644 --- a/crypto/conf/conf_err.c +++ b/crypto/conf/conf_err.c @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 1995-2021 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2023 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -41,6 +41,8 @@ static const ERR_STRING_DATA CONF_str_reasons[] = { "openssl conf references missing section"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_RECURSIVE_DIRECTORY_INCLUDE), "recursive directory include"}, + {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_RECURSIVE_SECTION_REFERENCE), + "recursive section reference"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_RELATIVE_PATH), "relative path"}, {ERR_PACK(ERR_LIB_CONF, 0, CONF_R_SSL_COMMAND_SECTION_EMPTY), "ssl command section empty"}, diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 8e6d9ddcae..eaa568bfa8 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -425,6 +425,7 @@ CONF_R_NUMBER_TOO_LARGE:121:number too large CONF_R_OPENSSL_CONF_REFERENCES_MISSING_SECTION:124:\ openssl conf references missing section CONF_R_RECURSIVE_DIRECTORY_INCLUDE:111:recursive directory include +CONF_R_RECURSIVE_SECTION_REFERENCE:126:recursive section reference CONF_R_RELATIVE_PATH:125:relative path CONF_R_SSL_COMMAND_SECTION_EMPTY:117:ssl command section empty CONF_R_SSL_COMMAND_SECTION_NOT_FOUND:118:ssl command section not found diff --git a/crypto/provider_conf.c b/crypto/provider_conf.c index d8454b7941..a4a5c5122b 100644 --- a/crypto/provider_conf.c +++ b/crypto/provider_conf.c @@ -64,13 +64,22 @@ static const char *skip_dot(const char *name) return name; } -static int provider_conf_params(OSSL_PROVIDER *prov, - OSSL_PROVIDER_INFO *provinfo, - const char *name, const char *value, - const CONF *cnf) +/* + * Parse the provider params section + * Returns: + * 1 for success + * 0 for non-fatal errors + * < 0 for fatal errors + */ +static int provider_conf_params_internal(OSSL_PROVIDER *prov, + OSSL_PROVIDER_INFO *provinfo, + const char *name, const char *value, + const CONF *cnf, + STACK_OF(OPENSSL_CSTRING) *visited) { STACK_OF(CONF_VALUE) *sect; int ok = 1; + int rc = 0; sect = NCONF_get_section(cnf, value); if (sect != NULL) { @@ -80,6 +89,25 @@ static int provider_conf_params(OSSL_PROVIDER *prov, OSSL_TRACE1(CONF, "Provider params: start section %s\n", value); + /* + * Check to see if the provided section value has already + * been visited. If it has, then we have a recursive lookup + * in the configuration which isn't valid. As such we should error + * out + */ + for (i = 0; i < sk_OPENSSL_CSTRING_num(visited); i++) { + if (sk_OPENSSL_CSTRING_value(visited, i) == value) { + ERR_raise(ERR_LIB_CONF, CONF_R_RECURSIVE_SECTION_REFERENCE); + return -1; + } + } + + /* + * We've not visited this node yet, so record it on the stack + */ + if (!sk_OPENSSL_CSTRING_push(visited, value)) + return -1; + if (name != NULL) { OPENSSL_strlcpy(buffer, name, sizeof(buffer)); OPENSSL_strlcat(buffer, ".", sizeof(buffer)); @@ -89,14 +117,20 @@ static int provider_conf_params(OSSL_PROVIDER *prov, for (i = 0; i < sk_CONF_VALUE_num(sect); i++) { CONF_VALUE *sectconf = sk_CONF_VALUE_value(sect, i); - if (buffer_len + strlen(sectconf->name) >= sizeof(buffer)) - return 0; + if (buffer_len + strlen(sectconf->name) >= sizeof(buffer)) { + sk_OPENSSL_CSTRING_pop(visited); + return -1; + } buffer[buffer_len] = '\0'; OPENSSL_strlcat(buffer, sectconf->name, sizeof(buffer)); - if (!provider_conf_params(prov, provinfo, buffer, sectconf->value, - cnf)) - return 0; + rc = provider_conf_params_internal(prov, provinfo, buffer, + sectconf->value, cnf, visited); + if (rc < 0) { + sk_OPENSSL_CSTRING_pop(visited); + return rc; + } } + sk_OPENSSL_CSTRING_pop(visited); OSSL_TRACE1(CONF, "Provider params: finish section %s\n", value); } else { @@ -110,6 +144,33 @@ static int provider_conf_params(OSSL_PROVIDER *prov, return ok; } +/* + * recursively parse the provider configuration section + * of the config file. + * Returns + * 1 on success + * 0 on non-fatal error + * < 0 on fatal errors + */ +static int provider_conf_params(OSSL_PROVIDER *prov, + OSSL_PROVIDER_INFO *provinfo, + const char *name, const char *value, + const CONF *cnf) +{ + int rc; + STACK_OF(OPENSSL_CSTRING) *visited = sk_OPENSSL_CSTRING_new_null(); + + if (visited == NULL) + return -1; + + rc = provider_conf_params_internal(prov, provinfo, name, + value, cnf, visited); + + sk_OPENSSL_CSTRING_free(visited); + + return rc; +} + static int prov_already_activated(const char *name, STACK_OF(OSSL_PROVIDER) *activated) { @@ -130,6 +191,13 @@ static int prov_already_activated(const char *name, return 0; } +/* + * Attempt to activate a provider + * Returns: + * 1 on successful activation + * 0 on failed activation for non-fatal error + * < 0 on failed activation for fatal errors + */ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, const char *value, const char *path, int soft, const CONF *cnf) @@ -141,7 +209,7 @@ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, if (pcgbl == NULL || !CRYPTO_THREAD_write_lock(pcgbl->lock)) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR); - return 0; + return -1; } if (!prov_already_activated(name, pcgbl->activated_providers)) { /* @@ -154,7 +222,7 @@ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, if (!ossl_provider_disable_fallback_loading(libctx)) { CRYPTO_THREAD_unlock(pcgbl->lock); ERR_raise(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR); - return 0; + return -1; } prov = ossl_provider_find(libctx, name, 1); if (prov == NULL) @@ -163,7 +231,7 @@ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, CRYPTO_THREAD_unlock(pcgbl->lock); if (soft) ERR_clear_error(); - return 0; + return (soft == 0) ? -1 : 0; } if (path != NULL) @@ -171,7 +239,7 @@ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, ok = provider_conf_params(prov, NULL, NULL, value, cnf); - if (ok) { + if (ok == 1) { if (!ossl_provider_activate(prov, 1, 0)) { ok = 0; } else if (!ossl_provider_add_to_store(prov, &actual, 0)) { @@ -195,7 +263,8 @@ static int provider_conf_activate(OSSL_LIB_CTX *libctx, const char *name, } } } - if (!ok) + + if (ok <= 0) ossl_provider_free(prov); } CRYPTO_THREAD_unlock(pcgbl->lock); @@ -212,6 +281,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name, const char *path = NULL; long activate = 0; int ok = 0; + int added = 0; name = skip_dot(name); OSSL_TRACE1(CONF, "Configuring provider %s\n", name); @@ -294,19 +364,23 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name, } if (ok) ok = provider_conf_params(NULL, &entry, NULL, value, cnf); - if (ok && (entry.path != NULL || entry.parameters != NULL)) + if (ok >= 1 && (entry.path != NULL || entry.parameters != NULL)) { ok = ossl_provider_info_add_to_store(libctx, &entry); - if (!ok || (entry.path == NULL && entry.parameters == NULL)) { - ossl_provider_info_clear(&entry); + added = 1; } - + if (added == 0) + ossl_provider_info_clear(&entry); } /* - * Even if ok is 0, we still return success. Failure to load a provider is - * not fatal. We want to continue to load the rest of the config file. + * Provider activation returns a tristate: + * 1 for successful activation + * 0 for non-fatal activation failure + * < 0 for fatal activation failure + * We return success (1) for activation, (1) for non-fatal activation + * failure, and (0) for fatal activation failure */ - return 1; + return ok >= 0; } static int provider_conf_init(CONF_IMODULE *md, const CONF *cnf) @@ -329,7 +403,7 @@ static int provider_conf_init(CONF_IMODULE *md, const CONF *cnf) for (i = 0; i < sk_CONF_VALUE_num(elist); i++) { cval = sk_CONF_VALUE_value(elist, i); if (!provider_conf_load(NCONF_get0_libctx((CONF *)cnf), - cval->name, cval->value, cnf)) + cval->name, cval->value, cnf)) return 0; } diff --git a/include/crypto/conferr.h b/include/crypto/conferr.h index cb367e4f32..fc9645127d 100644 --- a/include/crypto/conferr.h +++ b/include/crypto/conferr.h @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 2020-2021 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 2020-2023 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy diff --git a/include/openssl/conferr.h b/include/openssl/conferr.h index 496e2e1efd..a8798e7924 100644 --- a/include/openssl/conferr.h +++ b/include/openssl/conferr.h @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 1995-2021 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2023 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -38,6 +38,7 @@ # define CONF_R_NUMBER_TOO_LARGE 121 # define CONF_R_OPENSSL_CONF_REFERENCES_MISSING_SECTION 124 # define CONF_R_RECURSIVE_DIRECTORY_INCLUDE 111 +# define CONF_R_RECURSIVE_SECTION_REFERENCE 126 # define CONF_R_RELATIVE_PATH 125 # define CONF_R_SSL_COMMAND_SECTION_EMPTY 117 # define CONF_R_SSL_COMMAND_SECTION_NOT_FOUND 118 diff --git a/test/build.info b/test/build.info index af43407611..88620b86b3 100644 --- a/test/build.info +++ b/test/build.info @@ -60,10 +60,10 @@ IF[{- !$disabled{tests} -}] context_internal_test aesgcmtest params_test evp_pkey_dparams_test \ keymgmt_internal_test hexstr_test provider_status_test defltfips_test \ bio_readbuffer_test user_property_test pkcs7_test upcallstest \ - provfetchtest prov_config_test rand_test ca_internals_test \ - bio_tfo_test membio_test bio_dgram_test list_test fips_version_test \ - x509_test hpke_test pairwise_fail_test nodefltctxtest \ - evp_xof_test x509_load_cert_file_test + provfetchtest prov_config_test rand_test \ + ca_internals_test bio_tfo_test membio_test bio_dgram_test list_test \ + fips_version_test x509_test hpke_test pairwise_fail_test \ + nodefltctxtest evp_xof_test x509_load_cert_file_test IF[{- !$disabled{'rpk'} -}] PROGRAMS{noinst}=rpktest diff --git a/test/prov_config_test.c b/test/prov_config_test.c index 4b04211fa4..b44ec78d8d 100644 --- a/test/prov_config_test.c +++ b/test/prov_config_test.c @@ -8,9 +8,11 @@ */ #include +#include #include "testutil.h" static char *configfile = NULL; +static char *recurseconfigfile = NULL; /* * Test to make sure there are no leaks or failures from loading the config @@ -44,6 +46,30 @@ static int test_double_config(void) return testresult; } +static int test_recursive_config(void) +{ + OSSL_LIB_CTX *ctx = OSSL_LIB_CTX_new(); + int testresult = 0; + unsigned long err; + + if (!TEST_ptr(recurseconfigfile)) + goto err; + + if (!TEST_ptr(ctx)) + goto err; + + if (!TEST_false(OSSL_LIB_CTX_load_config(ctx, recurseconfigfile))) + goto err; + + err = ERR_peek_error(); + /* We expect to get a recursion error here */ + if (ERR_GET_REASON(err) == CONF_R_RECURSIVE_SECTION_REFERENCE) + testresult = 1; + err: + OSSL_LIB_CTX_free(ctx); + return testresult; +} + OPT_TEST_DECLARE_USAGE("configfile\n") int setup_tests(void) @@ -56,6 +82,10 @@ int setup_tests(void) if (!TEST_ptr(configfile = test_get_argument(0))) return 0; + if (!TEST_ptr(recurseconfigfile = test_get_argument(1))) + return 0; + + ADD_TEST(test_recursive_config); ADD_TEST(test_double_config); return 1; } diff --git a/test/recipes/30-test_prov_config.t b/test/recipes/30-test_prov_config.t index f97a26dbe9..7f6350fd84 100644 --- a/test/recipes/30-test_prov_config.t +++ b/test/recipes/30-test_prov_config.t @@ -22,11 +22,14 @@ my $no_fips = disabled('fips') || ($ENV{NO_FIPS} // 0); plan tests => 2; -ok(run(test(["prov_config_test", srctop_file("test", "default.cnf")])), +ok(run(test(["prov_config_test", srctop_file("test", "default.cnf"), + srctop_file("test", "recursive.cnf")])), "running prov_config_test default.cnf"); + SKIP: { skip "Skipping FIPS test in this build", 1 if $no_fips; - ok(run(test(["prov_config_test", srctop_file("test", "fips.cnf")])), + ok(run(test(["prov_config_test", srctop_file("test", "fips.cnf"), + srctop_file("test", "recursive.cnf")])), "running prov_config_test fips.cnf"); } diff --git a/test/recursive.cnf b/test/recursive.cnf new file mode 100644 index 0000000000..505733ae45 --- /dev/null +++ b/test/recursive.cnf @@ -0,0 +1,8 @@ +openssl_conf = openssl_init +config_diagnostics = yes + +[openssl_init] +providers = provider_sect + +[provider_sect] + = provider_sect