From: Jim Jagielski Date: Thu, 26 Nov 2015 13:42:42 +0000 (+0000) Subject: Merge r1710095, r1710105, r1711902 from trunk: X-Git-Tag: 2.4.18~44 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a6c311656679385970eaa71ddaeb25d5b9209e5d;p=apache 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 66b3be69ae..c2a210798d 100644 --- 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 63e19dfa62..bc7274999a 100644 --- 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 54bf838f4c..1cd71d6650 100644 --- 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 70bf2937c0..5b2c1e8a0c 100644 --- 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 386596ec69..7fc5b09663 100644 --- 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;