I am pretty sure that this isn't correct, or at least seems like overkill. We should definitely block unix: from being forwarded, but why would we want to block things like a urn: resolver?
To be clear, I'd rather remove all proxy functionality from httpd than suggest to the world that http(s) schemes are the only names that can be proxied through HTTP. It would break the Web architecture, so -1 to that. ....Roy > On Dec 14, 2021, at 7:35 AM, yla...@apache.org wrote: > > Author: ylavic > Date: Tue Dec 14 15:35:56 2021 > New Revision: 1895955 > > URL: http://svn.apache.org/viewvc?rev=1895955&view=rev > Log: > Merge r1895914, r1895921 from trunk: > > *) http: Enforce that fully qualified uri-paths not to be forward-proxied > have an http(s) scheme, and that the ones to be forward proxied have a > hostname, per HTTP specifications. > trunk patch: http://svn.apache.org/r1895914 > http://svn.apache.org/r1895921 > 2.4.x patch: > https://patch-diff.githubusercontent.com/raw/apache/httpd/pull/286.patch > backport PR: https://github.com/apache/httpd/pull/286 > +1: ylavic, minfrin, gbechis > > > mod_proxy: Detect unix: scheme syntax errors at load time. > > * modules/proxy/mod_proxy.c(add_pass, add_member, set_proxy_param, > proxysection): > Check return value of ap_proxy_de_socketfy(). > > * modules/proxy/proxy_util.c(ap_proxy_get_worker_ex): > Check return value of ap_proxy_de_socketfy(). > > > > http: Enforce that fully qualified uri-paths not to be forward-proxied > have an http(s) scheme, and that the ones to be forward proxied have a > hostname, per HTTP specifications. > > The early checks avoid failing the request later on and thus save cycles > for those invalid cases. > > > Submitted by: ylavic > Reviewed by: ylavic, minfrin, gbechis > Closes #286 > > Modified: > httpd/httpd/branches/2.4.x/ (props changed) > httpd/httpd/branches/2.4.x/CHANGES > httpd/httpd/branches/2.4.x/include/ap_mmn.h > httpd/httpd/branches/2.4.x/include/http_protocol.h > httpd/httpd/branches/2.4.x/modules/http/http_request.c > httpd/httpd/branches/2.4.x/modules/http2/h2_request.c > httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c > httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c > httpd/httpd/branches/2.4.x/server/protocol.c > > Propchange: httpd/httpd/branches/2.4.x/ > ------------------------------------------------------------------------------ > Merged /httpd/httpd/trunk:r1895914,1895921 > > Modified: httpd/httpd/branches/2.4.x/CHANGES > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original) > +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Tue Dec 14 15:35:56 2021 > @@ -1,6 +1,10 @@ > -*- coding: utf-8 -*- > Changes with Apache 2.4.52 > > + *) http: Enforce that fully qualified uri-paths not to be forward-proxied > + have an http(s) scheme, and that the ones to be forward proxied have a > + hostname, per HTTP specifications. [Ruediger Pluem, Yann Ylavic] > + > *) OpenSSL autoconf detection improvement: pick up openssl.pc in the > specified openssl path. [Joe Orton] > > > Modified: httpd/httpd/branches/2.4.x/include/ap_mmn.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/include/ap_mmn.h?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/include/ap_mmn.h (original) > +++ httpd/httpd/branches/2.4.x/include/ap_mmn.h Tue Dec 14 15:35:56 2021 > @@ -586,6 +586,7 @@ > * dav_find_attr(). > * 20120211.120 (2.4.51-dev) Add dav_liveprop_elem structure and > * dav_get_liveprop_element(). > + * 20120211.121 (2.4.51-dev) Add ap_post_read_request() > * > */ > > @@ -594,7 +595,7 @@ > #ifndef MODULE_MAGIC_NUMBER_MAJOR > #define MODULE_MAGIC_NUMBER_MAJOR 20120211 > #endif > -#define MODULE_MAGIC_NUMBER_MINOR 120 /* 0...n */ > +#define MODULE_MAGIC_NUMBER_MINOR 121 /* 0...n */ > > /** > * Determine if the server's current MODULE_MAGIC_NUMBER is at least a > > Modified: httpd/httpd/branches/2.4.x/include/http_protocol.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/include/http_protocol.h?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/include/http_protocol.h (original) > +++ httpd/httpd/branches/2.4.x/include/http_protocol.h Tue Dec 14 15:35:56 > 2021 > @@ -96,6 +96,13 @@ AP_DECLARE(void) ap_get_mime_headers(req > AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, > apr_bucket_brigade *bb); > > +/** > + * Run post_read_request hook and validate. > + * @param r The current request > + * @return OK or HTTP_... > + */ > +AP_DECLARE(int) ap_post_read_request(request_rec *r); > + > /* Finish up stuff after a request */ > > /** > > Modified: httpd/httpd/branches/2.4.x/modules/http/http_request.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http/http_request.c?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/modules/http/http_request.c (original) > +++ httpd/httpd/branches/2.4.x/modules/http/http_request.c Tue Dec 14 > 15:35:56 2021 > @@ -680,7 +680,7 @@ static request_rec *internal_internal_re > * to do their thing on internal redirects as well. Perhaps this is a > * misnamed function. > */ > - if ((access_status = ap_run_post_read_request(new))) { > + if ((access_status = ap_post_read_request(new))) { > ap_die(access_status, new); > return NULL; > } > > Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original) > +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Tue Dec 14 15:35:56 > 2021 > @@ -370,7 +370,7 @@ request_rec *h2_request_create_rec(const > ap_add_input_filter_handle(ap_http_input_filter_handle, > NULL, r, r->connection); > > - if ((access_status = ap_run_post_read_request(r))) { > + if ((access_status = ap_post_read_request(r))) { > /* Request check post hooks failed. An example of this would be a > * request for a vhost where h2 is disabled --> 421. > */ > > Modified: httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original) > +++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Tue Dec 14 15:35:56 > 2021 > @@ -775,13 +775,13 @@ static int proxy_detect(request_rec *r) > > /* Ick... msvc (perhaps others) promotes ternary short results to int */ > > - if (conf->req && r->parsed_uri.scheme) { > + if (conf->req && r->parsed_uri.scheme && r->parsed_uri.hostname) { > /* but it might be something vhosted */ > - if (!(r->parsed_uri.hostname > - && !ap_cstr_casecmp(r->parsed_uri.scheme, ap_http_scheme(r)) > - && ap_matches_request_vhost(r, r->parsed_uri.hostname, > - > (apr_port_t)(r->parsed_uri.port_str ? r->parsed_uri.port > - : > ap_default_port(r))))) { > + if (ap_cstr_casecmp(r->parsed_uri.scheme, ap_http_scheme(r)) != 0 > + || !ap_matches_request_vhost(r, r->parsed_uri.hostname, > + (apr_port_t)(r->parsed_uri.port_str > + ? r->parsed_uri.port > + : > ap_default_port(r)))) { > r->proxyreq = PROXYREQ_PROXY; > r->uri = r->unparsed_uri; > r->filename = apr_pstrcat(r->pool, "proxy:", r->uri, NULL); > @@ -2007,6 +2007,7 @@ static const char * > struct proxy_alias *new; > char *f = cmd->path; > char *r = NULL; > + const char *real; > char *word; > apr_table_t *params = apr_table_make(cmd->pool, 5); > const apr_array_header_t *arr; > @@ -2093,6 +2094,10 @@ static const char * > if (r == NULL) { > return "ProxyPass|ProxyPassMatch needs a path when not defined in a > location"; > } > + if (!(real = ap_proxy_de_socketfy(cmd->temp_pool, r))) { > + return "ProxyPass|ProxyPassMatch uses an invalid \"unix:\" URL"; > + } > + > > /* if per directory, save away the single alias */ > if (cmd->path) { > @@ -2109,7 +2114,7 @@ static const char * > } > > new->fake = apr_pstrdup(cmd->pool, f); > - new->real = apr_pstrdup(cmd->pool, ap_proxy_de_socketfy(cmd->pool, r)); > + new->real = apr_pstrdup(cmd->pool, real); > new->flags = flags; > if (worker_type & AP_PROXY_WORKER_IS_MATCH) { > new->regex = ap_pregcomp(cmd->pool, f, AP_REG_EXTENDED); > @@ -2635,6 +2640,7 @@ static const char *add_member(cmd_parms > proxy_worker *worker; > char *path = cmd->path; > char *name = NULL; > + const char *real; > char *word; > apr_table_t *params = apr_table_make(cmd->pool, 5); > const apr_array_header_t *arr; > @@ -2675,6 +2681,9 @@ static const char *add_member(cmd_parms > return "BalancerMember must define balancer name when outside <Proxy > > section"; > if (!name) > return "BalancerMember must define remote proxy server"; > + if (!(real = ap_proxy_de_socketfy(cmd->temp_pool, name))) { > + return "BalancerMember uses an invalid \"unix:\" URL"; > + } > > ap_str_tolower(path); /* lowercase scheme://hostname */ > > @@ -2687,8 +2696,7 @@ static const char *add_member(cmd_parms > } > > /* Try to find existing worker */ > - worker = ap_proxy_get_worker(cmd->temp_pool, balancer, conf, > - ap_proxy_de_socketfy(cmd->temp_pool, name)); > + worker = ap_proxy_get_worker(cmd->temp_pool, balancer, conf, real); > if (!worker) { > ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, cmd->server, APLOGNO(01147) > "Defining worker '%s' for balancer '%s'", > @@ -2785,9 +2793,14 @@ static const char * > } > } > else { > + const char *real; > + > + if (!(real = ap_proxy_de_socketfy(cmd->temp_pool, name))) { > + return "ProxySet uses an invalid \"unix:\" URL"; > + } > + > worker = ap_proxy_get_worker_ex(cmd->temp_pool, NULL, conf, > - ap_proxy_de_socketfy(cmd->temp_pool, > name), > - worker_type); > + real, worker_type); > if (!worker) { > if (in_proxy_section) { > err = ap_proxy_define_worker_ex(cmd->pool, &worker, NULL, > @@ -2930,9 +2943,14 @@ static const char *proxysection(cmd_parm > } > } > else { > + const char *real; > + > + if (!(real = ap_proxy_de_socketfy(cmd->temp_pool, conf->p))) { > + return "<Proxy/ProxyMatch > uses an invalid \"unix:\" URL"; > + } > + > worker = ap_proxy_get_worker_ex(cmd->temp_pool, NULL, sconf, > - > ap_proxy_de_socketfy(cmd->temp_pool, conf->p), > - worker_type); > + real, worker_type); > if (!worker) { > err = ap_proxy_define_worker_ex(cmd->pool, &worker, NULL, > sconf, > conf->p, worker_type); > > Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original) > +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Tue Dec 14 15:35:56 > 2021 > @@ -1742,6 +1742,9 @@ PROXY_DECLARE(proxy_worker *) ap_proxy_g > } > > url = ap_proxy_de_socketfy(p, url); > + if (!url) { > + return NULL; > + } > > c = ap_strchr_c(url, ':'); > if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0') { > > Modified: httpd/httpd/branches/2.4.x/server/protocol.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/server/protocol.c?rev=1895955&r1=1895954&r2=1895955&view=diff > ============================================================================== > --- httpd/httpd/branches/2.4.x/server/protocol.c (original) > +++ httpd/httpd/branches/2.4.x/server/protocol.c Tue Dec 14 15:35:56 2021 > @@ -1548,7 +1548,7 @@ request_rec *ap_read_request(conn_rec *c > /* we may have switched to another server */ > apply_server_config(r); > > - if ((access_status = ap_run_post_read_request(r))) { > + if ((access_status = ap_post_read_request(r))) { > goto die; > } > > @@ -1603,6 +1603,27 @@ ignore: > return NULL; > } > > +AP_DECLARE(int) ap_post_read_request(request_rec *r) > +{ > + int status; > + > + if ((status = ap_run_post_read_request(r))) { > + return status; > + } > + > + /* Enforce http(s) only scheme for non-forward-proxy requests */ > + if (!r->proxyreq > + && r->parsed_uri.scheme > + && (ap_cstr_casecmpn(r->parsed_uri.scheme, "http", 4) != 0 > + || (r->parsed_uri.scheme[4] != '\0' > + && (apr_tolower(r->parsed_uri.scheme[4]) != 's' > + || r->parsed_uri.scheme[5] != '\0')))) { > + return HTTP_BAD_REQUEST; > + } > + > + return OK; > +} > + > /* if a request with a body creates a subrequest, remove original request's > * input headers which pertain to the body which has already been read. > * out-of-line helper function for ap_set_sub_req_protocol. > >
