tool_parsecfg: detect and error on recursive --config use

The config file parser now has a maximum level of inclusions allowed (5)
to detect and prevent recursive inclusions of itself leading to badness.

Bonus: clean up return code handling from the config parser.

Test 774 verifies
Closes #19168

diff --git a/src/tool_getparam.c b/src/tool_getparam.c
index 0cff5b558d9b539f2087cfb28b1f5691d2017c5f..5624d3cd761d5de706d242e7c1020fa432081957 100644 (file)
--- a/src/tool_getparam.c
+++ b/src/tool_getparam.c
@@ -2167,7 +2167,8 @@ static ParameterError opt_bool(struct OperationConfig *config,
 /* opt_file handles file options */
 static ParameterError opt_file(struct OperationConfig *config,
                                const struct LongShort *a,
-                               const char *nextarg)
+                               const char *nextarg,
+                               int max_recursive)
 {
   ParameterError err = PARAM_OK;
   if((nextarg[0] == '-') && nextarg[1]) {
@@ -2189,9 +2190,13 @@ static ParameterError opt_file(struct OperationConfig *config,
     GetFileAndPassword(nextarg, &config->cert, &config->key_passwd);
     break;
   case C_CONFIG: /* --config */
-    if(parseconfig(nextarg)) {
-      errorf("cannot read config from '%s'", nextarg);
-      err = PARAM_READ_ERROR;
+    if(--max_recursive < 0) {
+      errorf("Max config file recursion level reached (%u)",
+             CONFIG_MAX_LEVELS);
+      err = PARAM_BAD_USE;
+    }
+    else {
+      err = parseconfig(nextarg, max_recursive);
     }
     break;
   case C_CRLFILE: /* --crlfile */
@@ -2831,7 +2836,8 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */
                             const char *nextarg,    /* NULL if unset */
                             bool *usedarg,    /* set to TRUE if the arg
                                                  has been used */
-                            struct OperationConfig *config)
+                            struct OperationConfig *config,
+                            int max_recursive)
 {
   const char *parse = NULL;
   bool longopt = FALSE;
@@ -2962,7 +2968,7 @@ ParameterError getparameter(const char *flag, /* f or -long-flag */
               "Maybe ASCII was intended?", nextarg);
       }
       if(ARGTYPE(a->desc) == ARG_FILE)
-        err = opt_file(config, a, nextarg);
+        err = opt_file(config, a, nextarg, max_recursive);
       else /* if(ARGTYPE(a->desc) == ARG_STRG) */
         err = opt_string(config, a, nextarg);
       if(a->desc & ARG_CLEAR)
@@ -3020,7 +3026,8 @@ ParameterError parse_args(int argc, argv_item_t argv[])
           }
         }
 
-        result = getparameter(orig_opt, nextarg, &passarg, config);
+        result = getparameter(orig_opt, nextarg, &passarg, config,
+                              CONFIG_MAX_LEVELS);
 
         unicodefree(nextarg);
         config = global->last;
@@ -3056,7 +3063,7 @@ ParameterError parse_args(int argc, argv_item_t argv[])
       bool used;
 
       /* Just add the URL please */
-      result = getparameter("--url", orig_opt, &used, config);
+      result = getparameter("--url", orig_opt, &used, config, 0);
     }
 
     if(!result) {

diff --git a/src/tool_getparam.h b/src/tool_getparam.h
index 6b97b9c11f45806f00d11f5d1bf48e9dd0edbcd7..6b37cc50ebe72d7460482cc3aaf94a674361fe45 100644 (file)
--- a/src/tool_getparam.h
+++ b/src/tool_getparam.h
@@ -335,7 +335,6 @@ struct LongShort {
 
 typedef enum {
   PARAM_OK = 0,
-  PARAM_OPTION_AMBIGUOUS,
   PARAM_OPTION_UNKNOWN,
   PARAM_REQUIRES_PARAMETER,
   PARAM_BAD_USE,
@@ -358,6 +357,7 @@ typedef enum {
   PARAM_EXPAND_ERROR, /* --expand problem */
   PARAM_BLANK_STRING,
   PARAM_VAR_SYNTAX, /* --variable syntax error */
+  PARAM_RECURSION,
   PARAM_LAST
 } ParameterError;
 
@@ -368,7 +368,8 @@ const struct LongShort *findshortopt(char letter);
 
 ParameterError getparameter(const char *flag, const char *nextarg,
                             bool *usedarg,
-                            struct OperationConfig *config);
+                            struct OperationConfig *config,
+                            int max_recursive);
 
 #ifdef UNITTESTS
 void parse_cert_parameter(const char *cert_parameter,

diff --git a/src/tool_helpers.c b/src/tool_helpers.c
index fbf3f1c932e059f2e4564c832c4961b017ef5831..b36bd4af1d87b6f7fd954dc2f411cd41f0bb95e6 100644 (file)
--- a/src/tool_helpers.c
+++ b/src/tool_helpers.c
@@ -40,8 +40,6 @@ const char *param2text(ParameterError error)
     return "had unsupported trailing garbage";
   case PARAM_OPTION_UNKNOWN:
     return "is unknown";
-  case PARAM_OPTION_AMBIGUOUS:
-    return "is ambiguous";
   case PARAM_REQUIRES_PARAMETER:
     return "requires parameter";
   case PARAM_BAD_USE:

diff --git a/src/tool_operate.c b/src/tool_operate.c
index 0c03114d407261afa4c71e3c8c9557f3e51f3d4c..1c003517297e3ff1ff5c96334114d6d37742b93d 100644 (file)
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -2221,7 +2221,7 @@ CURLcode operate(int argc, argv_item_t argv[])
   if((argc == 1) ||
      (first_arg && strncmp(first_arg, "-q", 2) &&
       strcmp(first_arg, "--disable"))) {
-    parseconfig(NULL); /* ignore possible failure */
+    parseconfig(NULL, CONFIG_MAX_LEVELS); /* ignore possible failure */
 
     /* If we had no arguments then make sure a url was specified in .curlrc */
     if((argc < 2) && (!global->first->url_list)) {

diff --git a/src/tool_parsecfg.c b/src/tool_parsecfg.c
index 5cf3527ce2a51760b0254690dbb7e74ae75db0f5..03f9930c5d8c07763b33714f909d9e5ad44ba663 100644 (file)
--- a/src/tool_parsecfg.c
+++ b/src/tool_parsecfg.c
@@ -81,11 +81,11 @@ static int unslashquote(const char *line, struct dynbuf *param)
 #define MAX_CONFIG_LINE_LENGTH (10*1024*1024)
 
 /* return 0 on everything-is-fine, and non-zero otherwise */
-int parseconfig(const char *filename)
+ParameterError parseconfig(const char *filename, int max_recursive)
 {
   FILE *file = NULL;
   bool usedarg = FALSE;
-  int rc = 0;
+  ParameterError err = PARAM_OK;
   struct OperationConfig *config = global->last;
   char *pathalloc = NULL;
 
@@ -96,7 +96,7 @@ int parseconfig(const char *filename)
       file = curlx_fopen(curlrc, FOPEN_READTEXT);
       if(!file) {
         free(curlrc);
-        return 1;
+        return PARAM_READ_ERROR;
       }
       filename = pathalloc = curlrc;
     }
@@ -133,12 +133,12 @@ int parseconfig(const char *filename)
     curlx_dyn_init(&pbuf, MAX_CONFIG_LINE_LENGTH);
     DEBUGASSERT(filename);
 
-    while(!rc && my_get_line(file, &buf, &fileerror)) {
+    while(!err && my_get_line(file, &buf, &fileerror)) {
       ParameterError res;
       lineno++;
       line = curlx_dyn_ptr(&buf);
       if(!line) {
-        rc = 1; /* out of memory */
+        err = PARAM_NO_MEM; /* out of memory */
         break;
       }
 
@@ -166,9 +166,11 @@ int parseconfig(const char *filename)
       /* the parameter starts here (unless quoted) */
       if(*line == '\"') {
         /* quoted parameter, do the quote dance */
-        rc = unslashquote(++line, &pbuf);
-        if(rc)
+        int rc = unslashquote(++line, &pbuf);
+        if(rc) {
+          err = PARAM_BAD_USE;
           break;
+        }
         param = curlx_dyn_len(&pbuf) ? curlx_dyn_ptr(&pbuf) : CURL_UNCONST("");
       }
       else {
@@ -206,7 +208,7 @@ int parseconfig(const char *filename)
 #ifdef DEBUG_CONFIG
       curl_mfprintf(tool_stderr, "PARAM: \"%s\"\n",(param ? param : "(null)"));
 #endif
-      res = getparameter(option, param, &usedarg, config);
+      res = getparameter(option, param, &usedarg, config, max_recursive);
       config = global->last;
 
       if(!res && param && *param && !usedarg)
@@ -240,10 +242,12 @@ int parseconfig(const char *filename)
            res != PARAM_VERSION_INFO_REQUESTED &&
            res != PARAM_ENGINES_REQUESTED &&
            res != PARAM_CA_EMBED_REQUESTED) {
-          const char *reason = param2text(res);
-          errorf("%s:%d: '%s' %s",
-                 filename, lineno, option, reason);
-          rc = (int)res;
+          /* only show error in the first level config call */
+          if(max_recursive == CONFIG_MAX_LEVELS) {
+            const char *reason = param2text(res);
+            errorf("%s:%d: '%s' %s", filename, lineno, option, reason);
+          }
+          err = res;
         }
       }
     }
@@ -252,13 +256,16 @@ int parseconfig(const char *filename)
     if(file != stdin)
       curlx_fclose(file);
     if(fileerror)
-      rc = 1;
+      err = PARAM_READ_ERROR;
   }
   else
-    rc = 1; /* could not open the file */
+    err = PARAM_READ_ERROR; /* could not open the file */
+
+  if((err == PARAM_READ_ERROR) && filename)
+    errorf("cannot read config from '%s'", filename);
 
   free(pathalloc);
-  return rc;
+  return err;
 }
 
 

diff --git a/src/tool_parsecfg.h b/src/tool_parsecfg.h
index 46977d8eddc952cdfc0e5d7939898eb846b74355..3aad9bdd9cbd3699c4c96c42371eec9b49ffc328 100644 (file)
--- a/src/tool_parsecfg.h
+++ b/src/tool_parsecfg.h
@@ -25,7 +25,9 @@
  ***************************************************************************/
 #include "tool_setup.h"
 
-int parseconfig(const char *filename);
+/* only allow this many levels of recursive --config use */
+#define CONFIG_MAX_LEVELS 5
+ParameterError parseconfig(const char *filename, int max_recursive);
 bool my_get_line(FILE *fp, struct dynbuf *db, bool *error);
 
 #endif /* HEADER_CURL_TOOL_PARSECFG_H */

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 9984e92839f833e93ea21f2ffdcd18d8156c7d49..21d20151d37510ba571f3aa662c01636c390dd0b 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -110,7 +110,7 @@ test736 test737 test738 test739 test740 test741 test742 test743 test744 \
 test745 test746 test747 test748 test749 test750 test751 test752 test753 \
 test754 test755 test756 test757 test758 test759 test760 test761 test762 \
 test763 test764 test765 test766 test767 test768 test769 test770 test771 \
-test772 test773 \
+test772 test773 test774 \
 test780 test781 test782 test783 test784 test785 test786 test787 test788 \
 test789 test790 test791 test792 test793 test794         test796 test797 \
 \

diff --git a/tests/data/test2080 b/tests/data/test2080
index d49bd5c453cc68f591654bf839b6f49ada9cf923..44d16b6ee3315a3dea961b4e595fc92434322030 100644 (file)
--- a/tests/data/test2080
+++ b/tests/data/test2080
@@ -30,7 +30,7 @@ config file with overly long option
 <verify>
 # the used option in the config file is too long
 <errorcode>
-26
+2
 </errorcode>
 </verify>
 </testcase>

diff --git a/tests/data/test462 b/tests/data/test462
index fdbce3b46210f775fb83d2d37df9ed83fdf2d3bd..3fbcae7313f3cfada5e34dadcd7a1079cb7412a5 100644 (file)
--- a/tests/data/test462
+++ b/tests/data/test462
@@ -30,7 +30,7 @@ http://%HOSTIP:%HTTPPORT/%TESTNUMBER -K %LOGDIR/cmd
 # Verify data after the test has been "shot"
 <verify>
 <errorcode>
-26
+2
 </errorcode>
 </verify>
 </testcase>

diff --git a/tests/data/test774 b/tests/data/test774
new file mode 100644 (file)
index 0000000..758ad0d
--- /dev/null
+++ b/tests/data/test774
@@ -0,0 +1,25 @@
+<testcase>
+<info>
+<keywords>
+--config
+</keywords>
+</info>
+
+<client>
+<name>
+config file recursively including itself
+</name>
+<file name="%LOGDIR/cmd">
+--config %LOGDIR/cmd
+</file>
+<command>
+http://%HOSTIP:%HTTPPORT/ -K %LOGDIR/cmd
+</command>
+</client>
+
+<verify>
+<errorcode>
+2
+</errorcode>
+</verify>
+</testcase>