GUACAMOLE-2185: Port numbers should be validated by guacd.

Author: bbennett-ks

src/libguac/guacamole/string.h

@@ -26,9 +26,39 @@
  * @file string.h
  */
 
+#include <limits.h>
 #include <stddef.h>
 #include <string.h>
 
+/**
+ *  String buffer size needed to represent 'int', and 'unsigned short'in decimal
+ *  format. This includes the NULL terminator.
+ * 
+ *  Integer size can vary by platform, but short size is consistent across
+ *  all modern platforms.
+ */
+#if   (__SIZEOF_INT__ == 2)     /* INT_MIN: '-32768' */
+    #define GUAC_INT_STRING_BUFSIZE 7
+#elif (__SIZEOF_INT__ == 4)     /* INT_MIN: '-2147483648' */
+    #define GUAC_INT_STRING_BUFSIZE 12
+#elif (__SIZEOF_INT__ == 8)     /* INT_MIN: '-9223372036854775808' */
+    #define GUAC_INT_STRING_BUFSIZE 21
+#else
+    #error Unsupported unsigned int size
+#endif
+
+#define GUAC_USHORT_STRING_BUFSIZE 6   /* '65535' */
+
+/**
+ * Convenience macros for the minimum and maximum values for int and and
+ * unsigned short data types.
+ */
+#define GUAC_ITOA_INT_MIN     INT_MIN
+#define GUAC_ITOA_INT_MAX     INT_MAX
+
+#define GUAC_ITOA_USHORT_MAX  USHRT_MAX
+#define GUAC_ITOA_USHORT_MIN  0
+
 /**
  * Convert the provided unsigned integer into a string, returning the number of
  * characters written into the destination string, or a negative value if an
@@ -37,15 +67,35 @@
  * @param dest
  *     The destination string to copy the data into, which should already be
  *     allocated and at a size that can handle the string representation of the
- *     inteer.
+ *     integer (at least GUAC_INT_STRING_BUFSIZE bytes).
  *
  * @param integer
- *     The unsigned integer to convert to a string.
+ *     The integer to convert to a string.
  * 
  * @return
  *     The number of characters written into the dest string.
  */
-int guac_itoa(char* restrict dest, unsigned int integer);
+int guac_itoa(char* restrict dest, int integer);
+
+/**
+ * Converts the given integer to a string safely. The resulting string will be
+ * written into the provided buffer, ensuring that the buffer is not exceeded.
+ * The conversion will fail if the buffer is too small to hold the result.
+ *
+ * @param dest
+ *     The buffer to write the converted string into.
+ *
+ * @param dest_size
+ *     The size of the provided buffer, in bytes.
+ *
+ * @param integer
+ *     The integer to convert to a string.
+ *
+ * @return
+ *     The number of characters written (excluding the null terminator), or
+ *     -1 if the string was truncated, or a negative value if an error occurs.
+ */
+int guac_itoa_safe(char* restrict dest, size_t dest_size, int integer);
 
 /**
  * Copies a limited number of bytes from the given source string to the given

src/libguac/guacamole/user.h

@@ -932,6 +932,98 @@ int guac_user_supports_webp(guac_user* user);
 char* guac_user_parse_args_string(guac_user* user, const char** arg_names,
         const char** argv, int index, const char* default_value);
 
+/**
+ * Automatically handles a single argument received from a joining user,
+ * returning a newly-allocated string containing that value. The argument
+ * is parsed as an integer and checked to ensure it is within the specified
+ * bounds. If the argument provided by the user is blank, a newly-allocated
+ * string containing the default value is returned. If the integer is out of
+ * bounds, the default value is returned.
+ *
+ * @param user
+ *     The user joining the connection and providing the given arguments.
+ *
+ * @param arg_names
+ *     A NULL-terminated array of argument names, corresponding to the provided
+ *     array of argument values. This array must be exactly the same size as
+ *     the argument value array, with one additional entry for the NULL
+ *     terminator.
+ *
+ * @param argv
+ *     An array of all argument values, corresponding to the provided array of
+ *     argument names. This array must be exactly the same size as the argument
+ *     name array, with the exception of the NULL terminator.
+ *
+ * @param index
+ *     The index of the entry in both the arg_names and argv arrays which
+ *     corresponds to the argument being parsed.
+ *
+ * @param default_value
+ *     The value to return if the provided argument is blank or invalid. If this
+ *     value is not NULL, the returned value will be a newly-allocated string
+ *     containing this value.
+ *
+ * @param min
+ *     The minimum allowed value for the parsed integer. If the parsed integer is
+ *     less than this value, the default value will be returned.
+ *
+ * @param max
+ *     The maximum allowed value for the parsed integer. If the parsed integer is
+ *     greater than this value, the default value will be returned.
+ *
+ * @return
+ *     A newly-allocated string containing the provided argument value, parsed as
+ *     an integer and checked against the specified bounds. If the argument is blank,
+ *     or if the integer is out of bounds, the function returns a newly-allocated string
+ *     containing the default value. If the default value is NULL and the provided
+ *     argument is blank or invalid, NULL is returned.
+ */
+char* guac_user_parse_args_int_string_bounded(guac_user* user, const char** arg_names,
+        const char** argv, int index, const char* default_value, int min, int max);
+
+/**
+ * Automatically handles a single integer argument received from a joining
+ * user, returning the integer value of that argument. The argument is checked
+ * to ensure it is within the specified bounds. If the argument is blank,
+ * invalid, or out of bounds, the default value is returned.
+ *
+ * @param user
+ *     The user joining the connection and providing the given arguments.
+ *
+ * @param arg_names
+ *     A NULL-terminated array of argument names, corresponding to the provided
+ *     array of argument values. This array must be exactly the same size as
+ *     the argument value array, with one additional entry for the NULL
+ *     terminator.
+ *
+ * @param argv
+ *     An array of all argument values, corresponding to the provided array of
+ *     argument names. This array must be exactly the same size as the argument
+ *     name array, with the exception of the NULL terminator.
+ *
+ * @param index
+ *     The index of the entry in both the arg_names and argv arrays which
+ *     corresponds to the argument being parsed.
+ *
+ * @param default_value
+ *     The value to return if the provided argument is blank, invalid, or out of bounds.
+ *
+ * @param min
+ *     The minimum allowed value for the argument. If the parsed value is less
+ *     than this, the default value will be returned.
+ *
+ * @param max
+ *     The maximum allowed value for the argument. If the parsed value is greater
+ *     than this, the default value will be returned.
+ *
+ * @return
+ *     The integer value parsed from the provided argument value, if it is valid
+ *     and within the specified bounds. If the argument is blank, invalid, or out of bounds,
+ *     the default value is returned.
+ */
+int guac_user_parse_args_int_bounded(guac_user* user, const char** arg_names,
+        const char** argv, int index, int default_value, int min, int max);
+
 /**
  * Automatically handles a single integer argument received from a joining
  * user, returning the integer value of that argument. If the argument provided

src/libguac/guacamole/wol-constants.h

@@ -20,6 +20,8 @@
 #ifndef GUAC_WOL_CONSTANTS_H
 #define GUAC_WOL_CONSTANTS_H
 
+#include "guacamole/string.h"
+
 /**
  * Header file that provides constants and defaults related to libguac
  * Wake-on-LAN support.

src/libguac/string.c

@@ -59,6 +59,21 @@ int guac_itoa(char* restrict dest, unsigned int integer) {
 
 }
 
+int guac_itoa_safe(char* restrict dest, size_t dest_size, int integer)
+{
+    int str_size = snprintf(dest, dest_size, "%d", integer);
+
+    if (str_size < 0)
+        return str_size;
+
+    /* Return an error if the string was truncated. */
+    if ((size_t)str_size >= dest_size)
+        return -1;
+
+    /* Return the number of characters written (excluding the terminator). */
+    return str_size;
+}
+
 size_t guac_strlcpy(char* restrict dest, const char* restrict src, size_t n) {
 
 #ifdef HAVE_STRLCPY

src/libguac/user.c

@@ -366,12 +366,15 @@ char* guac_user_parse_args_string(guac_user* user, const char** arg_names,
     /* Pull parameter value from argv */
     const char* value = argv[index];
 
-    /* Use default value if blank */
-    if (value[0] == 0) {
+    /* Use default_value if value is NULL, or an empty string */
+    if (value == NULL || value[0] == 0) {
 
         /* NULL is a completely legal default value */
-        if (default_value == NULL)
+        if (default_value == NULL) {
+            guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. No default value provided.",
+                    arg_names[index]);
             return NULL;
+        }
 
         /* Log use of default */
         guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. Using "
@@ -386,17 +389,74 @@ char* guac_user_parse_args_string(guac_user* user, const char** arg_names,
 
 }
 
-int guac_user_parse_args_int(guac_user* user, const char** arg_names,
-        const char** argv, int index, int default_value) {
+char* guac_user_parse_args_int_string_bounded(guac_user* user, const char** arg_names,
+        const char** argv, int index, const char* default_value, int min, int max) {
+
+    char* parse_end;
+    long parsed_value;
+
+    /* Pull parameter value from argv */
+    const char* value = argv[index];
+
+    /* Use default_value if value is NULL, or an empty string */
+    if (value == NULL || value[0] == 0) {
+
+        /* NULL is a completely legal default value */
+        if (default_value == NULL) {
+            guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. No default value provided.",
+                    arg_names[index]);
+
+            return NULL;
+        }
+
+        /* Log use of default */
+        guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. Using "
+                "default value of \"%s\".", arg_names[index], default_value);
+
+        return guac_strdup(default_value);
+
+    }
+
+    /* Parse value using strtol, checking for errors */
+    errno = 0;
+    parsed_value = strtol(value, &parse_end, 10);
+
+    /* Check for parsing errors or invalid range */
+    if (errno != 0 || *parse_end != '\0' || parsed_value < min || parsed_value > max) {
+
+        /* NULL is a completely legal default value */
+        if (default_value == NULL) {
+            guac_user_log(user, GUAC_LOG_WARNING, "Specified value \"%s\" for "
+                    "parameter \"%s\" is not valid. No default value provided",
+                    value, arg_names[index]);
+
+            return NULL;
+        }
+
+        /* Log use of default */
+        guac_user_log(user, GUAC_LOG_WARNING, "Specified value \"%s\" for "
+                "parameter \"%s\" is not valid. Using default value "
+                "of \"%s\".", value, arg_names[index], default_value);
+
+        return guac_strdup(default_value);
+
+    }
+
+    /* Otherwise use provided value */
+    return guac_strdup(value);
+}
+
+int guac_user_parse_args_int_bounded(guac_user* user, const char** arg_names,
+        const char** argv, int index, int default_value, int min, int max) {
 
     char* parse_end;
     long parsed_value;
 
     /* Pull parameter value from argv */
     const char* value = argv[index];
 
-    /* Use default value if blank */
-    if (value[0] == 0) {
+    /* Use default_value if value is NULL, or an empty string */
+    if (value == NULL || value[0] == 0) {
 
         /* Log use of default */
         guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. Using "
@@ -410,24 +470,28 @@ int guac_user_parse_args_int(guac_user* user, const char** arg_names,
     errno = 0;
     parsed_value = strtol(value, &parse_end, 10);
 
-    /* Ensure parsed value is within the legal range of an int */
-    if (parsed_value < INT_MIN || parsed_value > INT_MAX)
-        errno = ERANGE;
-
-    /* Resort to default if input is invalid */
-    if (errno != 0 || *parse_end != '\0') {
+    /* Check for parsing errors or invalid range */
+    if (errno != 0 || *parse_end != '\0' || parsed_value < min || parsed_value > max) {
 
         /* Log use of default */
         guac_user_log(user, GUAC_LOG_WARNING, "Specified value \"%s\" for "
-                "parameter \"%s\" is not a valid integer. Using default value "
+                "parameter \"%s\" is not valid. Using default value "
                 "of %i.", value, arg_names[index], default_value);
 
         return default_value;
 
     }
 
     /* Parsed successfully */
-    return parsed_value;
+    return (int)parsed_value;
+
+}
+
+int guac_user_parse_args_int(guac_user* user, const char** arg_names,
+        const char** argv, int index, int default_value) {
+
+    return guac_user_parse_args_int_bounded(user, arg_names, argv, index,
+                   default_value, GUAC_ITOA_INT_MIN, GUAC_ITOA_INT_MAX);
 
 }
 
@@ -437,8 +501,8 @@ int guac_user_parse_args_boolean(guac_user* user, const char** arg_names,
     /* Pull parameter value from argv */
     const char* value = argv[index];
 
-    /* Use default value if blank */
-    if (value[0] == 0) {
+    /* Use default_value if value is NULL, or an empty string */
+    if (value == NULL || value[0] == 0) {
 
         /* Log use of default */
         guac_user_log(user, GUAC_LOG_DEBUG, "Parameter \"%s\" omitted. Using "

src/protocols/kubernetes/settings.c

@@ -278,9 +278,9 @@ guac_kubernetes_settings* guac_kubernetes_parse_args(guac_user* user,
                 IDX_HOSTNAME, "");
 
     /* Read port */
-    settings->port =
-        guac_user_parse_args_int(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
-                IDX_PORT, GUAC_KUBERNETES_DEFAULT_PORT);
+    settings->port = (unsigned short)
+        guac_user_parse_args_int_bounded(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
+                IDX_PORT, GUAC_KUBERNETES_DEFAULT_PORT, GUAC_ITOA_USHORT_MIN, GUAC_ITOA_USHORT_MAX);
 
     /* Read Kubernetes namespace */
     settings->kubernetes_namespace =

src/protocols/kubernetes/settings.h

@@ -61,7 +61,7 @@ typedef struct guac_kubernetes_settings {
     /**
      * The port of the Kubernetes server to connect to.
      */
-    int port;
+    unsigned short port;
 
     /**
      * The name of the Kubernetes namespace of the pod containing the container