mod_proxy_connect: Implement "AllowCONNECT None".

changes-entries/proxy-connect-none.txt

@@ -0,0 +1,2 @@
+  *) mod_proxy_connect: Implement "AllowCONNECT None", which 
+     disallows CONNECT to all ports.  [Joe Orton]

docs/manual/mod/mod_proxy_connect.xml

@@ -101,8 +101,16 @@ Port ranges available since Apache 2.3.7.</compatibility>
     <directive>AllowCONNECT</directive> directive to override this default and
     allow connections to the listed ports only.</p>
 
-    <p>Set the value to <code>None</code> to disallow
-    <code>CONNECT</code> requests to all ports, including the defaults.</p>
+    <p>Set the value to <code>None</code> to disallow <code>CONNECT</code>
+    requests to all ports, including the defaults.  <code>None</code> cannot
+    be combined with port numbers in the same context.</p>
+
+    <p>Where <directive>AllowCONNECT</directive> is set in both the main
+    server and a virtual host, the lists of ports are merged, so a virtual
+    host may permit additional ports.  A value of <code>None</code> takes
+    precedence and disallows all ports where it is set; but an inherited
+    <code>None</code> is in turn overridden by an explicit list of ports in a
+    virtual host.</p>
 </usage>
 </directivesynopsis>
 

modules/proxy/mod_proxy_connect.c

@@ -48,6 +48,7 @@ module AP_MODULE_DECLARE_DATA proxy_connect_module;
 
 typedef struct {
     apr_array_header_t *allowed_connect_ports;
+    int none; /* AllowCONNECT None: disallow CONNECT to all ports */
 } connect_conf;
 
 typedef struct {
@@ -68,9 +69,31 @@ static void *merge_config(apr_pool_t *p, void *basev, void *overridesv)
     connect_conf *base = (connect_conf *) basev;
     connect_conf *overrides = (connect_conf *) overridesv;
 
-    c->allowed_connect_ports = apr_array_append(p,
+    if (overrides->none) {
+        /* "AllowCONNECT None" here disallows all ports regardless of base. */
+        c->allowed_connect_ports =
+            apr_array_copy(p, overrides->allowed_connect_ports);
+        c->none = 1;
+    }
+    else if (apr_is_empty_array(overrides->allowed_connect_ports)) {
+        /* Nothing set here: inherit the base. */
+        c->allowed_connect_ports =
+            apr_array_copy(p, base->allowed_connect_ports);
+        c->none = base->none;
+    }
+    else if (base->none || apr_is_empty_array(base->allowed_connect_ports)) {
+        /* Base was "None" or unset, so the ports set here stand alone; e.g.
+         * "AllowCONNECT 443" in a vhost re-allows that port under a global
+         * "AllowCONNECT None". */
+        c->allowed_connect_ports =
+            apr_array_copy(p, overrides->allowed_connect_ports);
+    }
+    else {
+        /* Two port lists merge as a union, as they always have. */
+        c->allowed_connect_ports = apr_array_append(p,
                                                 base->allowed_connect_ports,
                                                 overrides->allowed_connect_ports);
+    }
 
     return c;
 }
@@ -90,8 +113,21 @@ static const char *
     char *endptr;
     const char *p = arg;
 
+    /* "None" disallows CONNECT to all ports, including the defaults, and
+     * cannot be combined with a port list in the same context. */
+    if (!ap_cstr_casecmp(arg, "None")) {
+        if (!apr_is_empty_array(conf->allowed_connect_ports)) {
+            return "AllowCONNECT: \"None\" cannot be combined with port numbers";
+        }
+        conf->none = 1;
+        return NULL;
+    }
+    if (conf->none) {
+        return "AllowCONNECT: \"None\" cannot be combined with port numbers";
+    }
+
     if (!apr_isdigit(arg[0]))
-        return "AllowCONNECT: port numbers must be numeric";
+        return "AllowCONNECT: port numbers must be numeric or \"None\"";
 
     first = strtol(p, &endptr, 10);
     if (*endptr == '-') {
@@ -119,6 +155,10 @@ static int allowed_port(connect_conf *conf, int port)
     int i;
     port_range *list = (port_range *) conf->allowed_connect_ports->elts;
 
+    if (conf->none) {
+        return 0;
+    }
+
     if (apr_is_empty_array(conf->allowed_connect_ports)) {
         return port == APR_URI_HTTPS_DEFAULT_PORT
                || port == APR_URI_SNEWS_DEFAULT_PORT;
@@ -389,7 +429,8 @@ static void ap_proxy_connect_register_hook(apr_pool_t *p)
 static const command_rec cmds[] =
 {
     AP_INIT_ITERATE("AllowCONNECT", set_allowed_ports, NULL, RSRC_CONF,
-     "A list of ports or port ranges which CONNECT may connect to"),
+     "A list of ports or port ranges which CONNECT may connect to, or "
+     "\"None\" to disallow all"),
     {NULL}
 };
 

test/modules/proxy/env.py

@@ -69,7 +69,8 @@ def __init__(self, env: 'HttpdTestEnv'):
         super().__init__(env=env)
         self.add_source_dir(os.path.dirname(inspect.getfile(ProxyTestSetup)))
         self.add_modules(["proxy", "proxy_http", "proxy_ajp", "proxy_balancer",
-                          "proxy_uwsgi", "lbmethod_byrequests", "remoteip"])
+                          "proxy_connect", "proxy_uwsgi", "lbmethod_byrequests",
+                          "remoteip"])
 
 
 class ProxyTestEnv(HttpdTestEnv):

test/modules/proxy/test_07_connect.py

@@ -0,0 +1,108 @@
+import socket
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+class TestProxyConnect:
+    """AllowCONNECT, including the documented "None" value which disallows
+    CONNECT to all ports (including the 443/563 defaults)."""
+
+    def _mk_proxy(self, env, allow_connect):
+        # A forward proxy on its own (plain HTTP) port, so a raw CONNECT can
+        # be sent to it and the vhost handling it is unambiguous.
+        conf = HttpdConf(env)
+        conf.add(f"Listen {env.proxy_port}")
+        conf.start_vhost(domains=[env.d_forward], port=env.proxy_port)
+        conf.add([
+            "ProxyRequests on",
+            allow_connect,
+        ])
+        conf.end_vhost()
+        conf.install()
+        assert env.apache_restart() == 0
+
+    def _connect_status(self, env, target):
+        # Send a raw CONNECT request to the forward proxy and return the
+        # numeric status of its response.
+        req = f"CONNECT {target} HTTP/1.0\r\nHost: {target}\r\n\r\n"
+        with socket.create_connection(("127.0.0.1", env.proxy_port),
+                                      timeout=5) as s:
+            s.sendall(req.encode())
+            buf = b""
+            while b"\r\n" not in buf:
+                data = s.recv(1024)
+                if not data:
+                    break
+                buf += data
+        line = buf.split(b"\r\n", 1)[0].decode("latin-1")
+        return int(line.split(" ", 2)[1])
+
+    def test_proxy_connect_07_none(self, env):
+        # AllowCONNECT None rejects every CONNECT with 403, including the
+        # default https port and an otherwise-reachable target.
+        self._mk_proxy(env, "AllowCONNECT None")
+        assert self._connect_status(env, "127.0.0.1:443") == 403
+        assert self._connect_status(env, f"127.0.0.1:{env.http_port}") == 403
+        # the two rejections each log "Connect to remote machine blocked"
+        env.httpd_error_log.ignore_recent(lognos=["AH00898"])
+
+    def test_proxy_connect_07_allowed(self, env):
+        # Control: with the port explicitly allowed, the same CONNECT tunnels
+        # (200 Connection Established) - so it is None, above, which blocks it.
+        self._mk_proxy(env, f"AllowCONNECT {env.http_port}")
+        assert self._connect_status(env, f"127.0.0.1:{env.http_port}") == 200
+
+    def test_proxy_connect_07_inherit_override(self, env):
+        # "AllowCONNECT None" at the main-server level is overridden by an
+        # explicit port list in the vhost: that port is allowed there (200),
+        # while a port not listed remains blocked (403).
+        conf = HttpdConf(env)
+        conf.add("AllowCONNECT None")
+        conf.add(f"Listen {env.proxy_port}")
+        conf.start_vhost(domains=[env.d_forward], port=env.proxy_port)
+        conf.add([
+            "ProxyRequests on",
+            f"AllowCONNECT {env.http_port}",
+        ])
+        conf.end_vhost()
+        conf.install()
+        assert env.apache_restart() == 0
+        assert self._connect_status(env, f"127.0.0.1:{env.http_port}") == 200
+        assert self._connect_status(env, "127.0.0.1:443") == 403
+        env.httpd_error_log.ignore_recent(lognos=["AH00898"])
+
+    def test_proxy_connect_07_merge_union(self, env):
+        # Two port lists (main-server and vhost) still merge as a union, so a
+        # port allowed at the main-server level stays allowed in the vhost.
+        conf = HttpdConf(env)
+        conf.add(f"AllowCONNECT {env.http_port}")
+        conf.add(f"Listen {env.proxy_port}")
+        conf.start_vhost(domains=[env.d_forward], port=env.proxy_port)
+        conf.add([
+            "ProxyRequests on",
+            "AllowCONNECT 563",
+        ])
+        conf.end_vhost()
+        conf.install()
+        assert env.apache_restart() == 0
+        assert self._connect_status(env, f"127.0.0.1:{env.http_port}") == 200
+        assert self._connect_status(env, "127.0.0.1:9") == 403
+        env.httpd_error_log.ignore_recent(lognos=["AH00898"])
+
+    def test_proxy_connect_07_conflict(self, env):
+        # "None" and a port number in the same context is a config error.
+        conf = HttpdConf(env)
+        conf.add(f"Listen {env.proxy_port}")
+        conf.start_vhost(domains=[env.d_forward], port=env.proxy_port)
+        conf.add([
+            "ProxyRequests on",
+            "AllowCONNECT None",
+            "AllowCONNECT 443",
+        ])
+        conf.end_vhost()
+        conf.install()
+        assert env.apache_restart() != 0
+        # leave a working server behind for teardown / any later test
+        self._mk_proxy(env, "AllowCONNECT None")