From 58cedec02cb60fdcb6d47a59965e788c705ce951 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Mon, 13 Dec 2021 18:07:22 +0000 Subject: [PATCH 1/2] 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(). git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1895914 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 5c49a85c126d23f89fe02531d12da74ce33a0d92) --- modules/proxy/mod_proxy.c | 32 +++++++++++++++++++++++++------- modules/proxy/proxy_util.c | 3 +++ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c index 3fb84c85935..0a80477f30d 100644 --- a/modules/proxy/mod_proxy.c +++ b/modules/proxy/mod_proxy.c @@ -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 *cmd, void *dummy, const char *arg) 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 *cmd, void *dummy, const char *arg) return "BalancerMember must define balancer name when outside 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 *cmd, void *dummy, const char *arg) } /* 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_parms *cmd, void *mconfig, const char *arg) } } else { + const char *real; + + if (!(real = ap_proxy_de_socketfy(cmd->temp_pool, conf->p))) { + return " 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); diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index a3cf5460487..b4f6dcfadc6 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -1742,6 +1742,9 @@ PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker_ex(apr_pool_t *p, } 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') { From 567a1829bf0859c09c088850069094bf0ccff957 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Mon, 13 Dec 2021 18:55:18 +0000 Subject: [PATCH 2/2] 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. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1895921 13f79535-47bb-0310-9956-ffa450edef68 (cherry picked from commit 3ec0ffb9e1ac05622b97a7afd6992dd2bd41ce38) --- changes-entries/http_enforcements.txt | 3 +++ include/ap_mmn.h | 3 ++- include/http_protocol.h | 7 +++++++ modules/http/http_request.c | 2 +- modules/http2/h2_request.c | 2 +- modules/proxy/mod_proxy.c | 12 ++++++------ server/protocol.c | 23 ++++++++++++++++++++++- 7 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 changes-entries/http_enforcements.txt diff --git a/changes-entries/http_enforcements.txt b/changes-entries/http_enforcements.txt new file mode 100644 index 00000000000..3e16f109f2f --- /dev/null +++ b/changes-entries/http_enforcements.txt @@ -0,0 +1,3 @@ + *) 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. [Yann Ylavic] diff --git a/include/ap_mmn.h b/include/ap_mmn.h index fe24261ee87..90ff1a86a6f 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -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 diff --git a/include/http_protocol.h b/include/http_protocol.h index 9ccac893fcb..20bd2022266 100644 --- a/include/http_protocol.h +++ b/include/http_protocol.h @@ -96,6 +96,13 @@ AP_DECLARE(void) ap_get_mime_headers(request_rec *r); 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 */ /** diff --git a/modules/http/http_request.c b/modules/http/http_request.c index c9ae5af2864..d59cfe25999 100644 --- a/modules/http/http_request.c +++ b/modules/http/http_request.c @@ -680,7 +680,7 @@ static request_rec *internal_internal_redirect(const char *new_uri, * 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; } diff --git a/modules/http2/h2_request.c b/modules/http2/h2_request.c index 7c4fb95ea48..9ff6feb675f 100644 --- a/modules/http2/h2_request.c +++ b/modules/http2/h2_request.c @@ -370,7 +370,7 @@ request_rec *h2_request_create_rec(const h2_request *req, conn_rec *c) 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. */ diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c index 0a80477f30d..85d7ce2e6c1 100644 --- a/modules/proxy/mod_proxy.c +++ b/modules/proxy/mod_proxy.c @@ -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); diff --git a/server/protocol.c b/server/protocol.c index 3d74c5b3058..2214f72b5a4 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -1548,7 +1548,7 @@ request_rec *ap_read_request(conn_rec *conn) /* 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 @@ request_rec *ap_read_request(conn_rec *conn) 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.