httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r1768036 - in /httpd/httpd/branches/2.4.x-merge-http-strict: ./ CHANGES include/ap_mmn.h include/http_core.h include/httpd.h modules/http/http_filters.c server/core.c server/protocol.c server/util.c server/vhost.c
Date Wed, 16 Nov 2016 12:32:54 GMT


On 11/16/2016 01:08 PM, William A Rowe Jr wrote:
> On Tue, Nov 8, 2016 at 1:39 PM, Ruediger Pluem <rpluem@apache.org <mailto:rpluem@apache.org>>
wrote:
> 
> 
>     On 11/04/2016 03:20 PM, wrowe@apache.org <mailto:wrowe@apache.org> wrote:
>     > Author: wrowe
>     > Date: Fri Nov  4 14:20:16 2016
>     > New Revision: 1768036
>     >
>     > URL: http://svn.apache.org/viewvc?rev=1768036&view=rev <http://svn.apache.org/viewvc?rev=1768036&view=rev>
>     > Log:
>     > 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
>     > Modified: httpd/httpd/branches/2.4.x-merge-http-strict/server/vhost.c
>     > URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x-merge-http-strict/server/vhost.c?rev=1768036&r1=1768035&r2=1768036&view=diff
>     <http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x-merge-http-strict/server/vhost.c?rev=1768036&r1=1768035&r2=1768036&view=diff>
>     > ==============================================================================
>     > --- httpd/httpd/branches/2.4.x-merge-http-strict/server/vhost.c (original)
>     > +++ httpd/httpd/branches/2.4.x-merge-http-strict/server/vhost.c Fri Nov  4 14:20:16
2016
>     > +    if (conf->http_conformance & AP_HTTP_CONFORMANCE_STRICT) {
>     > +        /*
>     > +         * If we have both hostname from an absoluteURI and a Host header,
>     > +         * we must ignore the Host header (RFC 2616 5.2).
>     > +         * To enforce this, we reset the Host header to the value from the
>     > +         * request line.
>     > +         */
>     > +        if (have_hostname_from_url && host_header != NULL) {
>     > +            const char *info = "Would replace";
>     > +            const char *new = construct_host_header(r, is_v6literal);
>     > +            if (!(conf->http_conformance & AP_HTTP_CONFORMANCE_LOGONLY))
{
>     > +                apr_table_set(r->headers_in, "Host", r->hostname);
> 
>     Hm, why don't we use "new" here instead of r->hostname
> 
>     > +                info = "Replacing";
>     > +            }
>     > +            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02417)
>     > +                          "%s Host header '%s' with host from request uri:
"
>     > +                          "'%s'", info, host_header, new);
>     > +        }
>     > +    }
> 
>  
> Good question, with LOGONLY no longer an option, all that logic got simpler.
> 
> Here's why I think the whole logic is busted and the preserve r->hostname is
> the right thing to do for the outer request (not a child/client request to any
> backend host)...
> 
> I believe this logic to be busted. Given this request;
> 
> GET http://distant-host.com/ HTTP/1.1
> Host: proxy-host
> 
> we would now fail to evaluate the proxy-host virtual host rules.
> 
> This seems like a breaking change to our config. mod_proxy already
> follows this rule of RFC7230 section 5.4;
> 
>    When a proxy receives a request with an absolute-form of
>    request-target, the proxy MUST ignore the received Host header field
>    (if any) and instead replace it with the host information of the
>    request-target.  A proxy that forwards such a request MUST generate a
>    new Host field-value based on the received request-target rather than
>    forward the received Host field-value.
> 
> Section 5.5 of RFC7230 has this to say;
> 
>    Once the effective request URI has been constructed, an origin server
>    needs to decide whether or not to provide service for that URI via
>    the connection in which the request was received.  For example, the
>    request might have been misdirected, deliberately or accidentally,
>    such that the information within a received request-target or Host
>    header field differs from the host or port upon which the connection
>    has been made.  If the connection is from a trusted gateway, that
>    inconsistency might be expected; otherwise, it might indicate an
>    attempt to bypass security filters, trick the server into delivering
>    non-public content, or poison a cache.  See Section 9 for security
>    considerations regarding message routing.
> 
> Section 5.3.1 states;
> 
>    To allow for transition to the absolute-form for all requests in some
>    future version of HTTP, a server MUST accept the absolute-form in
>    requests, even though HTTP/1.1 clients will only send them in
>    requests to proxies.
> 
> It seems to me we should simply trust the Host: header and dump this whole
> mess. If we want to reject requests in absolute form after the proxy modules
> have had a chance to accept them, that wouldn't be a bad solution.
> 


Hm. I am confused now. 5.3.1 seem to state that we need to accept them in any
case, even for non proxy requests. For the proxy case 5.4 seems to be pretty clear
what to todo: Forget about the Host header, take it from the request.
For the other case we can IMHO decide what we want to do accept it (but what has
precedence in this case host or request) or drop / error out. I guess both behaviours
can make sense. The later one as a security check, the former one if we are on a backend
system and have a trusted proxy / reverse proxy aka gateway in front of us.

Regards

RĂ¼diger

Mime
View raw message