Commit 9f9b598d authored by Jim Jagielski's avatar Jim Jagielski
Browse files

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
parent 419ec2ea

CHANGES

+4 −0
Original line number Diff line number Diff line
@@ -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]

STATUS

+0 −12
Original line number Diff line number Diff line
@@ -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

include/httpd.h

+4 −0
Original line number Diff line number Diff line
@@ -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

modules/http/http_request.c

+80 −8
Original line number Diff line number Diff line
@@ -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;

    int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES;

    ap_input_mode_t mode = AP_MODE_SPECULATIVE;

    apr_size_t cr = 0;

    char buf[2];



        AP_DEBUG_ASSERT(APR_BRIGADE_EMPTY(bb));

        rv = ap_get_brigade(c->input_filters, bb, AP_MODE_SPECULATIVE,

                            APR_NONBLOCK_READ, 1);

    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 {

            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;

        }

    }

}

server/protocol.c

+12 −7
Original line number Diff line number Diff line
@@ -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;