Hi LTS list, I spent the last six hours backporting the CVE-2016-7067 patch[1] to monit 5.4 from Debian Wheezy. A lot of manual backporting work was needed.
I already tested the resulting package on a productive Wheezy system running monit and verified that it *) installs and upgrades cleanly *) indeed fixes the CSRF vulnerability: - tested with POST request lacking the CSRF protection token - tested with triggering status change over a GET request *) adds a "secure" flag if request comes over HTTPS *) doesn't introduce regressions to the basic functionality of monit Still, as the patch is rather intrusive and only tested by me so far, I'm asking for help: both testing the packages and reviewing the patch would be much appreciated. Wrt reviewing, the patch includes detailed documentation about what I did in order to backport the CSRF protection. The debdiff of monit 5.4-2+deb7u1 is attached to this mail. Source packages and binary packages for amd64 can be found here: https://people.debian.org/~mejo/wheezy-lts/ Cheers, jonas
diff -Nru monit-5.4/debian/changelog monit-5.4/debian/changelog --- monit-5.4/debian/changelog 2012-06-01 12:50:49.000000000 +0200 +++ monit-5.4/debian/changelog 2016-11-30 20:00:25.000000000 +0100 @@ -1,3 +1,12 @@ +monit (1:5.4-2+deb7u1) wheezy-security; urgency=high + + * Non-maintainer upload by the LTS Team. + * Add 09_CVE-2016-7067.patch, fixing CVE-2016-7067: monit actions are + vulnerable to cross-site request forgery (CSRF), e.g. allowing an attacker + to enable/disable monitoring of hosts and services. + + -- Jonas Meurer <m...@debian.org> Wed, 30 Nov 2016 20:00:25 +0100 + monit (1:5.4-2) unstable; urgency=low * Chande dh compat to 9, enable hardening support diff -Nru monit-5.4/debian/patches/09_CVE-2016-7067.patch monit-5.4/debian/patches/09_CVE-2016-7067.patch --- monit-5.4/debian/patches/09_CVE-2016-7067.patch 1970-01-01 01:00:00.000000000 +0100 +++ monit-5.4/debian/patches/09_CVE-2016-7067.patch 2016-11-30 19:59:22.000000000 +0100 @@ -0,0 +1,560 @@ +From: Jonas Meurer <jo...@freesources.org> +Date: Wed, 30 Nov 2016 15:45:34 +0100 +Subject: Fix CSRF vulnerability (CVE-2016-7067). + +* Backport upstream commit c6ec3820e627f85417053e6336de2987f2d863e3 to + monit 5.4 codebase. It implements the double-sumit cookie pattern to + protect all POST requests against Cross-Site Request Forgery (CSRF). + - Disabling the following http services for GET requests, requiring CSRF + protected POST: _doaction, _viewlog (_summary and _report are not + implemented in monit 5.4 yet). + - The service for "_runtime" and "/<servicename>" is still available via + GET, but only the non-state changing part (read-only) - the actions for + "_runtime" (force validate, stop monit http) and "/<servicename>" require + CSRF protected POST too. +* The original patch doesn't apply against monit 5.4 as the codebase + changed a lot since then. Additional manual backporting was needed: + - src/http/cervlet.c doPost(): enable actions _status and _status2 for + POST requests. + - src/http/processor.h: send_error() in monit 5.4 takes three arguments, + change __atribute__ at declaration of send_error() to "(printf, 3, 4)". + - src/http/processor.c: + - send_error(): add optional additional arguments ("..."). + - do_service(): use "Run.httpdssl" instead of "Run.http.flags && + Httpd_Ssl" as condition in invokation of set_header(). + - is_authenticated(): replace invokation of Socket_getRemoteHost() by + old name socket_get_remote_host(); "return FALSE" + instead of "return false". + - src/http/processor.c, src/util.c: remove superfluous first argument "req" + from invokation of send_error(). + - src/util.c: include system/Time.h from libmonit, as Util_getToken() uses + Time_now() from there. + - libmonit/src/util/Str.[ch]: backport Str_compareConstantTime() as it is + used in patched is_authenticated() from src/http/processor.c. + - src/control.c control_service_daemon(), src/collector.c data_send(): + the patch against _send() at src/http/client.c needs to go into both + functions. + +Origin: upstream, https://bitbucket.org/tildeslash/monit/commits/c6ec382 +Origin: upstream, https://bitbucket.org/tildeslash/monit/commits/8144093 + +--- a/libmonit/src/util/Str.c ++++ b/libmonit/src/util/Str.c +@@ -430,3 +430,16 @@ int Str_cmp(const void *x, const void *y + return strcmp((const char *)x, (const char *)y); + } + ++ ++int Str_compareConstantTime(const void *x, const void *y) { ++ // Copy input to zero initialized buffers of fixed size, to prevent string length timing attack (handle NULL input as well). If some string exceeds hardcoded buffer size, error is returned. ++ char _x[MAX_CONSTANT_TIME_STRING_LENGTH + 1] = {}; ++ char _y[MAX_CONSTANT_TIME_STRING_LENGTH + 1] = {}; ++ if (snprintf(_x, sizeof(_x), "%s", x ? (const char *)x : "") > MAX_CONSTANT_TIME_STRING_LENGTH || snprintf(_y, sizeof(_y), "%s", y ? (const char *)y : "") > MAX_CONSTANT_TIME_STRING_LENGTH) ++ return 1; ++ int rv = 0; ++ for (size_t i = 0; i < sizeof(_x); i++) ++ rv |= _x[i] ^ _y[i]; ++ return rv; ++} ++ +--- a/libmonit/src/util/Str.h ++++ b/libmonit/src/util/Str.h +@@ -37,6 +37,13 @@ + + + /** ++ * Maximum length of input for Str_compareConstantTime() method. We support ++ * currently up to 64 characters, which is enough for SHA256 digests. ++ */ ++#define MAX_CONSTANT_TIME_STRING_LENGTH 64 ++ ++ ++/** + * Test if the given string is defined. That is; not NULL nor the + * empty ("") string + * @param s The string to test +@@ -418,4 +425,14 @@ unsigned int Str_hash(const void *x); + int Str_cmp(const void *x, const void *y); + + ++/** ++ * Compare case sensitive two strings in constant time. This function ++ * can be used for timing-attack resistent comparison of credentials. ++ * @param x A String ++ * @param y A String ++ * @return 0 if x and y are equal otherwise a non-zero integer ++ */ ++int Str_compareConstantTime(const void *x, const void *y); ++ ++ + #endif +--- a/src/collector.c ++++ b/src/collector.c +@@ -64,10 +64,13 @@ + */ + static int data_send(Socket_T socket, Mmonit_T C, const char *D) { + char *auth = Util_getBasicAuthHeader(C->url->user, C->url->password); ++ MD_T token; ++ Util_getToken(token); + int rv = socket_print(socket, + "POST %s HTTP/1.1\r\n" + "Host: %s:%d\r\n" + "Content-Type: text/xml\r\n" ++ "Cookie: securitytoken=%s\r\n" + "Content-Length: %d\r\n" + "Pragma: no-cache\r\n" + "Accept: */*\r\n" +@@ -79,6 +82,7 @@ static int data_send(Socket_T socket, Mm + C->url->path, + C->url->hostname, C->url->port, + strlen(D), ++ token, + prog, VERSION, + auth?auth:"", + D); +--- a/src/control.c ++++ b/src/control.c +@@ -116,14 +116,19 @@ int control_service_daemon(const char *S + + /* Send request */ + auth = Util_getBasicAuthHeaderMonit(); ++ MD_T token; ++ Util_getToken(token); + if (socket_print(socket, + "POST /%s HTTP/1.0\r\n" + "Content-Type: application/x-www-form-urlencoded\r\n" ++ "Cookie: securitytoken=%s\r\n" + "Content-Length: %d\r\n" + "%s" + "\r\n" + "action=%s", ++ "securitytoken=%s&action=%s", + S, ++ token, + strlen("action=") + strlen(action), + auth ? auth : "", + action) < 0) +--- a/src/http/cervlet.c ++++ b/src/http/cervlet.c +@@ -87,7 +87,7 @@ + #define GETID "/_getid" + #define STATUS "/_status" + #define STATUS2 "/_status2" +-#define RUN "/_runtime" ++#define RUNTIME "/_runtime" + #define VIEWLOG "/_viewlog" + #define DOACTION "/_doaction" + +@@ -109,9 +109,11 @@ static void do_ping(HttpRequest, HttpRes + static void do_getid(HttpRequest, HttpResponse); + static void do_runtime(HttpRequest, HttpResponse); + static void do_viewlog(HttpRequest, HttpResponse); +-static void handle_action(HttpRequest, HttpResponse); +-static void handle_do_action(HttpRequest, HttpResponse); +-static void handle_run(HttpRequest, HttpResponse); ++static void handle_service(HttpRequest, HttpResponse); ++static void handle_service_action(HttpRequest, HttpResponse); ++static void handle_doaction(HttpRequest, HttpResponse); ++static void handle_runtime(HttpRequest, HttpResponse); ++static void handle_runtime_action(HttpRequest, HttpResponse); + static void is_monit_running(HttpRequest, HttpResponse); + static void do_service(HttpRequest, HttpResponse, Service_T); + static void print_alerts(HttpResponse, Mail_T); +@@ -185,12 +187,18 @@ static void doPost(HttpRequest req, Http + + set_content_type(res, "text/html"); + +- if(ACTION(RUN)) { +- handle_run(req, res); ++ if(ACTION(RUNTIME)) { ++ handle_runtime_action(req, res); ++ } else if(ACTION(VIEWLOG)) { ++ do_viewlog(req, res); ++ } else if(ACTION(STATUS)) { ++ print_status(req, res, 1); ++ } else if(ACTION(STATUS2)) { ++ print_status(req, res, 2); + } else if(ACTION(DOACTION)) { +- handle_do_action(req, res); ++ handle_doaction(req, res); + } else { +- handle_action(req, res); ++ handle_service_action(req, res); + } + + } +@@ -208,12 +216,10 @@ static void doGet(HttpRequest req, HttpR + LOCK(Run.mutex) + do_home(req, res); + END_LOCK; +- } else if(ACTION(RUN)) { +- handle_run(req, res); ++ } else if(ACTION(RUNTIME)) { ++ handle_runtime(req, res); + } else if(ACTION(TEST)) { + is_monit_running(req, res); +- } else if(ACTION(VIEWLOG)) { +- do_viewlog(req, res); + } else if(ACTION(ABOUT)) { + do_about(req, res); + } else if(ACTION(PING)) { +@@ -224,10 +230,8 @@ static void doGet(HttpRequest req, HttpR + print_status(req, res, 1); + } else if(ACTION(STATUS2)) { + print_status(req, res, 2); +- } else if(ACTION(DOACTION)) { +- handle_do_action(req, res); + } else { +- handle_action(req, res); ++ handle_service(req, res); + } + + } +@@ -455,15 +459,33 @@ static void do_runtime(HttpRequest req, + StringBuffer_append(res->outputbuffer, + "<table id='buttons'><tr>"); + StringBuffer_append(res->outputbuffer, +- "<td style='color:red;'><form method=POST action='_runtime'>Stop Monit http server? " +- "<input type=hidden name='action' value='stop'><input type=submit value='Go'></form></td>"); +- StringBuffer_append(res->outputbuffer, +- "<td><form method=POST action='_runtime'>Force validate now? <input type=hidden name='action' value='validate'>" +- "<input type=submit value='Go'></form></td>"); ++ "<td style='color:red;'>" ++ "<form method=POST action='_runtime'>Stop Monit http server? " ++ "<input type=hidden name='securitytoken' value='%s'>" ++ "<input type=hidden name='action' value='stop'>" ++ "<input type=submit value='Go'>" ++ "</form>" ++ "</td>", ++ res->token); ++ StringBuffer_append(res->outputbuffer, ++ "<td>" ++ "<form method=POST action='_runtime'>Force validate now? " ++ "<input type=hidden name='securitytoken' value='%s'>" ++ "<input type=hidden name='action' value='validate'>" ++ "<input type=submit value='Go'>" ++ "</form>" ++ "</td>", ++ res->token); + + if(Run.dolog && !Run.use_syslog) { + StringBuffer_append(res->outputbuffer, +- "<td><form method=GET action='_viewlog'>View Monit logfile? <input type=submit value='Go'></form></td>"); ++ "<td>" ++ "<form method=POST action='_viewlog'>View Monit logfile? " ++ "<input type=hidden name='securitytoken' value='%s'>" ++ "<input type=submit value='Go'>" ++ "</form>" ++ "</td>", ++ res->token); + } + StringBuffer_append(res->outputbuffer, + "</tr></table>"); +@@ -529,7 +551,18 @@ static void do_viewlog(HttpRequest req, + } + + +-static void handle_action(HttpRequest req, HttpResponse res) { ++static void handle_service(HttpRequest req, HttpResponse res) { ++ char *name = req->url; ++ Service_T s = Util_getService(++name); ++ if (! s) { ++ send_error(res, SC_NOT_FOUND, "There is no service named \"%s\"", name ? name : ""); ++ return; ++ } ++ do_service(req, res, s); ++} ++ ++ ++static void handle_service_action(HttpRequest req, HttpResponse res) { + int doaction; + char *name = req->url; + const char *action; +@@ -569,12 +602,11 @@ static void handle_action(HttpRequest re + } + + +-static void handle_do_action(HttpRequest req, HttpResponse res) { ++static void handle_doaction(HttpRequest req, HttpResponse res) { + Service_T s; + int doaction = ACTION_IGNORE; + const char *action = get_parameter(req, "action"); + const char *token = get_parameter(req, "token"); +- + if(action) { + HttpParameter p; + +@@ -623,8 +655,13 @@ static void handle_do_action(HttpRequest + } + + +-static void handle_run(HttpRequest req, HttpResponse res) { +- ++static void handle_runtime(HttpRequest req, HttpResponse res) { ++ LOCK(Run.mutex) ++ do_runtime(req, res); ++ END_LOCK; ++} ++ ++static void handle_runtime_action(HttpRequest req, HttpResponse res) { + const char *action= get_parameter(req, "action"); + + if(action) { +@@ -645,9 +682,7 @@ static void handle_run(HttpRequest req, + } + } + +- LOCK(Run.mutex) +- do_runtime(req, res); +- END_LOCK; ++ handle_runtime(req, res); + + } + +@@ -1440,29 +1475,47 @@ static void print_buttons(HttpRequest re + /* Start program */ + if(s->start) + StringBuffer_append(res->outputbuffer, +- "<td><form method=POST action=%s>" ++ "<td>" ++ "<form method=POST action=%s>" ++ "<input type=hidden name='securitytoken' value='%s'>" + "<input type=hidden value='start' name=action>" +- "<input type=submit value='Start service'></form></td>", s->name); ++ "<input type=submit value='Start service'>" ++ "</form>" ++ "</td>", s->name, res->token); + /* Stop program */ + if(s->stop) + StringBuffer_append(res->outputbuffer, +- "<td><form method=POST action=%s>" ++ "<td>" ++ "<form method=POST action=%s>" ++ "<input type=hidden name='securitytoken' value='%s'>" + "<input type=hidden value='stop' name=action>" +- "<input type=submit value='Stop service'></form></td>", s->name); ++ "<input type=submit value='Stop service'>" ++ "</form>" ++ "</td>", s->name, res->token); + /* Restart program */ + if(s->start && s->stop) + StringBuffer_append(res->outputbuffer, +- "<td><form method=POST action=%s>" ++ "<td>" ++ "<form method=POST action=%s>" ++ "<input type=hidden name='securitytoken' value='%s'>" + "<input type=hidden value='restart' name=action>" +- "<input type=submit value='Restart service'></form></td>", s->name); ++ "<input type=submit value='Restart service'>" ++ "</form>" ++ "</td>", s->name, res->token); + /* (un)monitor */ + StringBuffer_append(res->outputbuffer, +- "<td><form method=POST action=%s>" ++ "<td>" ++ "<form method=POST action=%s>" ++ "<input type=hidden name='securitytoken' value='%s'>" + "<input type=hidden value='%s' name=action>" +- "<input type=submit value='%s'></form></td></tr></table>", ++ "<input type=submit value='%s'>" ++ "</form>" ++ "</td>", + s->name, ++ res->token, + s->monitor ? "unmonitor" : "monitor", + s->monitor ? "Disable monitoring" : "Enable monitoring"); ++ StringBuffer_append(res->outputbuffer, "</tr></table>"); + } + + +--- a/src/http/processor.c ++++ b/src/http/processor.c +@@ -171,7 +171,7 @@ void add_Impl(void(*doGet)(HttpRequest, + * @param code Error Code to lookup and send + * @param msg Optional error message (may be NULL) + */ +-void send_error(HttpResponse res, int code, const char *msg) { ++void send_error(HttpResponse res, int code, const char *msg, ...) { + char server[STRLEN]; + const char *err= get_status_string(code); + +@@ -198,7 +198,7 @@ void send_error(HttpResponse res, int co + * @param name Header key name + * @param value Header key value + */ +-void set_header(HttpResponse res, const char *name, const char *value) { ++void set_header(HttpResponse res, const char *name, const char *value, ...) { + HttpHeader h= NULL; + + ASSERT(res); +@@ -206,7 +206,10 @@ void set_header(HttpResponse res, const + + NEW(h); + h->name= Str_dup(name); +- h->value= Str_dup(value); ++ va_list ap; ++ va_start(ap, value); ++ h->value= Str_vcat(value, ap); ++ va_end(ap); + if(res->headers) { + HttpHeader n, p; + for( n= p= res->headers; p; n= p, p= p->next) { +@@ -242,7 +245,7 @@ void set_status(HttpResponse res, int co + * @param mime Mime content type, e.g. text/html + */ + void set_content_type(HttpResponse res, const char *mime) { +- set_header(res, "Content-Type", mime); ++ set_header(res, "Content-Type", "%s", mime); + } + + +@@ -409,6 +412,7 @@ static void do_service(Socket_T s) { + + if(res && req) { + if(is_authenticated(req, res)) { ++ set_header(res, "Set-Cookie", "securitytoken=%s; Max-Age=600; HttpOnly; SameSite=strict%s", res->token, Run.httpdssl ? "; Secure" : ""); + if(IS(req->method, METHOD_GET)) { + Impl.doGet(req, res); + } else if(IS(req->method, METHOD_POST)) { +@@ -534,6 +538,7 @@ static HttpResponse create_HttpResponse( + res->is_committed= FALSE; + res->protocol= SERVER_PROTOCOL; + res->status_msg= get_status_string(SC_OK); ++ Util_getToken(res->token); + return res; + } + +@@ -701,6 +706,30 @@ static int is_authenticated(HttpRequest + return FALSE; + } + } ++ if (IS(req->method, METHOD_POST)) { ++ // Check CSRF double-submit cookie (https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Double_Submit_Cookie) ++ const char *cookie = get_header(req, "Cookie"); ++ const char *token = get_parameter(req, "securitytoken"); ++ if (! cookie) { ++ LogError("HttpRequest: access denied -- client [%s]: missing CSRF token cookie\n", NVLSTR(socket_get_remote_host(req->S))); ++ send_error(res, SC_FORBIDDEN, "Invalid CSRF Token"); ++ return FALSE; ++ } ++ if (! token) { ++ LogError("HttpRequest: access denied -- client [%s]: missing CSRF token in HTTP parameter\n", NVLSTR(socket_get_remote_host(req->S))); ++ send_error(res, SC_FORBIDDEN, "Invalid CSRF Token"); ++ return FALSE; ++ } ++ if (! Str_startsWith(cookie, "securitytoken=")) { ++ LogError("HttpRequest: access denied -- client [%s]: no CSRF token in cookie\n", NVLSTR(socket_get_remote_host(req->S))); ++ send_error(res, SC_FORBIDDEN, "Invalid CSRF Token"); ++ return FALSE; ++ } ++ if (Str_compareConstantTime(cookie + 14, token)) { ++ LogError("HttpRequest: access denied -- client [%s]: CSRF token mismatch\n", NVLSTR(socket_get_remote_host(req->S))); ++ send_error(res, SC_FORBIDDEN, "Invalid CSRF Token"); ++ } ++ } + return TRUE; + } + +--- a/src/http/processor.h ++++ b/src/http/processor.h +@@ -88,6 +88,7 @@ typedef struct response { + ssl_connection *ssl; + const char *status_msg; + StringBuffer_T outputbuffer; ++ MD_T token; + } *HttpResponse; + + +@@ -112,8 +113,8 @@ const char *get_status_string(int status + void add_Impl(void(*doGet)(HttpRequest, HttpResponse), void(*doPost)(HttpRequest, HttpResponse)); + void set_content_type(HttpResponse res, const char *mime); + const char *get_header(HttpRequest req, const char *header_name); +-void send_error(HttpResponse, int status, const char *message); ++void send_error(HttpResponse, int status, const char *message, ...) __attribute__((format (printf, 3, 4))); + const char *get_parameter(HttpRequest req, const char *parameter_name); +-void set_header(HttpResponse res, const char *name, const char *value); ++void set_header(HttpResponse res, const char *name, const char *value, ...) __attribute__((format (printf, 3, 4))); + + #endif +--- a/src/util.c ++++ b/src/util.c +@@ -129,6 +129,9 @@ + #include "process.h" + #include "event.h" + ++// libmonit ++# include "system/Time.h" ++ + + struct ad_user { + const char *login; +@@ -1319,15 +1322,23 @@ void Util_printServiceList() { + } + + ++char *Util_getToken(MD_T token) { ++ md5_context_t ctx; ++ char buf[STRLEN]; ++ MD_T digest; ++ snprintf(buf, STRLEN, "%lu%d%lu", (unsigned long)Time_now(), getpid(), random()); ++ md5_init(&ctx); ++ md5_append(&ctx, (const md5_byte_t *)buf, STRLEN - 1); ++ md5_finish(&ctx, (md5_byte_t *)digest); ++ Util_digest2Bytes((unsigned char *)digest, 16, token); ++ return token; ++} ++ + char *Util_monitId(char *idfile) { +- FILE *file = NULL; +- + ASSERT(idfile); +- ++ FILE *file = NULL; + if(! file_exist(idfile)) { +- md5_context_t ctx; +- char buf[STRLEN]; +- MD_T digest; ++ // Generate the unique id + mode_t mask = umask(PRIVATEMASK); + file = fopen(idfile, "w"); + umask(mask); +@@ -1335,13 +1346,7 @@ char *Util_monitId(char *idfile) { + LogError("%s: Error opening the idfile '%s' -- %s\n", prog, idfile, STRERROR); + return NULL; + } +- /* Generate the unique id */ +- snprintf(buf, STRLEN, "%lu%d%lu", (unsigned long)time(NULL), getpid(), random()); +- md5_init(&ctx); +- md5_append(&ctx, (const md5_byte_t *)buf, (int)strlen(buf)); +- md5_finish(&ctx, (md5_byte_t *)digest); +- Util_digest2Bytes((unsigned char *)digest, 16, Run.id); +- fprintf(file, "%s", Run.id); ++ fprintf(file, "%s", Util_getToken(Run.id)); + LogInfo("%s: generated unique Monit id %s and stored to '%s'\n", prog, Run.id, idfile); + } else { + if(! file_isFile(idfile)) { +--- a/src/util.h ++++ b/src/util.h +@@ -77,6 +77,7 @@ int Util_handle0Escapes(char *buf); + * @param digest buffer containing a MD digest + * @param mdlen digest length + * @param result buffer to write the result to. Must be at least 41 bytes long. ++ * @return pointer to result buffer + */ + char *Util_digest2Bytes(unsigned char *digest, int mdlen, MD_T result); + +@@ -166,6 +167,14 @@ void Util_printServiceList(); + + + /** ++ * Get a random token ++ * @param token buffer to store the MD digest ++ * @return pointer to token buffer ++ */ ++char *Util_getToken(MD_T token); ++ ++ ++/** + * Open and read the id from the given idfile. If the idfile doesn't exist, + * generate new id and store it in the id file. + * @param idfile An idfile with full path diff -Nru monit-5.4/debian/patches/series monit-5.4/debian/patches/series --- monit-5.4/debian/patches/series 2012-06-01 12:50:49.000000000 +0200 +++ monit-5.4/debian/patches/series 2016-11-30 12:01:01.000000000 +0100 @@ -2,3 +2,4 @@ 06_contrib.patch 07_spelling.patch 08_hide_low_priority_info_from_stderr.patch +09_CVE-2016-7067.patch
signature.asc
Description: OpenPGP digital signature