httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject [PATCH] prevent CSRF in mod_proxy_balancer
Date Tue, 11 Mar 2008 14:23:17 GMT
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);

Mime
View raw message