Detect and prevent recursive config parsing

If a malformed config file is provided such as the following:

openssl_conf = openssl_init
[openssl_init]
providers = provider_sect
[provider_sect]
 = provider_sect

The config parsing library will crash overflowing the stack, as it
recursively parses the same provider_sect ad nauseum.

Prevent this by maintaing a list of visited nodes as we recurse through
referenced sections, and erroring out in the event we visit any given
section node more than once.

Note, adding the test for this revealed that our diagnostic code
inadvertently pops recorded errors off the error stack because
provider_conf_load returns success even in the event that a
configuration parse failed. The call path to provider_conf_load has been
updated in this commit to address that shortcoming, allowing recorded
errors to be visibile to calling applications.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22898)

(cherry picked from commit 682fd21afb5428b5716e62eaefb09a7419f9cfd7)

diff --git a/crypto/conf/conf_err.c b/crypto/conf/conf_err.c
index 68ee90b97055e77a2f7f2b2cdf4999703170c5b6..9f1309c507c5bd36f4149c17c1c77efbf81068fe 100644 (file)
--- 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 69e4f61aa1801f69131898000c739ce7881af80c..6a29ab5ccf4385f10c99ea4efb1cd5c92d387e04 100644 (file)
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -403,6 +403,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 058fb588371be361244f5b059e72bc63074e41e9..403166d7e923715141a3783cc3e2c93310d40e68 100644 (file)
--- 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);
@@ -270,19 +340,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)
@@ -305,7 +379,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 cb367e4f32a04d6a3c2d42fed9b939e283faca67..fc9645127d12d44d37739635a15f0c3a76518cfe 100644 (file)
--- 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 496e2e1efd66a45bc7715e9856bb87a2db805b8d..a8798e792412bac701497177d0e5457f0c96339b 100644 (file)
--- 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/prov_config_test.c b/test/prov_config_test.c
index 4b04211fa47ef32b9e5d5548b00be21bc742b99f..b44ec78d8d24b4e068642a80c82f89e94bb86cc3 100644 (file)
--- a/test/prov_config_test.c
+++ b/test/prov_config_test.c
@@ -8,9 +8,11 @@
  */
 
 #include <openssl/evp.h>
+#include <openssl/conf.h>
 #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 f97a26dbe9543c6e760d3782f96c00dc83b6337b..7f6350fd84e1161eb1b5135d08d49c0dc180333d 100644 (file)
--- 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 (file)
index 0000000..505733a
--- /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