Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Subject: Re: svn commit: r1756729 - in /httpd/httpd/trunk:
 docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h server/core.c
 server/protocol.c
To: dev@httpd.apache.org
References: <20160818071507.9D74F3A0E73@svn01-us-west.apache.org>
From: Ruediger Pluem <rpluem@apache.org>
Message-ID: <57B5C5D7.4040507@apache.org>
Date: Thu, 18 Aug 2016 16:27:35 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:43.0) Gecko/20100101
 Firefox/43.0 SeaMonkey/2.40
MIME-Version: 1.0
In-Reply-To: <20160818071507.9D74F3A0E73@svn01-us-west.apache.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
archived-at: Thu, 18 Aug 2016 14:27:40 -0000


On 08/18/2016 09:15 AM, wrowe@apache.org wrote:
> Author: wrowe
> Date: Thu Aug 18 07:15:06 2016
> New Revision: 1756729
> 
> URL: http://svn.apache.org/viewvc?rev=1756729&view=rev
> Log:
> Perform correct, strict parsing of the request line, handling the
> http protocol tag, url and method appropriately, and attempting 
> to extract values even in the presence of unusual whitespace in
> keeping with section 3.5, prior to responding with whatever
> error reply is needed. Conforms to RFC7230 in all respects,
> the section 3.5 optional behavior can be disabled by the user
> with a new HttpProtocolOptions StrictWhitespace flag. In all
> cases, the_request is regenerated from the parsed components
> with exactly two space characters.
> 
> Shift sf's 'strict' method check from the Strict behavior because
> it violates forward proxy logic, adding a new RegisteredMethods
> flag, as it will certainly be useful to some.
> 
> 
> 
> Modified:
>     httpd/httpd/trunk/docs/manual/mod/core.xml
>     httpd/httpd/trunk/include/ap_mmn.h
>     httpd/httpd/trunk/include/http_core.h
>     httpd/httpd/trunk/server/core.c
>     httpd/httpd/trunk/server/protocol.c
> 


> Modified: httpd/httpd/trunk/include/http_core.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_core.h?rev=1756729&r1=1756728&r2=1756729&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/http_core.h (original)
> +++ httpd/httpd/trunk/include/http_core.h Thu Aug 18 07:15:06 2016
> @@ -739,6 +739,16 @@ typedef struct {
>  #define AP_HTTP_CONFORMANCE_STRICT    2
>      char http_conformance;
>  
> +#define AP_HTTP_WHITESPACE_UNSET      0
> +#define AP_HTTP_WHITESPACE_LENIENT    1
> +#define AP_HTTP_WHITESPACE_STRICT     2
> +    char http_whitespace;
> +
> +#define AP_HTTP_METHODS_UNSET         0
> +#define AP_HTTP_METHODS_LENIENT       1
> +#define AP_HTTP_METHODS_REGISTERED    2
> +    char http_methods;
> +
>  #define AP_HTTP_CL_HEAD_ZERO_UNSET    0
>  #define AP_HTTP_CL_HEAD_ZERO_ENABLE   1
>  #define AP_HTTP_CL_HEAD_ZERO_DISABLE  2

Any particular reason, why this was implemented in a way that requires a major bump and thus is not backportable?


> 
> Modified: httpd/httpd/trunk/server/protocol.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/protocol.c?rev=1756729&r1=1756728&r2=1756729&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/protocol.c (original)
> +++ httpd/httpd/trunk/server/protocol.c Thu Aug 18 07:15:06 2016
> @@ -559,18 +559,32 @@ AP_CORE_DECLARE(void) ap_parse_uri(reque
>      }
>  }
>  
> -static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
> +/* get the length of the field name for logging, but no more than 80 bytes */
> +#define LOG_NAME_MAX_LEN 80
> +static int field_name_len(const char *field)
>  {
> -    const char *ll;
> -    const char *uri;
> -    const char *pro;
> +    const char *end = ap_strchr_c(field, ':');
> +    if (end == NULL || end - field > LOG_NAME_MAX_LEN)
> +        return LOG_NAME_MAX_LEN;
> +    return end - field;
> +}
>  
> +static int read_request_line(request_rec *r, apr_bucket_brigade *bb)
> +{
> +    enum {
> +        rrl_none, rrl_badmethod, rrl_badwhitespace, rrl_excesswhitespace,
> +        rrl_missinguri, rrl_badprotocol, rrl_trailingtext
> +    } deferred_error = rrl_none;
> +    char *ll;
> +    char *uri;
>      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 = DEFAULT_LIMIT_BLANK_LINES;
>      core_server_config *conf = ap_get_core_module_config(r->server->module_config);
>      int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);
> +    const char *badwhitespace = strict ? "\t\n\v\f\r" : "\n\v\f\r";
> +    int strictspaces = (conf->http_whitespace == AP_HTTP_WHITESPACE_STRICT);
>  
>      /* Read past empty lines until we get a real request line,
>       * a read error, the connection closes (EOF), or we timeout.
> @@ -626,111 +640,211 @@ static int read_request_line(request_rec
>      }
>  
>      r->request_time = apr_time_now();
> -    ll = r->the_request;
> -    r->method = ap_getword_white(r->pool, &ll);
>  
> -    uri = ap_getword_white(r->pool, &ll);
> +    r->method = r->the_request;
> +    if (apr_isspace(*r->method))
> +        deferred_error = rrl_excesswhitespace; 
> +    for ( ; apr_isspace(*r->method); ++r->method)
> +        ; 
>  
> -    if (!*r->method || !*uri) {
> -        r->status    = HTTP_BAD_REQUEST;
> -        r->proto_num = HTTP_VERSION(1,0);
> -        r->protocol  = "HTTP/1.0";
> -        return 0;
> +    if (strict) {
> +        ll = (char*) ap_scan_http_token(r->method);
> +        if ((ll == r->method) || (*ll && !apr_isspace(*ll))) {
> +            deferred_error = rrl_badmethod;
> +            ll = strpbrk(ll, "\t\n\v\f\r ");
> +        }
>      }
> -
> -    /* Provide quick information about the request method as soon as known */
> -
> -    r->method_number = ap_method_number_of(r->method);
> -    if (r->method_number == M_GET && r->method[0] == 'H') {
> -        r->header_only = 1;
> +    else {
> +        ll = strpbrk(r->method, "\t\n\v\f\r ");
> +    }
> +    if (!ll) {
> +        if (deferred_error == rrl_none)
> +            deferred_error = rrl_missinguri;
> +        r->protocol = uri = "";
> +        len = 0;
> +        goto rrl_done;
>      }
>  
> -    ap_parse_uri(r, uri);
> -    if (r->status != HTTP_OK) {
> -        r->proto_num = HTTP_VERSION(1,0);
> -        r->protocol  = "HTTP/1.0";
> -        return 0;
> +    if (strictspaces && ll[0] && (ll[0] != ' ' || apr_isspace(ll[1]))
> +            && deferred_error == rrl_none) {
> +        deferred_error = rrl_excesswhitespace; 
> +    }
> +    for (uri = ll; apr_isspace(*uri); ++uri) 
> +        if (strchr(badwhitespace, *uri) && deferred_error == rrl_none)
> +            deferred_error = rrl_badwhitespace; 
> +    *ll = '\0';
> +    if (!*uri && deferred_error == rrl_none)
> +        deferred_error = rrl_missinguri;
> +
> +    if (!(ll = strpbrk(uri, " \t\n\v\f\r"))) {
> +        r->protocol = "";
> +        len = 0;
> +        goto rrl_done;
> +    }
> +    for (r->protocol = ll; apr_isspace(*r->protocol); ++r->protocol) 
> +        if (strchr(badwhitespace, *r->protocol) && deferred_error == rrl_none
> +                && deferred_error == rrl_none)

Double deferred_error == rrl_none ?


Regards

Rüdiger