Merge r1710095, r1710105, r1711902 from trunk:

core: Limit to ten the number of tolerated empty lines between request,
and consume them before the pipelining check to avoid possible response
delay when reading the next request without flushing.

Before this commit, the maximum number of empty lines was the same as
configured LimitRequestFields, defaulting to 100, which was way too much.
We now use a fixed/hard limit of 10 (DEFAULT_LIMIT_BLANK_LINES).

check_pipeline() is changed to check for (up to the limit) and comsume the
trailing [CR]LFs so that they won't be interpreted as pipelined requests,
otherwise we would block on the next read without flushing data, and hence
possibly delay pending response(s) until the next/real request comes in or
the keepalive timeout expires.

Finally, when the maximum number of empty line is reached in
read_request_line(), or that request line does not contains at least a method
and an (valid) URI, we can fail early and avoid some failure detected in
further processing.

core: follow up to r1710095.
Simplify logic in check_pipeline(), and log unexpected errors.

core: follow up to r1710095, r1710105.
We can do this in a single (no inner) loop, and simplify again the logic.

Submitted by: ylavic
Reviewed/backported by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1716651 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 66b3be69ae6d68b19be9d60ae213d2c30c7cdb5e..c2a210798da3cf591297e76b864e0f0a51c982c0 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -64,6 +64,10 @@ Changes with Apache 2.4.18
   *) core/util_script: making REDIRECT_URL a full URL is now opt-in
      via new 'QualifyRedirectURL' directive.
 
+  *) core: Limit to ten the number of tolerated empty lines between request,
+     and consume them before the pipelining check to avoid possible response
+     delay when reading the next request without flushing.  [Yann Ylavic]
+
   *) mod_ssl: Extend expression parser registration to support ssl variables
      in any expression using mod_rewrite syntax "%{SSL:VARNAME}" or function
      syntax "ssl(VARNAME)". [Rainer Jung]

diff --git a/STATUS b/STATUS
index 63e19dfa62a18511b68b06566cd48928916dfca6..bc7274999ac95b55e9763fbf7e7decb290299493 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -111,18 +111,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) core: Limit to ten the number of tolerated empty lines between request,
-     and consume them before the pipelining check to avoid possible response
-     delay when reading the next request without flushing.
-     trunk patch: http://svn.apache.org/r1710095
-                  http://svn.apache.org/r1710105
-                  http://svn.apache.org/r1711902
-     2.4.x patch: http://people.apache.org/~ylavic/httpd-2.4.x-check_pipeline_blank_lines.patch
-                  (trunk works, meant to ease review)
-     +1: ylavic, minfrin, jim
-     icing: test 3 fails for me in t/security/CVE-2005-3357.t
-     ylavic: not related (at least not the cause), fixed in r1715023.
-
   *) mod_ssl: For the "SSLStaplingReturnResponderErrors off" case, make sure
               to only staple responses with certificate status "good"
      trunk patch: https://svn.apache.org/r1711728

diff --git a/include/httpd.h b/include/httpd.h
index 54bf838f4cbe7a7f6a9b3b2d15e3a788d02090a5..1cd71d66507ff2139b8a7542a6028f3473e8a4c9 100644 (file)
--- a/include/httpd.h
+++ b/include/httpd.h
@@ -200,6 +200,10 @@ extern "C" {
 #ifndef DEFAULT_LIMIT_REQUEST_FIELDS
 #define DEFAULT_LIMIT_REQUEST_FIELDS 100
 #endif
+/** default/hard limit on number of leading/trailing empty lines */
+#ifndef DEFAULT_LIMIT_BLANK_LINES
+#define DEFAULT_LIMIT_BLANK_LINES 10
+#endif
 
 /**
  * The default default character set name to add if AddDefaultCharset is

diff --git a/modules/http/http_request.c b/modules/http/http_request.c
index 70bf2937c08dbd6cdbb346c50267ce25d5ab6146..5b2c1e8a0c7b4990c06b42df8bf7c2b010d6b47a 100644 (file)
--- a/modules/http/http_request.c
+++ b/modules/http/http_request.c
@@ -230,21 +230,93 @@ AP_DECLARE(void) ap_die(int type, request_rec *r)
 
 static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb)
 {
-    if (c->keepalive != AP_CONN_CLOSE && !c->aborted) {
-        apr_status_t rv;
-
-        AP_DEBUG_ASSERT(APR_BRIGADE_EMPTY(bb));
-        rv = ap_get_brigade(c->input_filters, bb, AP_MODE_SPECULATIVE,
-                            APR_NONBLOCK_READ, 1);
+    apr_status_t rv;
+    int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES;
+    ap_input_mode_t mode = AP_MODE_SPECULATIVE;
+    apr_size_t cr = 0;
+    char buf[2];
+
+    c->data_in_input_filters = 0;
+    while (c->keepalive != AP_CONN_CLOSE && !c->aborted) {
+        apr_size_t len = cr + 1;
+
+        apr_brigade_cleanup(bb);
+        rv = ap_get_brigade(c->input_filters, bb, mode,
+                            APR_NONBLOCK_READ, len);
         if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
             /*
              * Error or empty brigade: There is no data present in the input
              * filter
              */
-            c->data_in_input_filters = 0;
+            if (mode == AP_MODE_READBYTES) {
+                /* Unexpected error, stop with this connection */
+                ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(02967)
+                              "Can't consume pipelined empty lines");
+                c->keepalive = AP_CONN_CLOSE;
+            }
+            break;
+        }
+
+        /* Ignore trailing blank lines (which must not be interpreted as
+         * pipelined requests) up to the limit, otherwise we would block
+         * on the next read without flushing data, and hence possibly delay
+         * pending response(s) until the next/real request comes in or the
+         * keepalive timeout expires.
+         */
+        rv = apr_brigade_flatten(bb, buf, &len);
+        if (rv != APR_SUCCESS || len != cr + 1) {
+            int log_level;
+            if (mode == AP_MODE_READBYTES) {
+                /* Unexpected error, stop with this connection */
+                c->keepalive = AP_CONN_CLOSE;
+                log_level = APLOG_ERR;
+            }
+            else {
+                /* Let outside (non-speculative/blocking) read determine
+                 * where this possible failure comes from (metadata,
+                 * morphed EOF socket => empty bucket? debug only here).
+                 */
+                c->data_in_input_filters = 1;
+                log_level = APLOG_DEBUG;
+            }
+            ap_log_cerror(APLOG_MARK, log_level, rv, c, APLOGNO(02968)
+                          "Can't check pipelined data");
+            break;
+        }
+
+        if (mode == AP_MODE_READBYTES) {
+            mode = AP_MODE_SPECULATIVE;
+            cr = 0;
+        }
+        else if (cr) {
+            AP_DEBUG_ASSERT(len == 2 && buf[0] == APR_ASCII_CR);
+            if (buf[1] == APR_ASCII_LF) {
+                mode = AP_MODE_READBYTES;
+                num_blank_lines--;
+            }
+            else {
+                c->data_in_input_filters = 1;
+                break;
+            }
         }
         else {
-            c->data_in_input_filters = 1;
+            if (buf[0] == APR_ASCII_LF) {
+                mode = AP_MODE_READBYTES;
+                num_blank_lines--;
+            }
+            else if (buf[0] == APR_ASCII_CR) {
+                cr = 1;
+            }
+            else {
+                c->data_in_input_filters = 1;
+                break;
+            }
+        }
+        /* Enough blank lines with this connection?
+         * Stop and don't recycle it.
+         */
+        if (num_blank_lines < 0) {
+            c->keepalive = AP_CONN_CLOSE;
         }
     }
 }

diff --git a/server/protocol.c b/server/protocol.c
index 386596ec6936acc53f9c0a55dd97e378a8d9573f..7fc5b0966306ce365e29815b16683bc5ac73876c 100644 (file)
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -561,12 +561,7 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
     unsigned int major = 1, minor = 0;   /* Assume HTTP/1.0 if non-"HTTP" protocol */
     char http[5];
     apr_size_t len;
-    int num_blank_lines = 0;
-    int max_blank_lines = r->server->limit_req_fields;
-
-    if (max_blank_lines <= 0) {
-        max_blank_lines = DEFAULT_LIMIT_REQUEST_FIELDS;
-    }
+    int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES;
 
     /* Read past empty lines until we get a real request line,
      * a read error, the connection closes (EOF), or we timeout.
@@ -613,7 +608,7 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
             r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
             return 0;
         }
-    } while ((len <= 0) && (++num_blank_lines < max_blank_lines));
+    } while ((len <= 0) && (--num_blank_lines >= 0));
 
     if (APLOGrtrace5(r)) {
         ap_log_rerror(APLOG_MARK, APLOG_TRACE5, 0, r,
@@ -627,6 +622,13 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
 
     uri = ap_getword_white(r->pool, &ll);
 
+    if (!*r->method || !*uri) {
+        r->status    = HTTP_BAD_REQUEST;
+        r->proto_num = HTTP_VERSION(1,0);
+        r->protocol  = apr_pstrdup(r->pool, "HTTP/1.0");
+        return 0;
+    }
+
     /* Provide quick information about the request method as soon as known */
 
     r->method_number = ap_method_number_of(r->method);
@@ -635,6 +637,9 @@ static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
     }
 
     ap_parse_uri(r, uri);
+    if (r->status != HTTP_OK) {
+        return 0;
+    }
 
     if (ll[0]) {
         r->assbackwards = 0;