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

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1895955 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 910a689445c65299366fff618746d56444a039c1..12ad612d5cbe8fd20a7cb6a064578e01071fdcc1 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -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]
 

diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index fe24261ee87c540a090b40e4ca2215dbe06c509d..90ff1a86a6f6f1aeabb2172a8abd0af6cb0bd913 100644 (file)
--- 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 9ccac893fcb5e8d9fe823fcd7d5a99176561d1db..20bd2022266b91c61136c4e1387c90106f362c0e 100644 (file)
--- 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 c9ae5af2864d41532703ab8c9bb44213581b8da9..d59cfe259990058d78915e6d6d0a5048f1a08e77 100644 (file)
--- 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 7c4fb95ea482ac6871ab8dc52da06a6d2e727be9..9ff6feb675f4eb0ae33c1eb521854af05fdbb9d3 100644 (file)
--- 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 3fb84c859357ea0025fd32de1f233444b3145d9c..85d7ce2e6c1e516ddd054dbf20fed599a051c0a7 100644 (file)
--- 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);
@@ -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 <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 *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 "<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);

diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index a3cf54604872a2b6a89360dc1487210a100ff714..b4f6dcfadc6c74bcf2207de60435e09927c778dd 100644 (file)
--- 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') {

diff --git a/server/protocol.c b/server/protocol.c
index 3d74c5b30582b186cc75b088bad789fb4dc96637..2214f72b5a4267560c952c1fc5a872e3482a82bc 100644 (file)
--- 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 @@ 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.