It occurred to me recently that it is relatively simple to prevent
"CSRF" attacks against the balancer-handler (see CVE-2007-6420), by
generating a "secret" nonce at startup and requiring the presence of
that secret in the submitted parameters.
Any objections?
Index: modules/proxy/mod_proxy_balancer.c
===================================================================
--- modules/proxy/mod_proxy_balancer.c (revision 627685)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -23,9 +23,12 @@
#include "ap_mpm.h"
#include "apr_version.h"
#include "apr_hooks.h"
+#include "apr_uuid.h"
module AP_MODULE_DECLARE_DATA proxy_balancer_module;
+static apr_uuid_t balancer_nonce;
+
static int proxy_balancer_canon(request_rec *r, char *url)
{
char *host, *path, *search;
@@ -622,6 +625,27 @@
}
}
+static int balancer_init(apr_pool_t *p, apr_pool_t *plog,
+ apr_pool_t *ptemp, server_rec *s)
+{
+ void *data;
+ const char *userdata_key = "mod_proxy_balancer_init";
+
+ /* balancer_init() will be called twice, and if it's a DSO then
+ * all static data from the first call will be lost. Only set up
+ * the static data on the second call. */
+ apr_pool_userdata_get(&data, userdata_key, s->process->pool);
+ if (!data) {
+ apr_pool_userdata_set((const void *)1, userdata_key,
+ apr_pool_cleanup_null, s->process->pool);
+ return OK;
+ }
+
+ apr_uuid_get(&balancer_nonce);
+
+ return OK;
+}
+
/* Manages the loadfactors and member status
*/
static int balancer_handler(request_rec *r)
@@ -635,7 +659,10 @@
int access_status;
int i, n;
const char *name;
+ char nonce[APR_UUID_FORMATTED_LENGTH + 1];
+ apr_uuid_format(nonce, &balancer_nonce);
+
/* is this for us? */
if (strcmp(r->handler, "balancer-manager"))
return DECLINED;
@@ -664,6 +691,14 @@
return HTTP_BAD_REQUEST;
}
}
+
+ /* Check that the supplied nonce matches this server's nonce;
+ * otherwise ignore all parameters, to prevent a CSRF attack. */
+ if ((name = apr_table_get(params, "nonce")) == NULL
+ || strcmp(nonce, name) != 0) {
+ apr_table_clear(params);
+ }
+
if ((name = apr_table_get(params, "b")))
bsel = ap_proxy_get_balancer(r->pool, conf,
apr_pstrcat(r->pool, "balancer://", name, NULL));
@@ -801,6 +836,7 @@
ap_rvputs(r, "<tr>\n<td><a href=\"", r->uri, "?b=",
balancer->name + sizeof("balancer://") - 1, "&w=",
ap_escape_uri(r->pool, worker->name),
+ "&nonce=", nonce,
"\">", NULL);
ap_rvputs(r, worker->name, "</a></td>", NULL);
ap_rvputs(r, "<td>", ap_escape_html(r->pool, worker->s->route),
@@ -864,6 +900,8 @@
ap_rvputs(r, "<input type=hidden name=\"b\" ", NULL);
ap_rvputs(r, "value=\"", bsel->name + sizeof("balancer://") - 1,
"\">\n</form>\n", NULL);
+ ap_rvputs(r, "<input type=hidden name=\"nonce\" value=\"", nonce,
"\">\n",
+ NULL);
ap_rputs("<hr />\n", r);
}
ap_rputs(ap_psignature("",r), r);
@@ -1102,6 +1140,7 @@
*/
static const char *const aszPred[] = { "mpm_winnt.c", "mod_proxy.c", NULL};
/* manager handler */
+ ap_hook_post_config(balancer_init, NULL, NULL, APR_HOOK_MIDDLE);
ap_hook_handler(balancer_handler, NULL, NULL, APR_HOOK_FIRST);
ap_hook_child_init(child_init, aszPred, NULL, APR_HOOK_MIDDLE);
proxy_hook_pre_request(proxy_balancer_pre_request, NULL, NULL,
APR_HOOK_FIRST);
