Polish squid.conf parsing validation

* Updates ato*() functiosn to safer xato*() alternatives provided by
  the Squid compat library. Along with error messages on invalid
  configuration values detected by these.

* Add protection against integer overflow on most options

* Add parse deprecation messages on enable/disable for boolean and
  and trilean options.

* Add 'wrong-value' error messages on most options.

diff --git a/compat/xstrto.cc b/compat/xstrto.cc
index b1b44616739541c8cff88df32b4f803b50ce8e6d..7dd72a8e3d4623f097fe922cd255c6c629b45ef9 100644 (file)
--- a/compat/xstrto.cc
+++ b/compat/xstrto.cc
@@ -87,6 +87,11 @@ xstrtoui(const char *s, char **end, unsigned int *value,
     ret = xstrtoul(s, end, &v, min, max);
     if (value != NULL)
         *value = v;
+
+    if (v != static_cast<unsigned long>(*value)) {
+        return false;
+    }
+
     return ret;
 }
 

diff --git a/src/HelperChildConfig.cc b/src/HelperChildConfig.cc
index 76d2d86e6d9c8155dc314d130afbb9d929da4a0b..c82b9b5d85595fe99d3e42766afe12a147050d16 100644 (file)
--- a/src/HelperChildConfig.cc
+++ b/src/HelperChildConfig.cc
@@ -3,6 +3,7 @@
 #include "Debug.h"
 #include "HelperChildConfig.h"
 #include "globals.h"
+#include "Parsing.h"
 
 #include <string.h>
 
@@ -49,24 +50,27 @@ HelperChildConfig::parseConfig()
         self_destruct();
 
     /* starts with a bare number for the max... back-compatible */
-    n_max = atoi(token);
+    n_max = xatoui(token);
 
-    if (n_max < 1)
+    if (n_max < 1) {
+        debugs(0, DBG_CRITICAL, "ERROR: The maximum number of processes cannot be less than 1.");
         self_destruct();
+    }
 
     /* Parse extension options */
     for (; (token = strtok(NULL, w_space)) ;) {
         if (strncmp(token, "startup=", 8) == 0) {
-            n_startup = atoi(token + 8);
+            n_startup = xatoui(token + 8);
         } else if (strncmp(token, "idle=", 5) == 0) {
-            n_idle = atoi(token + 5);
+            n_idle = xatoui(token + 5);
             if (n_idle < 1) {
                 debugs(0, DBG_CRITICAL, "WARNING OVERIDE: Using idle=0 for helpers causes request failures. Overiding to use idle=1 instead.");
                 n_idle = 1;
             }
         } else if (strncmp(token, "concurrency=", 12) == 0) {
-            concurrency = atoi(token + 12);
+            concurrency = xatoui(token + 12);
         } else {
+            debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Undefined option: " << token << ".");
             self_destruct();
         }
     }

diff --git a/src/Parsing.cc b/src/Parsing.cc
index 4370bf3a46031270fe5b6d5116aeef9472c21e36..ab67afa1986f697060575181daac7342c5cb1a3d 100644 (file)
--- a/src/Parsing.cc
+++ b/src/Parsing.cc
@@ -34,6 +34,8 @@
 #include "cache_cf.h"
 #include "compat/strtoll.h"
 #include "Parsing.h"
+#include "globals.h"
+#include "Debug.h"
 
 /*
  * These functions is the same as atoi/l/f, except that they check for errors
@@ -42,11 +44,18 @@
 double
 xatof(const char *token)
 {
-    char *end;
+    char *end = NULL;
     double ret = strtod(token, &end);
 
-    if (ret == 0 && end == token)
+    if (ret == 0 && end == token) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: No digits were found in the input value '" << token << "'.");
         self_destruct();
+    }
+
+    if (*end) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Invalid value: '" << token << "' is supposed to be a number.");
+        self_destruct();
+    }
 
     return ret;
 }
@@ -54,17 +63,64 @@ xatof(const char *token)
 int
 xatoi(const char *token)
 {
-    return xatol(token);
+    int64_t input = xatoll(token, 10);
+    int ret = (int) input;
+
+    if (input != static_cast<int64_t>(ret)) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is larger than the type 'int'.");
+        self_destruct();
+    }
+
+    return ret;
+}
+
+unsigned int
+xatoui(const char *token)
+{
+    int64_t input = xatoll(token, 10);
+    if (input < 0) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The input value '" << token << "' cannot be less than 0.");
+        self_destruct();
+    }
+
+    unsigned int ret = (unsigned int) input;
+    if (input != static_cast<int64_t>(ret)) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is larger than the type 'unsigned int'.");
+        self_destruct();
+    }
+
+    return ret;
 }
 
 long
 xatol(const char *token)
 {
-    char *end;
-    long ret = strtol(token, &end, 10);
+    int64_t input = xatoll(token, 10);
+    long ret = (long) input;
+
+    if (input != static_cast<int64_t>(ret)) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is larger than the type 'long'.");
+        self_destruct();
+    }
+
+    return ret;
+}
+
+int64_t
+xatoll(const char *token, int base)
+{
+    char *end = NULL;
+    int64_t ret = strtoll(token, &end, base);
 
-    if (end == token || *end)
+    if (end == token) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: No digits were found in the input value '" << token << "'.");
         self_destruct();
+    }
+
+    if (*end) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Invalid value: '" << token << "' is supposed to be a number.");
+        self_destruct();
+    }
 
     return ret;
 }
@@ -74,8 +130,15 @@ xatos(const char *token)
 {
     long port = xatol(token);
 
-    if (port & ~0xFFFF)
+    if (port < 0) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' cannot be less than 0.");
+        self_destruct();
+    }
+
+    if (port & ~0xFFFF) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is larger than the type 'short'.");
         self_destruct();
+    }
 
     return port;
 }
@@ -84,16 +147,17 @@ int64_t
 GetInteger64(void)
 {
     char *token = strtok(NULL, w_space);
-    int64_t i;
 
     if (token == NULL)
         self_destruct();
 
-    i = strtoll(token, NULL, 10);
-
-    return i;
+    return xatoll(token, 10);
 }
 
+/*
+ * This function is different from others (e.g., GetInteger64, GetShort)
+ * because it supports octal and hexadecimal numbers
+ */
 int
 GetInteger(void)
 {
@@ -103,13 +167,46 @@ GetInteger(void)
     if (token == NULL)
         self_destruct();
 
-    // %i honors 0 and 0x prefixes, which are important for things like umask
-    if (sscanf(token, "%i", &i) != 1)
+    // The conversion must honor 0 and 0x prefixes, which are important for things like umask
+    int64_t ret = xatoll(token, 0);
+
+    i = (int) ret;
+    if (ret != static_cast<int64_t>(i)) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is larger than the type 'int'.");
         self_destruct();
+    }
 
     return i;
 }
 
+/*
+ * This function is similar as GetInteger() but the token might contain
+ * the percentage symbol (%) and we check whether the value is in the range
+ * of [0, 100]
+ * So, we accept two types of input: 1. XX% or 2. XX , 0<=XX<=100
+ */
+int
+GetPercentage(void)
+{
+    int p;
+    char *token = strtok(NULL, w_space);
+
+    //if there is a % in the end of the digits, we remove it and go on.
+    char* end = &token[strlen(token)-1];
+    if (*end == '%') {
+        *end = '\0';
+    }
+
+    p = xatoi(token);
+
+    if (p < 0 || p > 100) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: The value '" << token << "' is out of range. A percentage should be within [0, 100].");
+        self_destruct();
+    }
+
+    return p;
+}
+
 unsigned short
 GetShort(void)
 {
@@ -191,8 +288,8 @@ GetHostWithPort(char *token, Ip::Address *ipa)
 
         if (0 == port)
             return false;
-    } else if ((port = strtol(token, &tmp, 10)), !*tmp) {
-        /* port */
+    } else if (strtol(token, &tmp, 10) && !*tmp) {
+        port = xatos(token);
     } else {
         host = token;
         port = 0;

diff --git a/src/Parsing.h b/src/Parsing.h
index 291673b1c610dcf4047a696ee35707e5ab7b16a5..4f9aa075f5d50d39f9b23c5121c251d90fdfd7d8 100644 (file)
--- a/src/Parsing.h
+++ b/src/Parsing.h
@@ -38,7 +38,9 @@
 
 double xatof(const char *token);
 int xatoi(const char *token);
+unsigned int xatoui(const char *token);
 long xatol(const char *token);
+int64_t xatoll(const char *token, int base);
 unsigned short xatos(const char *token);
 
 /**
@@ -49,9 +51,19 @@ int64_t GetInteger64(void);
 /**
  * Parses an integer value.
  * Uses a method that obeys hexadecimal 0xN syntax needed for certain bitmasks.
+ * self_destruct() will be called to abort when invalid tokens are encountered.
  */
 int GetInteger(void);
 
+/**
+ * Parse a percentage value, e.g., 20%.
+ * The behavior of this function is similar as GetInteger().
+ * The difference is that the token might contain '%' as percentage symbol (%),
+ * and we further check whether the value is in the range of [0, 100]
+ * For example, 20% and 20 are both valid tokens, while 101%, 101, -1 are invalid.
+ */
+int GetPercentage(void);
+
 unsigned short GetShort(void);
 
 // on success, returns true and sets *p (if any) to the end of the integer

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index ea918934f37ff5f727e6b36d5cbe2834a30beecd..2fb372ff23d53a5fb4ac72866e62ac97e6550d98 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1017,6 +1017,12 @@ parseTimeLine(time_msec_t * tptr, const char *units,  bool allowMsec)
         self_destruct();
 
     *tptr = static_cast<time_msec_t>(m * d);
+
+    if (static_cast<double>(*tptr) * 2 != m * d * 2) {
+        debugs(3, DBG_CRITICAL, "ERROR: Invalid value '" <<
+               d << " " << token << ": integer overflow (time_msec_t).");
+        self_destruct();
+    }
 }
 
 static uint64_t
@@ -1097,8 +1103,11 @@ parseBytesLine64(int64_t * bptr, const char *units)
 
     *bptr = static_cast<int64_t>(m * d / u);
 
-    if (static_cast<double>(*bptr) * 2 != m * d / u * 2)
+    if (static_cast<double>(*bptr) * 2 != (m * d / u) * 2) {
+        debugs(3, DBG_CRITICAL, "ERROR: Invalid value '" <<
+               d << " " << token << ": integer overflow (int64_t).");
         self_destruct();
+    }
 }
 
 static void
@@ -1141,8 +1150,11 @@ parseBytesLine(size_t * bptr, const char *units)
 
     *bptr = static_cast<size_t>(m * d / u);
 
-    if (static_cast<double>(*bptr) * 2 != m * d / u * 2)
+    if (static_cast<double>(*bptr) * 2 != (m * d / u) * 2) {
+        debugs(3, DBG_CRITICAL, "ERROR: Invalid value '" <<
+               d << " " << token << ": integer overflow (size_t).");
         self_destruct();
+    }
 }
 
 #if !USE_DNSHELPER
@@ -1184,10 +1196,13 @@ parseBytesLineSigned(ssize_t * bptr, const char *units)
         return;
     }
 
-    *bptr = static_cast<size_t>(m * d / u);
+    *bptr = static_cast<ssize_t>(m * d / u);
 
-    if (static_cast<double>(*bptr) * 2 != m * d / u * 2)
+    if (static_cast<double>(*bptr) * 2 != (m * d / u) * 2) {
+        debugs(3, DBG_CRITICAL, "ERROR: Invalid value '" <<
+               d << " " << token << ": integer overflow (ssize_t).");
         self_destruct();
+    }
 }
 #endif
 
@@ -1224,7 +1239,7 @@ static void parseBytesOptionValue(size_t * bptr, const char *units, char const *
     }
 
     *bptr = static_cast<size_t>(m * d / u);
-    if (static_cast<double>(*bptr) * 2 != m * d / u * 2)
+    if (static_cast<double>(*bptr) * 2 != (m * d / u) * 2)
         self_destruct();
 }
 #endif
@@ -2266,7 +2281,7 @@ parse_peer(CachePeer ** head)
             safe_free(p->sslkey);
             p->sslkey = xstrdup(token + 7);
         } else if (strncmp(token, "sslversion=", 11) == 0) {
-            p->sslversion = atoi(token + 11);
+            p->sslversion = xatoi(token + 11);
         } else if (strncmp(token, "ssloptions=", 11) == 0) {
             safe_free(p->ssloptions);
             p->ssloptions = xstrdup(token + 11);
@@ -2597,10 +2612,20 @@ parse_onoff(int *var)
     if (token == NULL)
         self_destruct();
 
-    if (!strcasecmp(token, "on") || !strcasecmp(token, "enable"))
+    if (!strcasecmp(token, "on")) {
         *var = 1;
-    else
+    } else if (!strcasecmp(token, "enable")) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'enable' is deprecated. Please update to use 'on'.");
+        *var = 1;
+    } else if (!strcasecmp(token, "off")) {
+        *var = 0;
+    } else if (!strcasecmp(token, "disable")) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'disable' is deprecated. Please update to use 'off'.");
         *var = 0;
+    } else {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Invalid option: Boolean options can only be 'on' or 'off'.");
+        self_destruct();
+    }
 }
 
 #define free_onoff free_int
@@ -2628,12 +2653,22 @@ parse_tristate(int *var)
     if (token == NULL)
         self_destruct();
 
-    if (!strcasecmp(token, "on") || !strcasecmp(token, "enable"))
+    if (!strcasecmp(token, "on")) {
         *var = 1;
-    else if (!strcasecmp(token, "warn"))
+    } else if (!strcasecmp(token, "enable")) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'enable' is deprecated. Please update to use value 'on'.");
+        *var = 1;
+    } else if (!strcasecmp(token, "warn")) {
         *var = -1;
-    else
+    } else if (!strcasecmp(token, "off")) {
+        *var = 0;
+    } else if (!strcasecmp(token, "disable")) {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'disable' is deprecated. Please update to use value 'off'.");
         *var = 0;
+    } else {
+        debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: Invalid option: Tristate options can only be 'on', 'off', or 'warn'.");
+        self_destruct();
+    }
 }
 
 #define free_tristate free_int
@@ -2757,7 +2792,7 @@ parse_refreshpattern(RefreshPattern ** head)
 
     min = (time_t) (i * 60);   /* convert minutes to seconds */
 
-    i = GetInteger();          /* token: pct */
+    i = GetPercentage();       /* token: pct */
 
     pct = (double) i / 100.0;
 
@@ -2782,7 +2817,7 @@ parse_refreshpattern(RefreshPattern ** head)
         } else if (!strcmp(token, "store-stale")) {
             store_stale = 1;
         } else if (!strncmp(token, "max-stale=", 10)) {
-            max_stale = atoi(token + 10);
+            max_stale = xatoi(token + 10);
 #if USE_HTTP_VIOLATIONS
 
         } else if (!strcmp(token, "override-expire"))
@@ -3223,8 +3258,10 @@ parse_uri_whitespace(int *var)
         *var = URI_WHITESPACE_ENCODE;
     else if (!strcasecmp(token, "chop"))
         *var = URI_WHITESPACE_CHOP;
-    else
+    else {
+        debugs(0, DBG_PARSE_NOTE(2), "ERROR: Invalid option '" << token << "': 'uri_whitespace' accepts 'strip', 'deny', 'allow', 'encode', and 'chop'.");
         self_destruct();
+    }
 }
 
 static void
@@ -3335,8 +3372,10 @@ parse_memcachemode(SquidConfig * config)
     } else if (strcmp(token, "never") == 0) {
         Config.onoff.memory_cache_first = 0;
         Config.onoff.memory_cache_disk = 0;
-    } else
+    } else {
+        debugs(0, DBG_PARSE_NOTE(2), "ERROR: Invalid option '" << token << "': 'memory_cache_mode' accepts 'always', 'disk', 'network', and 'never'.");
         self_destruct();
+    }
 }
 
 static void
@@ -3473,8 +3512,8 @@ parsePortSpecification(AnyP::PortCfg * s, char *token)
         *t = '\0';
         port = xatos(t + 1);
 
-    } else if ((port = strtol(token, &junk, 10)), !*junk) {
-        /* port */
+    } else if (strtol(token, &junk, 10) && !*junk) {
+        port = xatos(token);
         debugs(3, 3, s->protocol << "_port: found Listen on Port: " << port);
     } else {
         debugs(3, DBG_CRITICAL, s->protocol << "_port: missing Port: " << token);
@@ -3639,16 +3678,16 @@ parse_port_option(AnyP::PortCfg * s, char *token)
     } else if (strncmp(token, "tcpkeepalive=", 13) == 0) {
         char *t = token + 13;
         s->tcp_keepalive.enabled = 1;
-        s->tcp_keepalive.idle = atoi(t);
+        s->tcp_keepalive.idle = xatoui(t);
         t = strchr(t, ',');
         if (t) {
             ++t;
-            s->tcp_keepalive.interval = atoi(t);
+            s->tcp_keepalive.interval = xatoui(t);
             t = strchr(t, ',');
         }
         if (t) {
             ++t;
-            s->tcp_keepalive.timeout = atoi(t);
+            s->tcp_keepalive.timeout = xatoui(t);
             // t = strchr(t, ','); // not really needed, left in as documentation
         }
 #if USE_SSL
@@ -3705,6 +3744,7 @@ parse_port_option(AnyP::PortCfg * s, char *token)
         parseBytesOptionValue(&s->dynamicCertMemCacheSize, B_BYTES_STR, token + 28);
 #endif
     } else {
+        debugs(3, DBG_CRITICAL, "FATAL: Unknown http(s)_port option '" << token << "'.");
         self_destruct();
     }
 }

diff --git a/src/wccp2.cc b/src/wccp2.cc
index 8861dd8c993c4225f2436d390407a8d7386a4fc5..cf03ef63868092e1b1c8fa1a4d2db309344d2f7f 100644 (file)
--- a/src/wccp2.cc
+++ b/src/wccp2.cc
@@ -2252,7 +2252,7 @@ parse_wccp2_service_ports(char *options, int portlist[])
 {
     int i = 0;
     int p;
-    char *tmp, *tmp2, *port, *end;
+    char *tmp, *tmp2, *port;
 
     if (!options) {
         return;
@@ -2264,7 +2264,7 @@ parse_wccp2_service_ports(char *options, int portlist[])
     port = strsep(&tmp2, ",");
 
     while (port && i < WCCP2_NUMPORTS) {
-        p = strtol(port, &end, 0);
+        p = xatoi(port);
 
         if (p < 1 || p > 65535) {
             fatalf("parse_wccp2_service_ports: port value '%s' isn't valid (1..65535)\n", port);