[checkmk-commits] fixed a couple of potential crash sources and theoretical security issues in the windows agent

Module: check_mk Branch: master Commit: 296aff3bbbd768be35791f1ef2bf4314e0ab6571 URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=296aff3bbbd768… Author: Sebastian Herbord &lt;sh(a)mathias-kettner.de&gt; Date: Mon Oct 26 16:48:09 2015 +0100 fixed a couple of potential crash sources and theoretical security issues in the windows agent the problems were all caused by a misuse of snprintf. --- agents/windows/Configuration.cc | 4 ++-- agents/windows/check_mk_agent.cc | 29 +++++++++++++++-------------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/agents/windows/Configuration.cc b/agents/windows/Configuration.cc index c083d40..818dea8 100644 --- a/agents/windows/Configuration.cc +++ b/agents/windows/Configuration.cc @@ -822,8 +822,8 @@ bool Configuration::handleMrpeConfigVariable(char *var, char *value) memset(tmp, 0, sizeof(*tmp)); if (user) - snprintf(tmp->user, sizeof(tmp->user), user); - snprintf(tmp->path, sizeof(tmp->path), value); + snprintf(tmp->user, sizeof(tmp->user), "%s", user); + snprintf(tmp->path, sizeof(tmp->path), "%s", value); _mrpe_includes.add(tmp); return true; } diff --git a/agents/windows/check_mk_agent.cc b/agents/windows/check_mk_agent.cc index e6f920e..89ebfe6 100644 --- a/agents/windows/check_mk_agent.cc +++ b/agents/windows/check_mk_agent.cc @@ -67,6 +67,7 @@ #include <map> #include <vector> #include <string> +#include <algorithm> #include "stringutil.h" #include "Environment.h" #include "Configuration.h" @@ -132,18 +133,20 @@ typedef map<unsigned long long, process_entry> process_entry_t; // Forward declarations of functions void listen_tcp_loop(const Environment &env); -void output(SOCKET &out, const char *format, ...); char *ipv4_to_text(uint32_t ip); void output_data(SOCKET &out, const Environment &env); double file_time(const FILETIME *filetime); void open_crash_log(const std::string &log_directory); void close_crash_log(); -void crash_log(const char *format, ...); void lowercase(char* value); void collect_script_data(script_execution_mode mode); void find_scripts(const Environment &env); void RunImmediate(const char *mode, int argc, char **argv); +void output(SOCKET &out, const char *format, ...) __attribute__ ((format (printf, 2, 3))); +void crash_log(const char *format, ...) __attribute__ ((format (printf, 1, 2))); +void verbose(const char *format, ...) __attribute__ ((format (printf, 1, 2))); + // .----------------------------------------------------------------------. // | ____ _ _ _ | // | / ___| | ___ | |__ __ _| |___ | @@ -2294,7 +2297,6 @@ int launch_program(script_container* cont) { int exit_code = 0; int out_offset = 0; - char buf[16635]; // i/o buffer STARTUPINFO si; SECURITY_ATTRIBUTES sa; @@ -2356,7 +2358,9 @@ int launch_program(script_container* cont) unsigned long bread; // bytes read unsigned long avail; // bytes available - memset(buf, 0, sizeof(buf)); + static const size_t BUFFER_SIZE = 16635; + char buf[BUFFER_SIZE]; // i/o buffer + memset(buf, 0, BUFFER_SIZE); time_t process_start = time(0); bool buffer_full = false; @@ -2400,9 +2404,9 @@ int launch_program(script_container* cont) break; if (bread > 0) { - memset(buf, 0, sizeof(buf)); - ReadFile(read_stdout, buf, sizeof(buf) - 1, &bread, NULL); - out_offset += snprintf(cont->buffer_work + out_offset, current_heap_size - out_offset, buf); + ReadFile(read_stdout, cont->buffer_work + out_offset, + std::min<size_t>(BUFFER_SIZE - 1, current_heap_size - out_offset), &bread, NULL); + out_offset += bread; } } if (buffer_full) { @@ -2416,7 +2420,6 @@ int launch_program(script_container* cont) Sleep(10); } - TerminateJobObject(cont->job_object, exit_code); // cleanup the mess @@ -2585,7 +2588,7 @@ void output_external_programs(SOCKET &out, script_type type) snprintf(cache_buffer + cache_buffer_offset, write_bytes, "%s", line); cache_buffer_offset += write_bytes - 1; - snprintf(cache_buffer + cache_buffer_offset, cache_len, cache_info); + snprintf(cache_buffer + cache_buffer_offset, cache_len, "%s", cache_info); cache_buffer_offset += cache_len - 1; write_bytes = 3 + cr_offset + 1 + 1; // >>> + \r + \n + \0 @@ -2614,7 +2617,7 @@ void output_external_programs(SOCKET &out, script_type type) cont->buffer = NULL; } if (cont->buffer) - output(out, cont->buffer); + output(out, "%s", cont->buffer); } it_cont++; } @@ -2701,10 +2704,8 @@ void update_mrpe_includes() p = strrchr(plugin_name, '\\'); if (p) plugin_name = p + 1; - strncpy(tmp_entry->plugin_name, plugin_name, - sizeof(tmp_entry->plugin_name)); - - snprintf(tmp_entry->run_as_user, sizeof(tmp_entry->run_as_user), (*it_include)->user); + strncpy(tmp_entry->plugin_name, plugin_name, sizeof(tmp_entry->plugin_name)); + strncpy(tmp_entry->run_as_user, (*it_include)->user, sizeof(tmp_entry->run_as_user)); g_included_mrpe_entries.push_back(tmp_entry); } }