MEDIUM: init: prevent process and thread creation at runtime

Some concerns are regularly raised about the risk to inherit some Lua
files which make use of a fork (e.g. via os.execute()) as well as
whether or not some of bugs we fix might or not be exploitable to run
some code. Given that haproxy is event-driven, any foreground activity
completely stops processing and is easy to detect, but background
activity is a different story. A Lua script could very well discretely
fork a sub-process connecting to a remote location and taking commands,
and some injected code could also try to hide its activity by creating
a process or a thread without blocking the rest of the processing. While
such activities should be extremely limited when run in an empty chroot
without any permission, it would be better to get a higher assurance
they cannot happen.

This patch introduces something very simple: it limits the number of
processes and threads to zero in the workers after the last thread was
created. By doing so, it effectively instructs the system to fail on
any fork() or clone() syscall. Thus any undesired activity has to happen
in the foreground and is way easier to detect.

This will obviously break external checks (whose concept is already
totally insecure), and for this reason a new option
"insecure-fork-wanted" was added to disable this protection, and it
is suggested in the fork() error report from the checks. It is
obviously recommended not to use it and to reconsider the reasons
leading to it being enabled in the first place.

If for any reason we fail to disable forks, we still start because it
could be imaginable that some operating systems refuse to set this
limit to zero, but in this case we emit a warning, that may or may not
be reported since we're after the fork point. Ideally over the long
term it should be conditionned by strict-limits and cause a hard fail.

diff --git a/doc/configuration.txt b/doc/configuration.txt
index eadf1f344a5581e13d3b89ec6ee5887b3e4d1e27..8938195a2cc5019d75d2c8ef68f2c65f926da34e 100644 (file)
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -594,6 +594,7 @@ The following keywords are supported in the "global" section :
    - hard-stop-after
    - h1-case-adjust
    - h1-case-adjust-file
+   - insecure-fork-wanted
    - log
    - log-tag
    - log-send-hostname
@@ -822,9 +823,10 @@ deviceatlas-properties-cookie <name>
   and set to DAPROPS by default if not set.
 
 external-check
-  Allows the use of an external agent to perform health checks.
-  This is disabled by default as a security precaution.
-  See "option external-check".
+  Allows the use of an external agent to perform health checks. This is
+  disabled by default as a security precaution, and even when enabled, checks
+  may still fail unless "insecure-fork-wanted" is enabled as well.
+  See "option external-check", and "insecure-fork-wanted".
 
 gid <number>
   Changes the process' group ID to <number>. It is recommended that the group
@@ -903,6 +905,24 @@ h1-case-adjust-file <hdrs-file>
   See "h1-case-adjust", "option h1-case-adjust-bogus-client" and
   "option h1-case-adjust-bogus-server".
 
+insecure-fork-wanted
+  By default haproxy tries hard to prevent any thread and process creation
+  after it starts. Doing so is particularly important when using Lua files of
+  uncertain origin, and when experimenting with development versions which may
+  still contain bugs whose exploitability is uncertain. And generally speaking
+  it's good hygiene to make sure that no unexpected background activity can be
+  triggered by traffic. But this prevents external checks from working, and may
+  break some very specific Lua scripts which actively rely on the ability to
+  fork. This option is there to disable this protection. Note that it is a bad
+  idea to disable it, as a vulnerability in a library or within haproxy itself
+  will be easier to exploit once disabled. In addition, forking from Lua or
+  anywhere else is not reliable as the forked process may randomly embed a lock
+  set by another thread and never manage to finish an operation. As such it is
+  highly recommended that this option is never used and that any workload
+  requiring such a fork be reconsidered and moved to a safer solution (such as
+  agents instead of external checks). This option supports the "no" prefix to
+  disable it.
+
 log <address> [len <length>] [format <format>] [sample <ranges>:<smp_size>]
     <facility> [max level [min level]]
   Adds a global syslog server. Several global servers can be defined. They

diff --git a/include/types/global.h b/include/types/global.h
index d5dbc7f90dc5b7aa249b5bb2a9e3b60eb34a8628..8c4a7c55b44c49f43282babb259fd52d26328c3f 100644 (file)
--- a/include/types/global.h
+++ b/include/types/global.h
@@ -74,6 +74,7 @@
 #define GTUNE_SET_DUMPABLE       (1<<13)
 #define GTUNE_USE_EVPORTS        (1<<14)
 #define GTUNE_STRICT_LIMITS      (1<<15)
+#define GTUNE_INSECURE_FORK      (1<<16)
 
 /* SSL server verify mode */
 enum {

diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c
index dd37559a53a5a5db789f31d70447d5f495b57bcb..c083a05d5974b2938d057308f46d58ea3271280b 100644 (file)
--- a/src/cfgparse-global.c
+++ b/src/cfgparse-global.c
@@ -94,6 +94,14 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
                else
                        global.tune.options |=  GTUNE_SET_DUMPABLE;
        }
+       else if (!strcmp(args[0], "insecure-fork-wanted")) { /* "no insecure-fork-wanted" or "insecure-fork-wanted" */
+               if (alertif_too_many_args(0, file, linenum, args, &err_code))
+                       goto out;
+               if (kwm == KWM_NO)
+                       global.tune.options &= ~GTUNE_INSECURE_FORK;
+               else
+                       global.tune.options |=  GTUNE_INSECURE_FORK;
+       }
        else if (!strcmp(args[0], "nosplice")) {
                if (alertif_too_many_args(0, file, linenum, args, &err_code))
                        goto out;

diff --git a/src/cfgparse.c b/src/cfgparse.c
index 2e200e885244bb5750af7a095b927501a69b5524..fdc19f4171271b5c4f526f456756a6d95cfd32e6 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -2160,10 +2160,11 @@ next_line:
 
                if (kwm != KWM_STD && strcmp(args[0], "option") != 0 &&
                    strcmp(args[0], "log") != 0 && strcmp(args[0], "busy-polling") != 0 &&
-                   strcmp(args[0], "set-dumpable") != 0 && strcmp(args[0], "strict-limits") != 0) {
+                   strcmp(args[0], "set-dumpable") != 0 && strcmp(args[0], "strict-limits") != 0 &&
+                   strcmp(args[0], "insecure-fork-wanted") != 0) {
                        ha_alert("parsing [%s:%d]: negation/default currently "
                                 "supported only for options, log, busy-polling, "
-                                "set-dumpable and strict-limits.\n", file, linenum);
+                                "set-dumpable, strict-limits, and insecure-fork-wanted.\n", file, linenum);
                        err_code |= ERR_ALERT | ERR_FATAL;
                }
 
@@ -2549,6 +2550,11 @@ int check_config_validity()
                                         curproxy->id, "option external-check");
                                cfgerr++;
                        }
+                       if (!(global.tune.options & GTUNE_INSECURE_FORK)) {
+                               ha_warning("Proxy '%s' : 'insecure-fork-wanted' not enabled in the global section, '%s' will likely fail.\n",
+                                        curproxy->id, "option external-check");
+                               err_code |= ERR_WARN;
+                       }
                }
 
                if (curproxy->email_alert.set) {

diff --git a/src/checks.c b/src/checks.c
index 247caf12e2a34a98b0754b6777aba50d2cce9f55..909bd52f22fb89a9bf342e2113aee809c4728224 100644 (file)
--- a/src/checks.c
+++ b/src/checks.c
@@ -2012,7 +2012,9 @@ static int connect_proc_chk(struct task *t)
 
        pid = fork();
        if (pid < 0) {
-               ha_alert("Failed to fork process for external health check: %s. Aborting.\n",
+               ha_alert("Failed to fork process for external health check%s: %s. Aborting.\n",
+                        (global.tune.options & GTUNE_INSECURE_FORK) ?
+                        "" : " (likely caused by missing 'insecure-fork-wanted')",
                         strerror(errno));
                set_server_check_status(check, HCHK_STATUS_SOCKERR, strerror(errno));
                goto out;

diff --git a/src/haproxy.c b/src/haproxy.c
index b03d0ad5e43c41b796c011973a4b60ef56d54b54..7ba3ae1ba457dc073fcee99e58efa40bbc823bff 100644 (file)
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -2749,6 +2749,23 @@ static void *run_thread_poll_loop(void *data)
        pthread_mutex_unlock(&init_mutex);
 #endif
 
+#if defined(RLIMIT_NPROC)
+       /* all threads have started, it's now time to prevent any new thread
+        * or process from starting. Obviously we do this in workers only. We
+        * can't hard-fail on this one as it really is implementation dependent
+        * though we're interested in feedback, hence the warning.
+        */
+       if (!(global.tune.options & GTUNE_INSECURE_FORK) && !master) {
+               struct rlimit limit = { .rlim_cur = 0, .rlim_max = 0 };
+               static int warn_fail;
+
+               if (setrlimit(RLIMIT_NPROC, &limit) == -1 && !_HA_ATOMIC_XADD(&warn_fail, 1)) {
+                       ha_warning("Failed to disable forks, please report to developers with detailed "
+                                  "information about your operating system. You can silence this warning "
+                                  "by adding 'insecure-fork-wanted' in the 'global' section.\n");
+               }
+       }
+#endif
        run_poll_loop();
 
        list_for_each_entry(ptdf, &per_thread_deinit_list, list)