Sanitize command arguments. (#14064)

* Sanitize bash arguments.

Remove leading dashes and escape single quotes in command arguments.

* Quote expanded variable in test

daemon/main.c

@@ -1024,6 +1024,9 @@ int main(int argc, char **argv) {
                             fprintf(stderr, "\n\nALL TESTS PASSED\n\n");
                             return 0;
                         }
+                        else if(strcmp(optarg, "escapetest") == 0) {
+                            return command_argument_sanitization_tests();
+                        }
 #ifdef ENABLE_ML_TESTS
                         else if(strcmp(optarg, "mltest") == 0) {
                             return test_ml(argc, argv);

daemon/unit_test.c

@@ -2,6 +2,61 @@
 
 #include "common.h"
 
+static bool cmd_arg_sanitization_test(const char *expected, const char *src, char *dst, size_t dst_size) {
+    bool ok = sanitize_command_argument_string(dst, src, dst_size);
+
+    if (!expected)
+        return ok == false;
+
+    return strcmp(expected, dst) == 0;
+}
+
+bool command_argument_sanitization_tests() {
+    char dst[1024];
+
+    for (size_t i = 0; i != 5; i++)  {
+        const char *expected = i == 4 ? "'\\''" : NULL;
+        if (cmd_arg_sanitization_test(expected, "'", dst, i) == false) {
+            fprintf(stderr, "expected: >>>%s<<<, got: >>>%s<<<\n", expected, dst);
+            return 1;
+        }
+    }
+
+    for (size_t i = 0; i != 9; i++)  {
+        const char *expected = i == 8 ? "'\\'''\\''" : NULL;
+        if (cmd_arg_sanitization_test(expected, "''", dst, i) == false) {
+            fprintf(stderr, "expected: >>>%s<<<, got: >>>%s<<<\n", expected, dst);
+            return 1;
+        }
+    }
+
+    for (size_t i = 0; i != 7; i++)  {
+        const char *expected = i == 6 ? "'\\''a" : NULL;
+        if (cmd_arg_sanitization_test(expected, "'a", dst, i) == false) {
+            fprintf(stderr, "expected: >>>%s<<<, got: >>>%s<<<\n", expected, dst);
+            return 1;
+        }
+    }
+
+    for (size_t i = 0; i != 7; i++)  {
+        const char *expected = i == 6 ? "a'\\''" : NULL;
+        if (cmd_arg_sanitization_test(expected, "a'", dst, i) == false) {
+            fprintf(stderr, "expected: >>>%s<<<, got: >>>%s<<<\n", expected, dst);
+            return 1;
+        }
+    }
+
+    for (size_t i = 0; i != 22; i++)  {
+        const char *expected = i == 21 ? "foo'\\''a'\\'''\\'''\\''b" : NULL;
+        if (cmd_arg_sanitization_test(expected, "--foo'a'''b", dst, i) == false) {
+            fprintf(stderr, "expected: >>>%s<<<, got: >>>%s<<<\n length: %zu\n", expected, dst, strlen(dst));
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 static int check_number_printing(void) {
     struct {
         NETDATA_DOUBLE n;

daemon/unit_test.h

@@ -3,6 +3,8 @@
 #ifndef NETDATA_UNIT_TEST_H
 #define NETDATA_UNIT_TEST_H 1
 
+#include "stdbool.h"
+
 int unit_test_storage(void);
 int unit_test(long delay, long shift);
 int run_all_mockup_tests(void);
@@ -19,4 +21,6 @@ void dbengine_stress_test(unsigned TEST_DURATION_SEC, unsigned DSET_CHARTS, unsi
 
 #endif
 
+bool command_argument_sanitization_tests();
+
 #endif /* NETDATA_UNIT_TEST_H */

health/health.c

@@ -17,6 +17,145 @@
 #error WORKER_UTILIZATION_MAX_JOB_TYPES has to be at least 10
 #endif
 
+static bool prepare_command(BUFFER *wb,
+                            const char *exec,
+                            const char *recipient,
+                            const char *registry_hostname,
+                            uint32_t unique_id,
+                            uint32_t alarm_id,
+                            uint32_t alarm_event_id,
+                            uint32_t when,
+                            const char *alert_name,
+                            const char *alert_chart_name,
+                            const char *alert_family,
+                            const char *new_status,
+                            const char *old_status,
+                            NETDATA_DOUBLE new_value,
+                            NETDATA_DOUBLE old_value,
+                            const char *alert_source,
+                            uint32_t duration,
+                            uint32_t non_clear_duration,
+                            const char *alert_units,
+                            const char *alert_info,
+                            const char *new_value_string,
+                            const char *old_value_string,
+                            const char *source,
+                            const char *error_msg,
+                            int n_warn,
+                            int n_crit,
+                            const char *warn_alarms,
+                            const char *crit_alarms,
+                            const char *classification,
+                            const char *edit_command,
+                            const char *machine_guid)
+{
+    char buf[8192];
+    size_t n = 8192 - 1;
+
+    buffer_strcat(wb, "exec");
+
+    if (!sanitize_command_argument_string(buf, exec, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, recipient, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, registry_hostname, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    buffer_sprintf(wb, " '%u'", unique_id);
+
+    buffer_sprintf(wb, " '%u'", alarm_id);
+
+    buffer_sprintf(wb, " '%u'", alarm_event_id);
+
+    buffer_sprintf(wb, " '%u'", when);
+
+    if (!sanitize_command_argument_string(buf, alert_name, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, alert_chart_name, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, alert_family, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, new_status, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, old_status, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    buffer_sprintf(wb, " '" NETDATA_DOUBLE_FORMAT_ZERO "'", new_value);
+
+    buffer_sprintf(wb, " '" NETDATA_DOUBLE_FORMAT_ZERO "'", old_value);
+
+    if (!sanitize_command_argument_string(buf, alert_source, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    buffer_sprintf(wb, " '%u'", duration);
+
+    buffer_sprintf(wb, " '%u'", non_clear_duration);
+
+    if (!sanitize_command_argument_string(buf, alert_units, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, alert_info, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, new_value_string, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, old_value_string, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, source, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, error_msg, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    buffer_sprintf(wb, " '%d'", n_warn);
+
+    buffer_sprintf(wb, " '%d'", n_crit);
+
+    if (!sanitize_command_argument_string(buf, warn_alarms, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, crit_alarms, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, classification, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, edit_command, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    if (!sanitize_command_argument_string(buf, machine_guid, n))
+        return false;
+    buffer_sprintf(wb, " '%s'", buf);
+
+    return true;
+}
 
 unsigned int default_health_enabled = 1;
 char *silencers_filename;
@@ -235,7 +374,6 @@ static inline RRDCALC_STATUS rrdcalc_value2status(NETDATA_DOUBLE n) {
     return RRDCALC_STATUS_CLEAR;
 }
 
-#define ALARM_EXEC_COMMAND_LENGTH 8192
 #define ACTIVE_ALARMS_LIST_EXAMINE 500
 #define ACTIVE_ALARMS_LIST 15
 
@@ -306,8 +444,6 @@ static inline void health_alarm_execute(RRDHOST *host, ALARM_ENTRY *ae) {
 
     log_health("[%s]: Sending notification for alarm '%s.%s' status %s.", rrdhost_hostname(host), ae_chart_name(ae), ae_name(ae), rrdcalc_status2string(ae->new_status));
 
-    static char command_to_run[ALARM_EXEC_COMMAND_LENGTH + 1];
-
     const char *exec      = (ae->exec)      ? ae_exec(ae)      : string2str(host->health_default_exec);
     const char *recipient = (ae->recipient) ? ae_recipient(ae) : string2str(host->health_default_recipient);
 
@@ -375,49 +511,53 @@ static inline void health_alarm_execute(RRDHOST *host, ALARM_ENTRY *ae) {
 
     char *edit_command = ae->source ? health_edit_command_from_source(ae_source(ae)) : strdupz("UNKNOWN=0=UNKNOWN");
 
-    snprintfz(command_to_run, ALARM_EXEC_COMMAND_LENGTH, "exec %s '%s' '%s' '%u' '%u' '%u' '%lu' '%s' '%s' '%s' '%s' '%s' '" NETDATA_DOUBLE_FORMAT_ZERO
-        "' '" NETDATA_DOUBLE_FORMAT_ZERO
-        "' '%s' '%u' '%u' '%s' '%s' '%s' '%s' '%s' '%s' '%d' '%d' '%s' '%s' '%s' '%s' '%s'",
-              exec,
-              recipient,
-              rrdhost_registry_hostname(host),
-              ae->unique_id,
-              ae->alarm_id,
-              ae->alarm_event_id,
-              (unsigned long)ae->when,
-              ae_name(ae),
-              ae->chart?ae_chart_name(ae):"NOCHART",
-              ae->family?ae_family(ae):"NOFAMILY",
-              rrdcalc_status2string(ae->new_status),
-              rrdcalc_status2string(ae->old_status),
-              ae->new_value,
-              ae->old_value,
-              ae->source?ae_source(ae):"UNKNOWN",
-              (uint32_t)ae->duration,
-              (uint32_t)ae->non_clear_duration,
-              ae_units(ae),
-              ae_info(ae),
-              ae_new_value_string(ae),
-              ae_old_value_string(ae),
-              (expr && expr->source)?expr->source:"NOSOURCE",
-              (expr && expr->error_msg)?buffer_tostring(expr->error_msg):"NOERRMSG",
-              n_warn,
-              n_crit,
-              buffer_tostring(warn_alarms),
-              buffer_tostring(crit_alarms),
-              ae->classification?ae_classification(ae):"Unknown",
-              edit_command,
-              host != localhost ? host->machine_guid:""
-    );
-
-    ae->flags |= HEALTH_ENTRY_FLAG_EXEC_RUN;
-    ae->exec_run_timestamp = now_realtime_sec(); /* will be updated by real time after spawning */
-
-    debug(D_HEALTH, "executing command '%s'", command_to_run);
-    ae->flags |= HEALTH_ENTRY_FLAG_EXEC_IN_PROGRESS;
-    ae->exec_spawn_serial = spawn_enq_cmd(command_to_run);
-    enqueue_alarm_notify_in_progress(ae);
+    BUFFER *wb = buffer_create(8192);
+    bool ok = prepare_command(wb,
+                              exec,
+                              recipient,
+                              rrdhost_registry_hostname(host),
+                              ae->unique_id,
+                              ae->alarm_id,
+                              ae->alarm_event_id,
+                              (unsigned long)ae->when,
+                              ae_name(ae),
+                              ae->chart?ae_chart_name(ae):"NOCHART",
+                              ae->family?ae_family(ae):"NOFAMILY",
+                              rrdcalc_status2string(ae->new_status),
+                              rrdcalc_status2string(ae->old_status),
+                              ae->new_value,
+                              ae->old_value,
+                              ae->source?ae_source(ae):"UNKNOWN",
+                              (uint32_t)ae->duration,
+                              (uint32_t)ae->non_clear_duration,
+                              ae_units(ae),
+                              ae_info(ae),
+                              ae_new_value_string(ae),
+                              ae_old_value_string(ae),
+                              (expr && expr->source)?expr->source:"NOSOURCE",
+                              (expr && expr->error_msg)?buffer_tostring(expr->error_msg):"NOERRMSG",
+                              n_warn,
+                              n_crit,
+                              buffer_tostring(warn_alarms),
+                              buffer_tostring(crit_alarms),
+                              ae->classification?ae_classification(ae):"Unknown",
+                              edit_command,
+                              host != localhost ? host->machine_guid:"");
+
+    const char *command_to_run = buffer_tostring(wb);
+    if (ok) {
+        ae->flags |= HEALTH_ENTRY_FLAG_EXEC_RUN;
+        ae->exec_run_timestamp = now_realtime_sec(); /* will be updated by real time after spawning */
+
+        debug(D_HEALTH, "executing command '%s'", command_to_run);
+        ae->flags |= HEALTH_ENTRY_FLAG_EXEC_IN_PROGRESS;
+        ae->exec_spawn_serial = spawn_enq_cmd(command_to_run);
+        enqueue_alarm_notify_in_progress(ae);
+    } else {
+        error("Failed to format command arguments");
+    }
 
+    buffer_free(wb);
     freez(edit_command);
     buffer_free(warn_alarms);
     buffer_free(crit_alarms);

health/notifications/alarm-notify.sh.in

@@ -250,7 +250,7 @@ fi
 # -----------------------------------------------------------------------------
 # find a suitable hostname to use, if netdata did not supply a hostname
 
-if [ -z ${args_host} ]; then
+if [ -z "${args_host}" ]; then
   this_host=$(hostname -s 2>/dev/null)
   host="${this_host}"
   args_host="${this_host}"

libnetdata/inlined.h

@@ -181,6 +181,42 @@ static inline void sanitize_json_string(char *dst, const char *src, size_t dst_s
     *dst = '\0';
 }
 
+static inline bool sanitize_command_argument_string(char *dst, const char *src, size_t dst_size) {
+    // skip leading dashes
+    while (src[0] == '-')
+        src++;
+
+    // escape single quotes
+    while (src[0] != '\0') {
+        if (src[0] == '\'') {
+            if (dst_size < 4)
+                return false;
+
+            dst[0] = '\''; dst[1] = '\\'; dst[2] = '\''; dst[3] = '\'';
+
+            dst += 4;
+            dst_size -= 4;
+        } else {
+            if (dst_size < 1)
+                return false;
+
+            dst[0] = src[0];
+
+            dst += 1;
+            dst_size -= 1;
+        }
+
+        src++;
+    }
+
+    // make sure we have space to terminate the string
+    if (dst_size == 0)
+        return false;
+    *dst = '\0';
+
+    return true;
+}
+
 static inline int read_file(const char *filename, char *buffer, size_t size) {
     if(unlikely(!size)) return 3;
 

spawn/spawn.c

@@ -8,7 +8,7 @@ int spawn_thread_shutdown;
 
 struct spawn_queue spawn_cmd_queue;
 
-static struct spawn_cmd_info *create_spawn_cmd(char *command_to_run)
+static struct spawn_cmd_info *create_spawn_cmd(const char *command_to_run)
 {
     struct spawn_cmd_info *cmdinfo;
 
@@ -57,7 +57,7 @@ static void init_spawn_cmd_queue(void)
 /*
  * Returns serial number of the enqueued command
  */
-uint64_t spawn_enq_cmd(char *command_to_run)
+uint64_t spawn_enq_cmd(const char *command_to_run)
 {
     unsigned queue_size;
     uint64_t serial;

spawn/spawn.h

@@ -84,7 +84,7 @@ void spawn_init(void);
 void spawn_server(void);
 void spawn_client(void *arg);
 void destroy_spawn_cmd(struct spawn_cmd_info *cmdinfo);
-uint64_t spawn_enq_cmd(char *command_to_run);
+uint64_t spawn_enq_cmd(const char *command_to_run);
 void spawn_wait_cmd(uint64_t serial, int *exit_status, time_t *exec_run_timestamp);
 void spawn_deq_cmd(struct spawn_cmd_info *cmdinfo);
 struct spawn_cmd_info *spawn_get_unprocessed_cmd(void);