Re: svn commit: r1769965 - /httpd/httpd/trunk/server/vhost.c

2016-11-16 Thread William A Rowe Jr
On Wed, Nov 16, 2016 at 1:52 PM, Ruediger Pluem  wrote:

>
> On 11/16/2016 01:05 PM, wr...@apache.org wrote:
> > Author: wrowe
> > Date: Wed Nov 16 12:05:53 2016
> > New Revision: 1769965
> >
> > URL: http://svn.apache.org/viewvc?rev=1769965&view=rev
> > Log:
> > Actually cause the Host header to be overridden, as noted by rpluem,
> > and simplify now that there isn't a log-only mode.
> >
> > 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.
> >
> > Modified:
> > httpd/httpd/trunk/server/vhost.c
> >
> > Modified: httpd/httpd/trunk/server/vhost.c
> > URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/
> vhost.c?rev=1769965&r1=1769964&r2=1769965&view=diff
> > 
> ==
> > --- httpd/httpd/trunk/server/vhost.c (original)
> > +++ httpd/httpd/trunk/server/vhost.c Wed Nov 16 12:05:53 2016
> > @@ -1165,13 +1165,11 @@ AP_DECLARE(void) ap_update_vhost_from_he
> >   * request line.
> >   */
> >  if (have_hostname_from_url && host_header != NULL) {
> > -const char *info = "Would replace";
> > -const char *new = construct_host_header(r, is_v6literal);
> > -apr_table_set(r->headers_in, "Host", r->hostname);
>
> IMHO the old code was wrong because r->hostname misses the surrounding []
> in case of IPV6 literals,
> but otherwise I see no change in logic here: Host part of the request
> still takes precedence over Host header.
>

Ok, I misread your original post, I thought you were pointing out that
r->hostname
is the Host: header value.


> > -info = "Replacing";
> > +const char *repl = construct_host_header(r, is_v6literal);
> > +apr_table_set(r->headers_in, "Host", repl);
> >  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);
> > +  "Replacing host header '%s' with host '%s'
> given "
> > +  "in the request uri", host_header, repl);
> >  }
> >  }
> >
> >
> >
> >
>
> Doesn't this need to get added to the large conformance backport proposal?


Added, or discarded entirely, let's resume on the other discussion thread.


Re: svn commit: r1769965 - /httpd/httpd/trunk/server/vhost.c

2016-11-16 Thread Ruediger Pluem


On 11/16/2016 01:05 PM, wr...@apache.org wrote:
> Author: wrowe
> Date: Wed Nov 16 12:05:53 2016
> New Revision: 1769965
> 
> URL: http://svn.apache.org/viewvc?rev=1769965&view=rev
> Log:
> Actually cause the Host header to be overridden, as noted by rpluem,
> and simplify now that there isn't a log-only mode.
> 
> 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.
> 
> 
> 
> Modified:
> httpd/httpd/trunk/server/vhost.c
> 
> Modified: httpd/httpd/trunk/server/vhost.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/vhost.c?rev=1769965&r1=1769964&r2=1769965&view=diff
> ==
> --- httpd/httpd/trunk/server/vhost.c (original)
> +++ httpd/httpd/trunk/server/vhost.c Wed Nov 16 12:05:53 2016
> @@ -1165,13 +1165,11 @@ AP_DECLARE(void) ap_update_vhost_from_he
>   * request line.
>   */
>  if (have_hostname_from_url && host_header != NULL) {
> -const char *info = "Would replace";
> -const char *new = construct_host_header(r, is_v6literal);
> -apr_table_set(r->headers_in, "Host", r->hostname);

IMHO the old code was wrong because r->hostname misses the surrounding [] in 
case of IPV6 literals,
but otherwise I see no change in logic here: Host part of the request still 
takes precedence over Host header.

> -info = "Replacing";
> +const char *repl = construct_host_header(r, is_v6literal);
> +apr_table_set(r->headers_in, "Host", repl);
>  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);
> +  "Replacing host header '%s' with host '%s' given "
> +  "in the request uri", host_header, repl);
>  }
>  }
>  
> 
> 
> 

Doesn't this need to get added to the large conformance backport proposal?

Regards

RĂ¼diger