httpd-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From wr...@apache.org
Subject svn commit: r1005669 - in /httpd/httpd/branches/2.2.x: CHANGES STATUS modules/filters/mod_reqtimeout.c server/connection.c
Date Thu, 07 Oct 2010 23:51:56 GMT
Author: wrowe
Date: Thu Oct  7 23:51:55 2010
New Revision: 1005669

URL: http://svn.apache.org/viewvc?rev=1005669&view=rev
Log:
SECURITY: CVE-2010-1623 (cve.mitre.org)
Fix a denial of service attack against mod_reqtimeout.
[Stefan Fritsch]

mod_req-timeout/core: Backport bugfixes from trunk up to r935339:
    - Do not wrongly enforce timeouts for mod_proxy's backend
      connections and other protocol handlers (like mod_ftp).
    - Enforce the timeout for AP_MODE_GETLINE.
    - If there is a timeout, shorten the lingering close time from 30 to
      2 seconds (involves a change in the core).

Backports: r921378, r921526, r922407, r923418, r923429, r925986, r928881
           r933341, r933547, r935339, r983116, r984985&view=rev
Submitted by: sf
Reviewed by: wrowe, trawick

Modified:
    httpd/httpd/branches/2.2.x/CHANGES
    httpd/httpd/branches/2.2.x/STATUS
    httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c
    httpd/httpd/branches/2.2.x/server/connection.c

Modified: httpd/httpd/branches/2.2.x/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/CHANGES?rev=1005669&r1=1005668&r2=1005669&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/CHANGES [utf-8] (original)
+++ httpd/httpd/branches/2.2.x/CHANGES [utf-8] Thu Oct  7 23:51:55 2010
@@ -1,6 +1,15 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.2.17
 
+  *) SECURITY: CVE-2010-1623 (cve.mitre.org)
+     Fix a denial of service attack against mod_reqtimeout.
+     [Stefan Fritsch]
+
+  *) mod_reqtimeout: Do not wrongly enforce timeouts for mod_proxy's backend
+     connections and other protocol handlers (like mod_ftp). Enforce the
+     timeout for AP_MODE_GETLINE. If there is a timeout, shorten the lingering
+     close time from 30 to 2 seconds. [Stefan Fritsch]
+
   *) Proxy balancer: support setting error status according to HTTP response
      code from a backend.  PR 48939.  [Daniel Ruggeri <DRuggeri primary.net>]
 

Modified: httpd/httpd/branches/2.2.x/STATUS
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS?rev=1005669&r1=1005668&r2=1005669&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/STATUS (original)
+++ httpd/httpd/branches/2.2.x/STATUS Thu Oct  7 23:51:55 2010
@@ -103,34 +103,6 @@ PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
       http://mail-archives.apache.org/mod_mbox/httpd-dev/201010.mbox/%3C20101007095048.GA5423@redhat.com%3E
       [/NOT/i redundant]
 
-  * mod_req-timeout/core: Backport bugfixes from trunk up to r935339:
-    - Do not wrongly enforce timeouts for mod_proxy's backend
-      connections and other protocol handlers (like mod_ftp).
-    - Enforce the timeout for AP_MODE_GETLINE.
-    - If there is a timeout, shorten the lingering close time from 30 to
-      2 seconds (involves a change in the core).
-    Trunk patch: http://svn.apache.org/viewvc?rev=921378&view=rev
-                 http://svn.apache.org/viewvc?rev=921526&view=rev
-                 http://svn.apache.org/viewvc?rev=922407&view=rev
-                 http://svn.apache.org/viewvc?rev=923418&view=rev
-                 http://svn.apache.org/viewvc?rev=923429&view=rev
-                 http://svn.apache.org/viewvc?rev=925986&view=rev
-                 http://svn.apache.org/viewvc?rev=928881&view=rev
-                 http://svn.apache.org/viewvc?rev=933341&view=rev
-                 http://svn.apache.org/viewvc?rev=933547&view=rev
-                 http://svn.apache.org/viewvc?rev=935339&view=rev
-                 http://svn.apache.org/viewvc?rev=983116&view=rev
-                 http://svn.apache.org/viewvc?rev=984985&view=rev
-    2.2.x patch: http://people.apache.org/~sf/mod_reqtimeout_up_to_r983116.diff
-    sf: It would be nice if someone could review this specifically WRT Windows
-        compatibility.
-    +1: sf, wrowe
-
-    Additionally backport fix for CVE-2010-1623:
-    Trunk patch: http://svn.apache.org/viewvc?rev=1003626&view=rev
-    2.2.x patch: trunk patch works on top of mod_reqtimeout_up_to_r983116.diff
-    combined 2.2.x patch: http://people.apache.org/~sf/mod_reqtimeout-2.2.x-v3.diff
-    +1: sf, wrowe
 
  PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]

Modified: httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c?rev=1005669&r1=1005668&r2=1005669&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c (original)
+++ httpd/httpd/branches/2.2.x/modules/filters/mod_reqtimeout.c Thu Oct  7 23:51:55 2010
@@ -14,15 +14,18 @@
  * limitations under the License.
  */
 
+#define CORE_PRIVATE
 #include "httpd.h"
 #include "http_config.h"
 #include "http_request.h"
 #include "http_connection.h"
 #include "http_protocol.h"
 #include "http_log.h"
+#include "http_core.h"
 #include "util_filter.h"
 #define APR_WANT_STRFUNC
 #include "apr_strings.h"
+#include "apr_version.h"
 
 module AP_MODULE_DECLARE_DATA reqtimeout_module;
 
@@ -38,6 +41,7 @@ typedef struct
     apr_time_t body_rate_factor;
 } reqtimeout_srv_cfg;
 
+/* this struct is used both as conn_config and as filter context */
 typedef struct
 {
     apr_time_t timeout_at;
@@ -47,14 +51,11 @@ typedef struct
     int new_max_timeout;
     int in_keep_alive;
     char *type;
+    apr_socket_t *socket;
     apr_time_t rate_factor;
+    apr_bucket_brigade *tmpbb;
 } reqtimeout_con_cfg;
 
-typedef struct
-{
-    apr_socket_t *socket;
-} reqtimeout_ctx;
-
 static const char *const reqtimeout_filter_name = "reqtimeout";
 
 static void extend_timeout(reqtimeout_con_cfg *ccfg, apr_bucket_brigade *bb)
@@ -74,24 +75,95 @@ static void extend_timeout(reqtimeout_co
     }
 }
 
+static apr_status_t check_time_left(reqtimeout_con_cfg *ccfg,
+                                    apr_time_t *time_left_p)
+{
+    *time_left_p = ccfg->timeout_at - apr_time_now();
+    if (*time_left_p <= 0)
+        return APR_TIMEUP;
+    
+    if (*time_left_p < apr_time_from_sec(1)) {
+        *time_left_p = apr_time_from_sec(1);
+    }
+    return APR_SUCCESS;
+}
+
+static apr_status_t have_lf_or_eos(apr_bucket_brigade *bb)
+{
+    apr_bucket *b = APR_BRIGADE_LAST(bb);
+
+    for ( ; b != APR_BRIGADE_SENTINEL(bb) ; b = APR_BUCKET_PREV(b) ) {
+    	const char *str;
+    	apr_size_t len;
+    	apr_status_t rv;
+
+        if (APR_BUCKET_IS_EOS(b))
+            return APR_SUCCESS;
+
+        if (APR_BUCKET_IS_METADATA(b))
+            continue;
+
+        rv = apr_bucket_read(b, &str, &len, APR_BLOCK_READ);
+        if (rv != APR_SUCCESS)
+            return rv;
+
+        if (len == 0)
+            continue;
+
+        if (str[len-1] == APR_ASCII_LF)
+            return APR_SUCCESS;
+    }
+    return APR_INCOMPLETE;
+}
+
+/*
+ * Append bbIn to bbOut and merge small buckets, to avoid DoS by high memory
+ * usage
+ */
+static apr_status_t brigade_append(apr_bucket_brigade *bbOut, apr_bucket_brigade *bbIn)
+{
+    while (!APR_BRIGADE_EMPTY(bbIn)) {
+        apr_bucket *e = APR_BRIGADE_FIRST(bbIn);
+        const char *str;
+        apr_size_t len;
+        apr_status_t rv;
+
+        rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
+        if (rv != APR_SUCCESS) {
+            return rv;
+        }
+
+        APR_BUCKET_REMOVE(e);
+        if (APR_BUCKET_IS_METADATA(e) || len > APR_BUCKET_BUFF_SIZE/4) {
+            APR_BRIGADE_INSERT_TAIL(bbOut, e);
+        }
+        else {
+            if (len > 0) {
+                rv = apr_brigade_write(bbOut, NULL, NULL, str, len);
+                if (rv != APR_SUCCESS) {
+                    apr_bucket_destroy(e);
+                    return rv;
+                }
+            }
+            apr_bucket_destroy(e);
+        }
+    }
+    return APR_SUCCESS;
+}
+
+
+#define MIN(x,y) ((x) < (y) ? (x) : (y))
 static apr_status_t reqtimeout_filter(ap_filter_t *f,
                                       apr_bucket_brigade *bb,
                                       ap_input_mode_t mode,
                                       apr_read_type_e block,
                                       apr_off_t readbytes)
 {
-    reqtimeout_ctx *ctx;
     apr_time_t time_left;
     apr_time_t now;
     apr_status_t rv;
     apr_interval_time_t saved_sock_timeout = -1;
-    reqtimeout_con_cfg *ccfg;
-
-    ctx = f->ctx;
-    AP_DEBUG_ASSERT(ctx != NULL);
-
-    ccfg = ap_get_module_config(f->c->conn_config, &reqtimeout_module);
-    AP_DEBUG_ASSERT(ccfg != NULL);
+    reqtimeout_con_cfg *ccfg = f->ctx;
 
     if (ccfg->in_keep_alive) {
         /* For this read, the normal keep-alive timeout must be used */
@@ -114,13 +186,14 @@ static apr_status_t reqtimeout_filter(ap
         return ap_get_brigade(f->next, bb, mode, block, readbytes);
     }
 
-    time_left = ccfg->timeout_at - now;
-    if (time_left <= 0) {
-        ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, f->c,
-                      "Request %s read timeout", ccfg->type);
-        return APR_TIMEUP;
+    if (!ccfg->socket) {
+        ccfg->socket = ap_get_module_config(f->c->conn_config, &core_module);
     }
 
+    rv = check_time_left(ccfg, &time_left);
+    if (rv != APR_SUCCESS)
+        goto out;
+
     if (block == APR_NONBLOCK_READ || mode == AP_MODE_INIT
         || mode == AP_MODE_EATCRLF) {
         rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
@@ -130,41 +203,116 @@ static apr_status_t reqtimeout_filter(ap
         return rv;
     }
 
-    if (time_left < apr_time_from_sec(1)) {
-        time_left = apr_time_from_sec(1);
-    }
+    rv = apr_socket_timeout_get(ccfg->socket, &saved_sock_timeout);
+    AP_DEBUG_ASSERT(rv == APR_SUCCESS);
 
-    rv = apr_socket_timeout_get(ctx->socket, &saved_sock_timeout);
+    rv = apr_socket_timeout_set(ccfg->socket, MIN(time_left, saved_sock_timeout));
     AP_DEBUG_ASSERT(rv == APR_SUCCESS);
 
-    if (saved_sock_timeout >= time_left) {
-        rv = apr_socket_timeout_set(ctx->socket, time_left);
-        AP_DEBUG_ASSERT(rv == APR_SUCCESS);
-    }
-    else {
-        saved_sock_timeout = -1;
-    }
+    if (mode == AP_MODE_GETLINE) {
+        /*
+         * For a blocking AP_MODE_GETLINE read, apr_brigade_split_line()
+         * would loop until a whole line has been read. As this would make it
+         * impossible to enforce a total timeout, we only do non-blocking
+         * reads.
+         */
+        apr_off_t remaining = HUGE_STRING_LEN;
+        do {
+            apr_off_t bblen;
+#if APR_MAJOR_VERSION < 2
+            apr_int32_t nsds;
+            apr_interval_time_t poll_timeout;
+            apr_pollfd_t pollset;
+#endif
+
+            rv = ap_get_brigade(f->next, bb, AP_MODE_GETLINE, APR_NONBLOCK_READ, remaining);
+            if (rv != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(rv)) {
+                break;
+            }
+
+            if (!APR_BRIGADE_EMPTY(bb)) {
+                if (ccfg->min_rate > 0) {
+                    extend_timeout(ccfg, bb);
+                }
+
+                rv = have_lf_or_eos(bb);
+                if (rv != APR_INCOMPLETE) {
+                    break;
+                }
+
+                rv = apr_brigade_length(bb, 1, &bblen);
+                if (rv != APR_SUCCESS) {
+                    break;
+                }
+                remaining -= bblen;
+                if (remaining <= 0) {
+                    break;
+                }
+
+                /* Haven't got a whole line yet, save what we have ... */
+                if (!ccfg->tmpbb) {
+                    ccfg->tmpbb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
+                }
+                rv = brigade_append(ccfg->tmpbb, bb);
+                if (rv != APR_SUCCESS)
+                    break;
+            }
+
+            /* ... and wait for more */
+#if APR_MAJOR_VERSION < 2
+            pollset.p = f->c->pool;
+            pollset.desc_type = APR_POLL_SOCKET;
+            pollset.reqevents = APR_POLLIN|APR_POLLHUP;
+            pollset.desc.s = ccfg->socket;
+            apr_socket_timeout_get(ccfg->socket, &poll_timeout);
+            rv = apr_poll(&pollset, 1, &nsds, poll_timeout);
+#else
+            rv = apr_socket_wait(ccfg->socket, APR_WAIT_READ);
+#endif
+            if (rv != APR_SUCCESS)
+                break;
+
+            rv = check_time_left(ccfg, &time_left);
+            if (rv != APR_SUCCESS)
+                break;
+
+            rv = apr_socket_timeout_set(ccfg->socket,
+                                   MIN(time_left, saved_sock_timeout));
+            AP_DEBUG_ASSERT(rv == APR_SUCCESS);
 
-    rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
+        } while (1);
 
-    if (saved_sock_timeout != -1) {
-        apr_socket_timeout_set(ctx->socket, saved_sock_timeout);
-    }
+        if (ccfg->tmpbb)
+            APR_BRIGADE_PREPEND(bb, ccfg->tmpbb);
 
-    if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
-        extend_timeout(ccfg, bb);
+    }
+    else {
+        /* mode != AP_MODE_GETLINE */
+        rv = ap_get_brigade(f->next, bb, mode, block, readbytes);
+        if (ccfg->min_rate > 0 && rv == APR_SUCCESS) {
+            extend_timeout(ccfg, bb);
+        }
     }
 
+    apr_socket_timeout_set(ccfg->socket, saved_sock_timeout);
+
+out:
     if (APR_STATUS_IS_TIMEUP(rv)) {
         ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, f->c,
                       "Request %s read timeout", ccfg->type);
+        /*
+         * If we allow a normal lingering close, the client may keep this
+         * process/thread busy for another 30s (MAX_SECS_TO_LINGER).
+         * Therefore we tell ap_lingering_close() to shorten this period to
+         * 2s (SECONDS_TO_LINGER).
+         */
+        apr_table_setn(f->c->notes, "short-lingering-close", "1");
     }
     return rv;
 }
 
-static int reqtimeout_pre_conn(conn_rec *c, void *csd)
+static int reqtimeout_init(conn_rec *c)
 {
-    reqtimeout_ctx *ctx;
     reqtimeout_con_cfg *ccfg;
     reqtimeout_srv_cfg *cfg;
 
@@ -173,12 +321,9 @@ static int reqtimeout_pre_conn(conn_rec 
     AP_DEBUG_ASSERT(cfg != NULL);
     if (cfg->header_timeout <= 0 && cfg->body_timeout <= 0) {
         /* not configured for this vhost */
-        return OK;
+        return DECLINED;
     }
 
-    ctx = apr_pcalloc(c->pool, sizeof(reqtimeout_ctx));
-    ctx->socket = csd;
-
     ccfg = apr_pcalloc(c->pool, sizeof(reqtimeout_con_cfg));
     ccfg->new_timeout = cfg->header_timeout;
     ccfg->new_max_timeout = cfg->header_max_timeout;
@@ -187,8 +332,9 @@ static int reqtimeout_pre_conn(conn_rec 
     ccfg->rate_factor = cfg->header_rate_factor;
     ap_set_module_config(c->conn_config, &reqtimeout_module, ccfg);
 
-    ap_add_input_filter("reqtimeout", ctx, NULL, c);
-    return OK;
+    ap_add_input_filter("reqtimeout", ccfg, NULL, c);
+    /* we are not handling the connection, we just do initialization */
+    return DECLINED;
 }
 
 static int reqtimeout_after_headers(request_rec *r)
@@ -198,7 +344,7 @@ static int reqtimeout_after_headers(requ
         ap_get_module_config(r->connection->conn_config, &reqtimeout_module);
 
     if (ccfg == NULL) {
-        /* not configured for this vhost */
+        /* not configured for this connection */
         return OK;
     }
 
@@ -208,11 +354,13 @@ static int reqtimeout_after_headers(requ
 
     ccfg->timeout_at = 0;
     ccfg->max_timeout_at = 0;
-    ccfg->new_timeout = cfg->body_timeout;
-    ccfg->new_max_timeout = cfg->body_max_timeout;
-    ccfg->min_rate = cfg->body_min_rate;
-    ccfg->rate_factor = cfg->body_rate_factor;
-    ccfg->type = "body";
+    if (r->method_number != M_CONNECT) {
+        ccfg->new_timeout = cfg->body_timeout;
+        ccfg->new_max_timeout = cfg->body_max_timeout;
+        ccfg->min_rate = cfg->body_min_rate;
+        ccfg->rate_factor = cfg->body_rate_factor;
+        ccfg->type = "body";
+    }
 
     return OK;
 }
@@ -224,7 +372,7 @@ static int reqtimeout_after_body(request
         ap_get_module_config(r->connection->conn_config, &reqtimeout_module);
 
     if (ccfg == NULL) {
-        /* not configured for this vhost */
+        /* not configured for this connection */
         return OK;
     }
 
@@ -406,7 +554,16 @@ static void reqtimeout_hooks(apr_pool_t 
      */
     ap_register_input_filter(reqtimeout_filter_name, reqtimeout_filter, NULL,
                              AP_FTYPE_CONNECTION + 8);
-    ap_hook_pre_connection(reqtimeout_pre_conn, NULL, NULL, APR_HOOK_MIDDLE);
+
+    /*
+     * mod_reqtimeout needs to be called before ap_process_http_request (which
+     * is run at APR_HOOK_REALLY_LAST) but after all other protocol modules.
+     * This ensures that it only influences normal http connections and not
+     * e.g. mod_ftp. Also, if mod_reqtimeout used the pre_connection hook, it
+     * would be inserted on mod_proxy's backend connections.
+     */
+    ap_hook_process_connection(reqtimeout_init, NULL, NULL, APR_HOOK_LAST);
+
     ap_hook_post_read_request(reqtimeout_after_headers, NULL, NULL,
                               APR_HOOK_MIDDLE);
     ap_hook_log_transaction(reqtimeout_after_body, NULL, NULL,

Modified: httpd/httpd/branches/2.2.x/server/connection.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/server/connection.c?rev=1005669&r1=1005668&r2=1005669&view=diff
==============================================================================
--- httpd/httpd/branches/2.2.x/server/connection.c (original)
+++ httpd/httpd/branches/2.2.x/server/connection.c Thu Oct  7 23:51:55 2010
@@ -154,8 +154,20 @@ AP_DECLARE(void) ap_lingering_close(conn
             break;
 
         if (timeup == 0) {
-            /* First time through; calculate now + 30 seconds. */
-            timeup = apr_time_now() + apr_time_from_sec(MAX_SECS_TO_LINGER);
+            /*
+             * First time through;
+             * calculate now + 30 seconds (MAX_SECS_TO_LINGER).
+             *
+             * If some module requested a shortened waiting period, only wait
+             * for 2s (SECONDS_TO_LINGER). This is useful for mitigating
+             * certain DoS attacks.
+             */
+            if (apr_table_get(c->notes, "short-lingering-close")) {
+                timeup = apr_time_now() + apr_time_from_sec(SECONDS_TO_LINGER);
+            }
+            else {
+                timeup = apr_time_now() + apr_time_from_sec(MAX_SECS_TO_LINGER);
+            }
             continue;
         }
     } while (apr_time_now() < timeup);



Mime
View raw message