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

Add an option to enforce stricter HTTP conformance 

This is a first stab, the checks will likely have to be revised.
For now, we check

 * if the request line contains control characters
 * if the request uri has fragment or username/password
 * that the request method is standard or registered with RegisterHttpMethod
 * that the request protocol is of the form HTTP/[1-9]+.[0-9]+,
   or missing for 0.9
 * if there is garbage in the request line after the protocol
 * if any request header contains control characters
 * if any request header has an empty name
 * for the host name in the URL or Host header:
   - if an IPv4 dotted decimal address: Reject octal or hex values, require
     exactly four parts
   - if a DNS host name: Reject non-alphanumeric characters besides '.' and
     '-'. As a side effect, this rejects multiple Host headers.
 * if any response header contains control characters
 * if any response header has an empty name
 * that the Location response header (if present) has a valid scheme and is
   absolute

If we have a host name both from the URL and the Host header, we replace the
Host header with the value from the URL to enforce RFC conformance.

There is a log-only mode, but the loglevels of the logged messages need some
thought/work. Currently, the  checks for incoming data log for 'core' and the
checks for outgoing data log for 'http'. Maybe we need a way to configure the
loglevels separately from the core/http loglevels.

change protocol number parsing in strict mode according to HTTPbis draft
- only accept single digit version components
- don't accept white-space after protocol specification

Clean up comment, fix log tags.
Submitted by: sf
Backports: r1426877, r1426879, r1426988, r1426992



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

CHANGES

+3 −0
Original line number Diff line number Diff line
@@ -2,6 +2,9 @@ 


Changes with Apache 2.4.24



  *) core, http: Extend HttpProtocol with an option to enforce stricter HTTP

     conformance or to only log the found problems. [Stefan Fritsch]



  *) core: Correctly parse an IPv6 literal host specification in an absolute

     URL in the request line. [Stefan Fritsch]

include/ap_mmn.h

+2 −0
Original line number Diff line number Diff line
@@ -488,6 +488,8 @@ 
 * 20120211.66 (2.4.24-dev) Rename ap_proxy_check_backend() to

 *                          ap_proxy_check_connection().

 * 20120211.67 (2.5.0-dev)  Add http09_enable to core_server_config

 *                          Add http_conformance to core_server_config,

 *                          add ap_has_cntrl()

 */



#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */

include/http_core.h

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

    char http09_enable;



#define AP_HTTP_CONFORMANCE_UNSET     0

#define AP_HTTP_CONFORMANCE_LIBERAL   1

#define AP_HTTP_CONFORMANCE_STRICT    2

#define AP_HTTP_CONFORMANCE_LOGONLY   4

    char http_conformance;

} core_server_config;



/* for AddOutputFiltersByType in core.c */

include/httpd.h

+9 −0
Original line number Diff line number Diff line
@@ -2317,6 +2317,15 @@ AP_DECLARE(char *) ap_get_exec_line(apr_pool_t *p, 
                                    const char *cmd,

                                    const char * const *argv);



/**

 * Check if string contains a control character

 * @param str the string to check

 * @param srclen length of the data

 * @return 1 if yes, 0 if no control characters

 */

AP_DECLARE(int) ap_has_cntrl(const char *str)

                AP_FN_ATTR_NONNULL_ALL;



#define AP_NORESTART APR_OS_START_USEERR + 1



/**

modules/http/http_filters.c

+89 −2
Original line number Diff line number Diff line
@@ -668,14 +668,91 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 
    return APR_SUCCESS;

}



struct check_header_ctx {

    request_rec *r;

    int error;

};



/* 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;

    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;

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

                      "Response header name '%s' contains control "

                      "characters, aborting request",

                      name);

        return 0;

    }

    if (ap_has_cntrl(val)) {

        ctx->error = 1;

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

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

                      "aborting request: %s",

                      name, val);

        return 0;

    }

    return 1;

}



/**

 * Check headers for HTTP conformance

 * @return 1 if ok, 0 if bad

 */

static APR_INLINE int check_headers(request_rec *r)

{

    const char *loc;

    struct check_header_ctx ctx = { r, 0 };



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

    if (ctx.error)

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



    if ((loc = apr_table_get(r->headers_out, "Location")) != NULL) {

        const char *scheme_end = ap_strchr_c(loc, ':');

        const char *s = loc;



        /*

         * Check that the URI has a valid scheme and is absolute

         * XXX Should we do a full uri parse here?

         */

        if (scheme_end == NULL || scheme_end == loc)

            goto bad;



        do {

            if ((!apr_isalnum(*s) && *s != '.' && *s != '+' && *s != '-')

                || !apr_isascii(*s) ) {

                goto bad;

            }

        } while (++s < scheme_end);



        if (scheme_end[1] != '/' || scheme_end[2] != '/')

            goto bad;

    }



    return 1;



bad:

    ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02431)

                  "Bad Location header in response: '%s', aborting request",

                  loc);

    return 0;

}



typedef struct header_struct {

    apr_pool_t *pool;

    apr_bucket_brigade *bb;

} header_struct;



/* Send a single HTTP header field to the client.  Note that this function

 * is used in calls to table_do(), so their interfaces are co-dependent.

 * In other words, don't change this one without checking table_do in alloc.c.

 * is used in calls to apr_table_do(), so don't change its interface.

 * It returns true unless there was a write error of some kind.

 */

static int form_header_field(header_struct *h,
@@ -1175,6 +1252,7 @@ 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);
@@ -1230,6 +1308,15 @@ 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_STRICT) {

        int ok = check_headers(r);

        if (!ok && !(conf->http_conformance & AP_HTTP_CONFORMANCE_LOGONLY)) {

            ap_die(HTTP_INTERNAL_SERVER_ERROR, r);

            return AP_FILTER_ERROR;

        }

    }



    /*

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

     * Since this will have nasty effects on HTTP/1.1 caches, force