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

Author: bbennett-ks

src/libguac/guacamole/string.h

@@ -30,22 +30,24 @@
 #include <string.h>
 
 /**
- * Convert the provided unsigned integer into a string, returning the number of
- * characters written into the destination string, or a negative value if an
- * error occurs.
+ * 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 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.
+ *     The buffer to write the converted string into.
+ *
+ * @param dest_size
+ *     The size of the provided buffer, in 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.
+ *     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(char* restrict dest, unsigned int integer);
+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_bound(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_bound(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

@@ -59,5 +59,12 @@
  */
 #define GUAC_WOL_PORT 9
 
+/**
+ * The maximum length (in characters) of the decimal string form of a TCP/UDP
+ * port number, including the null terminator. The largest valid port number is
+ * 65535 (5 digits), so the buffer must be at least 6 bytes long.
+ */
+#define GUAC_WOL_PORT_STRLEN 6
+
 #endif /* GUAC_WOL_CONSTANTS_H */
 

src/libguac/string.c

@@ -45,18 +45,19 @@
  */
 #define REMAINING(n, length) (((n) < (length)) ? 0 : ((n) - (length)))
 
-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);
 
-    /* Determine size of string. */
-    int str_size = snprintf(dest, 0, "%i", integer);
-
-    /* If an error occurs, just return that and skip the conversion. */
     if (str_size < 0)
         return str_size;
 
-    /* Do the conversion and return. */
-    return snprintf(dest, (str_size + 1), "%i", integer);
+    /* 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) {

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_bound(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_bound(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_bound(user, arg_names, argv, index,
+                   default_value, -INT_MAX, 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_bound(user, GUAC_KUBERNETES_CLIENT_ARGS, argv,
+                IDX_PORT, GUAC_KUBERNETES_DEFAULT_PORT, 0, USHRT_MAX);
 
     /* Read Kubernetes namespace */
     settings->kubernetes_namespace =

src/protocols/rdp/rdp.c

@@ -737,13 +737,12 @@ void* guac_rdp_client_thread(void* data) {
          */
         if (settings->wol_wait_time > 0) {
             guac_client_log(client, GUAC_LOG_DEBUG, "Sending Wake-on-LAN packet, "
-                    "and pausing for %d seconds.", settings->wol_wait_time);
+                    "and retrying connection check %d times every %d seconds.",
+                    GUAC_WOL_DEFAULT_CONNECT_RETRIES, settings->wol_wait_time);
 
-            /* char representation of a port should be, at most, 5 digits plus terminator. */
-            char* str_port = guac_mem_alloc(6);
-            if (guac_itoa(str_port, settings->port) < 1) {
-                guac_client_log(client, GUAC_LOG_ERROR, "Failed to convert port to integer for WOL function.");
-                guac_mem_free(str_port);
+            char str_port[GUAC_WOL_PORT_STRLEN];
+            if (guac_itoa_safe(str_port, sizeof(str_port), settings->port) < 1) {
+                guac_client_log(client, GUAC_LOG_ERROR, "Failed to convert port to string for WOL function.");
                 return NULL;
             }
 
@@ -754,26 +753,27 @@ void* guac_rdp_client_thread(void* data) {
                     settings->wol_wait_time,
                     GUAC_WOL_DEFAULT_CONNECT_RETRIES,
                     settings->hostname,
-                    (const char *) str_port,
-                    GUAC_WOL_DEFAULT_CONNECTION_TIMEOUT)) {
-                guac_client_log(client, GUAC_LOG_ERROR, "Failed to send WOL packet, or server failed to wake up.");
-                guac_mem_free(str_port);
+                    str_port,
+                    settings->timeout)) {
+                guac_client_log(client, GUAC_LOG_ERROR, "Failed to send WOL packet, or connect to remote server.");
                 return NULL;
             }
-
-            guac_mem_free(str_port);
-
         }
 
-        /* Just send the packet and continue the connection, or return if failed. */
-        else if(guac_wol_wake(settings->wol_mac_addr,
+        /* Just send the packet, or return NULL if failed. */
+        else {
+            guac_client_log(client, GUAC_LOG_DEBUG, "Sending RDP Wake-on-LAN packet.");
+
+            if(guac_wol_wake(settings->wol_mac_addr,
                     settings->wol_broadcast_addr,
                     settings->wol_udp_port)) {
-            guac_client_log(client, GUAC_LOG_ERROR, "Failed to send WOL packet.");
-            return NULL;
+                guac_client_log(client, GUAC_LOG_ERROR, "Failed to send WOL packet.");
+                return NULL;
+            }
         }
+
     }
-    
+
     /* If audio enabled, choose an encoder */
     if (settings->audio_enabled) {