You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2008/05/30 13:49:31 UTC
svn commit: r661666 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_balancer.c
Author: jorton
Date: Fri May 30 04:49:31 2008
New Revision: 661666
URL: http://svn.apache.org/viewvc?rev=661666&view=rev
Log:
Prevent CSRF attacks against the balancer-manager (CVE-2007-6420)
* modules/proxy/mod_proxy_balancer.c (balancer_init): New function.
(balancer_handler): Place a nonce in the form output, and check that
the submitted form data includes that nonce.
(ap_proxy_balancer_register_hook): Register the new post_config hook.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=661666&r1=661665&r2=661666&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Fri May 30 04:49:31 2008
@@ -2,6 +2,10 @@
Changes with Apache 2.3.0
[ When backported to 2.2.x, remove entry from this file ]
+ *) SECURITY: CVE-2007-6420 (cve.mitre.org)
+ mod_proxy_balancer: Prevent CSRF attacks against the balancer-manager
+ interface. [Joe Orton]
+
*) mod_proxy_http: Do not forward requests with 'Expect: 100-continue' to
known HTTP/1.0 servers. Return 'Expectation failed' (417) instead.
[Ruediger Pluem]
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=661666&r1=661665&r2=661666&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Fri May 30 04:49:31 2008
@@ -21,9 +21,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;
@@ -619,6 +622,27 @@
}
}
+/* post_config hook: */
+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 during startup. So, only
+ * set up the static data the second time through. */
+ 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)
@@ -632,6 +656,9 @@
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"))
@@ -661,6 +688,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));
@@ -798,6 +833,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),
@@ -861,6 +897,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);
@@ -1099,6 +1137,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);
Re: svn commit: r661666 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_balancer.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jun 6, 2008, at 10:47 AM, Joe Orton wrote:
> On Sat, May 31, 2008 at 12:00:55AM +0200, Ruediger Pluem wrote:
>> On 05/30/2008 01:49 PM, jorton@apache.org wrote:
>>> URL: http://svn.apache.org/viewvc?rev=661666&view=rev
>>> Log:
>>> Prevent CSRF attacks against the balancer-manager (CVE-2007-6420)
> ...
>>> @@ -619,6 +622,27 @@
>>> }
>>> }
>>> +/* post_config hook: */
>>> +static int balancer_init(apr_pool_t *p, apr_pool_t *plog,
>>> + apr_pool_t *ptemp, server_rec *s)
>>> +{
> ...
>>> +
>>> + apr_uuid_get(&balancer_nonce);
>>
>> Why don't we do apr_uuid_format already here and store the string
>> directly?
>
> Sorry I didn't get to this sooner! No reason at all - I've changed
> the
> code as you suggested in r663967; thanks for the review. (Since
> this is
> not performance critical code I think the 2.2.x backport is fine as-
> is)
>
I'll propose after some testing, so if we have time before
the T&R, we could possibly get it in.
Re: svn commit: r661666 - in /httpd/httpd/trunk: CHANGES
modules/proxy/mod_proxy_balancer.c
Posted by Joe Orton <jo...@redhat.com>.
On Sat, May 31, 2008 at 12:00:55AM +0200, Ruediger Pluem wrote:
> On 05/30/2008 01:49 PM, jorton@apache.org wrote:
>> URL: http://svn.apache.org/viewvc?rev=661666&view=rev
>> Log:
>> Prevent CSRF attacks against the balancer-manager (CVE-2007-6420)
...
>> @@ -619,6 +622,27 @@
>> }
>> }
>> +/* post_config hook: */
>> +static int balancer_init(apr_pool_t *p, apr_pool_t *plog,
>> + apr_pool_t *ptemp, server_rec *s)
>> +{
...
>> +
>> + apr_uuid_get(&balancer_nonce);
>
> Why don't we do apr_uuid_format already here and store the string directly?
Sorry I didn't get to this sooner! No reason at all - I've changed the
code as you suggested in r663967; thanks for the review. (Since this is
not performance critical code I think the 2.2.x backport is fine as-is)
joe
Re: svn commit: r661666 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_balancer.c
Posted by Ruediger Pluem <rp...@apache.org>.
In the light of getting this into 2.2.9, any update on my question Joe?
Regards
Rüdiger
On 05/31/2008 12:00 AM, Ruediger Pluem wrote:
>
>
> On 05/30/2008 01:49 PM, jorton@apache.org wrote:
>> Author: jorton
>> Date: Fri May 30 04:49:31 2008
>> New Revision: 661666
>>
>> URL: http://svn.apache.org/viewvc?rev=661666&view=rev
>> Log:
>> Prevent CSRF attacks against the balancer-manager (CVE-2007-6420)
>>
>> * modules/proxy/mod_proxy_balancer.c (balancer_init): New function.
>> (balancer_handler): Place a nonce in the form output, and check that
>> the submitted form data includes that nonce.
>> (ap_proxy_balancer_register_hook): Register the new post_config hook.
>>
>> Modified:
>> httpd/httpd/trunk/CHANGES
>> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>>
>
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=661666&r1=661665&r2=661666&view=diff
>>
>> ==============================================================================
>>
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Fri May 30
>> 04:49:31 2008
>> @@ -21,9 +21,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;
>> @@ -619,6 +622,27 @@
>> }
>> }
>>
>> +/* post_config hook: */
>> +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 during startup. So, only
>> + * set up the static data the second time through. */
>> + 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);
>
> Why don't we do apr_uuid_format already here and store the string directly?
>
> Regards
>
> Rüdiger
>
>
Re: svn commit: r661666 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_balancer.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 05/30/2008 01:49 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Fri May 30 04:49:31 2008
> New Revision: 661666
>
> URL: http://svn.apache.org/viewvc?rev=661666&view=rev
> Log:
> Prevent CSRF attacks against the balancer-manager (CVE-2007-6420)
>
> * modules/proxy/mod_proxy_balancer.c (balancer_init): New function.
> (balancer_handler): Place a nonce in the form output, and check that
> the submitted form data includes that nonce.
> (ap_proxy_balancer_register_hook): Register the new post_config hook.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
>
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=661666&r1=661665&r2=661666&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Fri May 30 04:49:31 2008
> @@ -21,9 +21,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;
> @@ -619,6 +622,27 @@
> }
> }
>
> +/* post_config hook: */
> +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 during startup. So, only
> + * set up the static data the second time through. */
> + 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);
Why don't we do apr_uuid_format already here and store the string directly?
Regards
Rüdiger