Commit 7dcdbde5 authored by William A. Rowe Jr's avatar William A. Rowe Jr
Browse files

Dropped the never-released ap_has_cntrls() as it had very limited 

and inefficient application at that, added ap_scan_vchar_obstext()
to accomplish a similar purpose.

Dropped HttpProtocolOptions StrictURL option, this will be better
handled in the future with a specific directive and perhaps multiple
levels of scrutiny, use ap_scan_vchar_obstext() to simply ensure there
are no control characters or whitespace within the URI.

Changed the scanning of the response header table by check_headers()
to follow the same rulesets as reading request headers. Disallow any
CTL character within a response header value, and any CTL or whitespace
in response header field name, even in strict mode.

Apply HttpProtocolOptions Strict to chunk header parsing, invalid
whitespace is invalid, line termination must follow CRLF convention.
Submitted by: wrowe
Backport: r1764961,1765112-1765115 

When redrawing the parser, ap_get_http_token looked to be useful, but there's
no application for this yet in httpd, so hold off adding this function when
we backport the enhancements. ap_scan_http_token was entirely sufficient.
If the community wants this new function, we can add it when backporting
work is complete.

This patch, and the earlier patches Friday actually demanded an mmn major
bump due to struct member changes. In any final backport, new members must
be added to the end of the struct to retain an mmn minor designation.
Submitted by: wrowe
Backport: r1765451

Appears we cannot disallow this whitespace, since the chunk BNF coexisted
with the implied *LWS rule, before RFC7230 eliminated the later. Whether
this is actually OWS or BWS is an editorial decision beyond our pay grade.
Submitted by: wrowe
Backport: r1765475




git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x-merge-http-strict@1769672 13f79535-47bb-0310-9956-ffa450edef68
parent f8eec8bb

docs/manual/mod/core.xml

+13 −22
Original line number Diff line number Diff line
@@ -1241,9 +1241,9 @@ EnableSendfile On 
<directivesynopsis>

<name>HttpProtocolOptions</name>

<description>Modify restrictions on HTTP Request Messages</description>

<syntax>HttpProtocolOptions [Strict|Unsafe] [StrictURL|UnsafeURL]

 [RegisteredMethods|LenientMethods] [Allow0.9|Require1.0]</syntax>

<default>HttpProtocolOptions Strict StrictURL LenientMethods Allow0.9</default>

<syntax>HttpProtocolOptions [Strict|Unsafe] [RegisteredMethods|LenientMethods]

 [Allow0.9|Require1.0]</syntax>

<default>HttpProtocolOptions Strict LenientMethods Allow0.9</default>

<contextlist><context>server config</context>

<context>virtual host</context></contextlist>

<compatibility>2.2.32 or 2.4.24 and later</compatibility>
@@ -1255,11 +1255,11 @@ EnableSendfile On 
    (<a href="https://tools.ietf.org/html/rfc7230#section-3.2"

      >RFC 7230 &sect;3.2</a>), which are now applied by default or using

    the <code>Strict</code> option. Due to legacy modules, applications or

    custom user-agents which must be deperecated, <code>Unsafe</code>

    and <code>UnsafeURL</code> options have been added to revert to the legacy

    behaviors. These rules are applied prior to request processing, so must be

    configured at the global or default (first) matching virtual host section,

    by IP/port interface and not by name, to be honored.</p>

    custom user-agents which must be deperecated the <code>Unsafe</code>

    option has been added to revert to the legacy behaviors. These rules

    are applied prior to request processing, so must be configured at the

    global or default (first) matching virtual host section, by IP/port

    interface (and not by name) to be honored.</p>



    <p>Prior to the introduction of this directive, the Apache HTTP Server

    request message parsers were tolerant of a number of forms of input
@@ -1277,21 +1277,12 @@ EnableSendfile On 
    mode, and the strict whitespace suggested by section 3.5 is enforced

    and cannot be relaxed.</p>



    <p><a href="https://tools.ietf.org/html/rfc3986#section-2.2"

         >RFC 3986 &sect;2.2 and 2.3</a> define "Reserved Characters" and

    "Unreserved Characters". All other character octets are required to

    be %XX encoded under this spec, and RFC7230 defers to these requirements.

    By default the <code>StrictURI</code> option will reject all requests 

    containing invalid characters. This rule can be relaxed with the

    <code>UnsafeURI</code> option to support badly written user-agents.</p>

    

    <p>Users are strongly cautioned against toggling the <code>Unsafe</code>

    or <code>UnsafeURI</code> modes of operation, particularly on

    outward-facing, publicly accessible server deployments.

    If an interface is required for faulty monitoring or other custom service

    consumers running on an intranet, users should toggle only those Unsafe

    options which are necessary, and only on a specific virtual host configured

    to service only their internal private network.</p>

    mode of operation, particularly on outward-facing, publicly accessible

    server deployments.  If an interface is required for faulty monitoring

    or other custom service consumers running on an intranet, users should

    toggle the Unsafe option only on a specific virtual host configured

    to service their internal private network.</p>



    <p>Reviewing the messages logged to the <directive>ErrorLog</directive>,

    configured with <directive>LogLevel</directive> <code>debug</code> level,

include/ap_mmn.h

+3 −2
Original line number Diff line number Diff line
@@ -489,8 +489,9 @@ 
 *                          ap_proxy_check_connection().

 * 20120211.67 (2.4.24-dev) Add http09_enable, http_conformance, and

 *                          http_methods to core_server_config

 *                          Add ap_has_cntrl(), ap_get_http_token()

 *                          Add ap_scan_http_field_[content|token]()

 *                          Add ap_scan_http_field_token(),

 *                          ap_scan_http_field_content(),

 *                          and ap_scan_vchar_obstext()

 *                          Replaced fold boolean with with multiple bit flags

 *                          to ap_[r]getline()

 */

include/http_core.h

+0 −5
Original line number Diff line number Diff line
@@ -741,11 +741,6 @@ typedef struct { 
#define AP_HTTP_METHODS_REGISTERED    2

    char http_methods;



#define AP_HTTP_URI_UNSET             0

#define AP_HTTP_URI_UNSAFE            1

#define AP_HTTP_URI_STRICT            2

    char http_stricturi;



} core_server_config;



/* for AddOutputFiltersByType in core.c */

include/httpd.h

+4 −14
Original line number Diff line number Diff line
@@ -1600,22 +1600,12 @@ AP_DECLARE(const char *) ap_scan_http_field_content(const char *ptr); 
 */

AP_DECLARE(const char *) ap_scan_http_token(const char *ptr);



/* Scan a string for valid URI characters per RFC3986, and 

 * return a pointer to the first non-URI character encountered.

/* Scan a string for visible ASCII (0x21-0x7E) or obstext (0x80+)

 * and return a pointer to the first SP/CTL/NUL character encountered.

 * @param ptr The string to scan

 * @return A pointer to the first non-token character.

 */

AP_DECLARE(const char *) ap_scan_http_uri_safe(const char *ptr);



/* Retrieve a token, advancing the pointer to the first non-token character

 * and returning a copy of the token string.

 * @param ptr The string to scan. On return, this points to the first non-token

 *  character encountered, or NULL if *ptr was not a token character

 * @return A copy of the token string

 * @note The caller must handle leading and trailing whitespace as applicable

 *  and evaluate the terminating character.

 * @return A pointer to the first SP/CTL character.

 */

AP_DECLARE(char *) ap_get_http_token(apr_pool_t *p, const char **ptr);

AP_DECLARE(const char *) ap_scan_vchar_obstext(const char *ptr);



/**

 * Retrieve an array of tokens in the format "1#token" defined in RFC2616. Only

modules/http/http_filters.c

+59 −31
Original line number Diff line number Diff line
@@ -126,14 +126,15 @@ static apr_status_t bail_out_on_error(http_ctx_t *ctx, 


/**

 * Parse a chunk line with optional extension, detect overflow.

 * There are two error cases:

 * There are several error cases:

 *  1) If the chunk link is misformatted, APR_EINVAL is returned.

 *  1) If the conversion would require too many bits, APR_EGENERAL is returned.

 *  2) If the conversion used the correct number of bits, but an overflow

 *     caused only the sign bit to flip, then APR_ENOSPC is returned.

 * In general, any negative number can be considered an overflow error.

 * A negative chunk length always indicates an overflow error.

 */

static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer,

                                     apr_size_t len, int linelimit)

                                     apr_size_t len, int linelimit, int strict)

{

    apr_size_t i = 0;
@@ -146,6 +147,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, 
        if (ctx->state == BODY_CHUNK_END

                || ctx->state == BODY_CHUNK_END_LF) {

            if (c == LF) {

                if (strict && (ctx->state != BODY_CHUNK_END_LF)) {

                    /*

                     * CR missing before LF.

                     */

                    return APR_EINVAL;

                }

                ctx->state = BODY_CHUNK;

            }

            else if (c == CR && ctx->state == BODY_CHUNK_END) {
@@ -153,7 +160,7 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, 
            }

            else {

                /*

                 * LF expected.

                 * CRLF expected.

                 */

                return APR_EINVAL;

            }
@@ -180,6 +187,12 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, 
        }



        if (c == LF) {

            if (strict && (ctx->state != BODY_CHUNK_LF)) {

                /*

                 * CR missing before LF.

                 */

                return APR_EINVAL;

            }

            if (ctx->remaining) {

                ctx->state = BODY_CHUNK_DATA;

            }
@@ -201,13 +214,14 @@ static apr_status_t parse_chunk_size(http_ctx_t *ctx, const char *buffer, 
        }

        else if (ctx->state == BODY_CHUNK_EXT) {

            /*

             * Control chars (but tabs) are invalid.

             * Control chars (excluding tabs) are invalid.

             * TODO: more precisely limit input

             */

            if (c != '\t' && apr_iscntrl(c)) {

                return APR_EINVAL;

            }

        }

        else if (c == ' ' || c == '\t') {

        else if (!strict && (c == ' ' || c == '\t')) {

            /* Be lenient up to 10 BWS (term from rfc7230 - 3.2.3).

             */

            ctx->state = BODY_CHUNK_CR;
@@ -324,7 +338,10 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 
                            ap_input_mode_t mode, apr_read_type_e block,

                            apr_off_t readbytes)

{

    core_server_config *conf;

    core_server_config *conf =

        (core_server_config *) ap_get_module_config(f->r->server->module_config,

                                                    &core_module);

    int strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);

    apr_bucket *e;

    http_ctx_t *ctx = f->ctx;

    apr_status_t rv;
@@ -332,9 +349,6 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 
    apr_bucket_brigade *bb;

    int again;



    conf = (core_server_config *)

        ap_get_module_config(f->r->server->module_config, &core_module);



    /* just get out of the way of things we don't want. */

    if (mode != AP_MODE_READBYTES && mode != AP_MODE_GETLINE) {

        return ap_get_brigade(f->next, b, mode, block, readbytes);
@@ -526,7 +540,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 
                    if (rv == APR_SUCCESS) {

                        parsing = 1;

                        rv = parse_chunk_size(ctx, buffer, len,

                                f->r->server->limit_req_fieldsize);

                                f->r->server->limit_req_fieldsize, strict);

                    }

                    if (rv != APR_SUCCESS) {

                        ap_log_rerror(APLOG_MARK, APLOG_INFO, rv, f->r, APLOGNO(01590)
@@ -670,32 +684,49 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 


struct check_header_ctx {

    request_rec *r;

    int error;

    int strict;

};



/* check a single header, to be used with apr_table_do() */

static int check_header(void *arg, const char *name, const char *val)

{

    struct check_header_ctx *ctx = arg;

    const char *test;



    if (name[0] == '\0') {

        ctx->error = 1;

        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02428)

                      "Empty response header name, aborting request");

        return 0;

    }

    if (ap_has_cntrl(name)) {

        ctx->error = 1;



    if (ctx->strict) { 

        test = ap_scan_http_token(name);

    }

    else {

        test = ap_scan_vchar_obstext(name);

    }

    if (*test) {

        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02429)

                      "Response header name '%s' contains control "

                      "Response header name '%s' contains invalid "

                      "characters, aborting request",

                      name);

        return 0;

    }

    if (ap_has_cntrl(val)) {

        ctx->error = 1;



    if (ctx->strict) { 

        test = ap_scan_http_field_content(val);

    }

    else {

        /* Simply terminate scanning on a CTL char, allowing whitespace */

        test = val;

        do {

            test = ap_scan_vchar_obstext(test);

        } while (*test == ' ' || *test == '\t');

    }

    if (*test) {

        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, ctx->r, APLOGNO(02430)

                      "Response header '%s' contains control characters, "

                      "aborting request: %s",

                      "Response header '%s' value of '%s' contains invalid "

                      "characters, aborting request",

                      name, val);

        return 0;

    }
@@ -709,11 +740,13 @@ static int check_header(void *arg, const char *name, const char *val) 
static APR_INLINE int check_headers(request_rec *r)

{

    const char *loc;

    struct check_header_ctx ctx = { 0, 0 };

    struct check_header_ctx ctx;

    core_server_config *conf =

            ap_get_core_module_config(r->server->module_config);



    ctx.r = r;

    apr_table_do(check_header, &ctx, r->headers_out, NULL);

    if (ctx.error)

    ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE);

    if (!apr_table_do(check_header, &ctx, r->headers_out, NULL))

        return 0; /* problem has been logged by check_header() */



    if ((loc = apr_table_get(r->headers_out, "Location")) != NULL) {
@@ -1245,7 +1278,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, 
    header_filter_ctx *ctx = f->ctx;

    const char *ctype;

    ap_bucket_error *eb = NULL;

    core_server_config *conf;



    AP_DEBUG_ASSERT(!r->main);
@@ -1301,14 +1333,10 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, 
                                           r->headers_out);

    }



    conf = ap_get_core_module_config(r->server->module_config);

    if (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE) {

        int ok = check_headers(r);

        if (!ok) {

    if (!check_headers(r)) {

        ap_die(HTTP_INTERNAL_SERVER_ERROR, r);

        return AP_FILTER_ERROR;

    }

    }



    /*

     * Remove the 'Vary' header field if the client can't handle it.