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>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from https://github.com/openssl/openssl/pull/23120)

3 files changed, 88 insertions, 19 deletions

diff --git a/crypto/conf/conf_err.c b/crypto/conf/conf_err.c
index 68ee90b970..fc0eee7d2f 100644
--- a/crypto/conf/conf_err.c
+++ b/crypto/conf/conf_err.c
@@ -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 36de321b74..36fe318baf 100644
--- 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 c13c887c3d..9333b8777f 100644
--- a/crypto/provider_conf.c
+++ b/crypto/provider_conf.c
@@ -70,13 +70,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) {
@@ -86,6 +95,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));
@@ -95,14 +123,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 {
@@ -116,6 +150,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)
 {
@@ -146,6 +207,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);
@@ -218,7 +280,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name,
 
             ok = provider_conf_params(prov, NULL, NULL, value, cnf);
 
-            if (ok) {
+            if (ok > 0) {
                 if (!ossl_provider_activate(prov, 1, 0)) {
                     ok = 0;
                 } else if (!ossl_provider_add_to_store(prov, &actual, 0)) {
@@ -242,7 +304,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name,
                     }
                 }
             }
-            if (!ok)
+            if (ok <= 0)
                 ossl_provider_free(prov);
         }
         CRYPTO_THREAD_unlock(pcgbl->lock);
@@ -267,19 +329,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)
@@ -302,7 +368,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;
     }