[new-sashok724]

The reload_check_config test ran as uid/gid 0 with no supplementary groups. If keepalived runs as non-root, setgid(0) fails and, unless strict_script_exec is set, the test exits with status 0, so the new config was reloaded unchecked. Clearing the supplementary groups could also stop the test reading config files that keepalived itself can read.

Run the test with keepalived's own uid, gid and supplementary groups instead. When keepalived runs as root with gid 0, the only difference is that the test keeps keepalived's supplementary groups.

[new-sashok724]

(I waited until the previous PRs were merged before opening this one, because it depends on #2732 :))

[pqarmitage]

@new-sashok724 I think what we really want here is not to change the uid/gid or supplementary groups at all, since we want to run with the same uid/gids as keepalived is currently running.

I think this can be generalised. If we don't change the uid, then we shouldn't change the supplementary groups, since the supplementary groups are a property of the uid. I think the following does this:

diff --git a/keepalived/core/notify.c b/keepalived/core/notify.c
index 64ecec14..e381517f 100644
--- a/keepalived/core/notify.c
+++ b/keepalived/core/notify.c
@@ -91,15 +91,15 @@ set_script_env(const user_id_t *user_id)
                }
        }

-       /* Set/clear any extra supplementary groups. If we can't, check_script_secure()
-        * has ensured that we have none that the script's user does not. */
-       if (can_set_sup_grp &&
-           setgroups(user_id->num_sup_grp, user_id->sup_grp) < 0) {
-               log_message(LOG_ALERT, "Couldn't setgroups for %u (%m)", user_id->uid);
-               return true;
-       }
-
        if (user_id->uid != our_uid) {
+               /* Set/clear any extra supplementary groups. If we can't, check_script_secure()
+                * has ensured that we have none that the script's user does not. */
+               if (can_set_sup_grp &&
+                   setgroups(user_id->num_sup_grp, user_id->sup_grp) < 0) {
+                       log_message(LOG_ALERT, "Couldn't setgroups for %u (%m)", user_id->uid);
+                       return true;
+               }
+
                if (setuid(user_id->uid) < 0) {
                        log_message(LOG_ALERT, "Couldn't setuid: %u (%m)", user_id->uid);
                        return true;

Do you have any thoughts on this?

Your patch still applies, although it can be simplified by not setting num_sup_grp and sup_grp.

[new-sashok724]

@pqarmitage I agree that the config test shouldn't change credentials. In practice it already doesn't, since the uid and gid are unchanged and setgroups() sets the groups we already have, but you're right that the setgroups() call is unnecessary there.

However, I'm not sure about skipping setgroups() whenever the uid is unchanged, because I don't think that supplementary groups are really a property of the uid: keepalived's own groups depend on how it was started, while a script's groups come from its configuration. With this change, a script running as keepalived's own user (which is also the default if there's no keepalived_script user) would keep keepalived's supplementary groups regardless of the config, effectively ignoring : and set_supplementary_groups. Users could reasonably consider that a bug.

More generally, I wonder whether the config reload check should use system_call_script() at all. It was written for running scripts, and some of its behavior clearly doesn't fit this use. Changing credentials is one example; another is that without strict_script_exec, a child that fails to exec exits with status 0, so the reload goes ahead unchecked, which probably isn't what we want. That's a much bigger change than this fix, though, so it's probably best done separately. In the meantime, I'd suggest keeping this PR as it is, if that works for you.

[pqarmitage]

@new-sashok724 I have taken the opportunity to, as you suggest, add a new function for config checks instead of system_call_script().

The patch is:

diff --git a/doc/man/man5/keepalived.conf.5.in b/doc/man/man5/keepalived.conf.5.in
index 70f6e625..34d3c08e 100644
--- a/doc/man/man5/keepalived.conf.5.in
+++ b/doc/man/man5/keepalived.conf.5.in
@@ -288,7 +288,7 @@ possibly following any cleanup actions needed.
     # Treat a script that cannot be executed (for example if it cannot be
     # found, or its user/group cannot be set), or whose shell is killed by a
     # signal, as exiting with status 1 rather than 0. This affects vrrp_script,
-    # MISC_CHECK and the reload_check_config config test.
+    # and MISC_CHECK.
     \fBstrict_script_exec \fR[<BOOL>]

     # The startup and shutdown scripts are run once, when keepalived starts
diff --git a/keepalived/core/main.c b/keepalived/core/main.c
index fb94abf5..a29c1b3c 100644
--- a/keepalived/core/main.c
+++ b/keepalived/core/main.c
@@ -171,6 +171,7 @@ const char *snmp_socket;                            /* Socket to use for SNMP agent */
 static const char *syslog_ident;                       /* syslog ident if not default */
 bool use_pid_dir;                                      /* Put pid files in /run/keepalived or @localstatedir@/run/keepalived */
 bool children_started;                                 /* Set once children have been run first time */
+static char *executable_full_path;                     /* Full path name of our executable */

 unsigned os_major;                                     /* Kernel version */
 unsigned os_minor;
@@ -316,6 +317,8 @@ free_parent_mallocs_startup(bool am_child)

                free_notify_script(&global_data->startup_script);
                free_notify_script(&global_data->shutdown_script);
+
+               FREE_PTR(executable_full_path);
        }
 }

@@ -976,19 +979,95 @@ reload_check_child_thread(thread_ref_t thread)

        if (WIFEXITED(thread->u.c.status)) {
                if (WEXITSTATUS(thread->u.c.status)) {
-                       log_message(LOG_INFO, "New config failed validation, see %s for details", global_data->reload_check_config);
+                       if (strcmp(global_data->reload_check_config, "/dev/null"))
+                               log_message(LOG_INFO, "New config failed validation, see %s for details", global_data->reload_check_config);
+                       else
+                               log_message(LOG_INFO, "New config failed validation");
                        return;
                }

                do_reload();
-       } else
+       } else {
                report_child_status(thread->u.c.status, thread->u.c.pid, "reload_check");
+               log_message(LOG_INFO, "Unable to start reload due to above problem");
+       }
+}
+
+static int
+call_self(thread_master_t *m, const char * const *argv)
+{
+       pid_t pid;
+       union non_const_args args;
+
+       /* Daemonization to not degrade our scheduling timer */
+#ifdef ENABLE_LOG_TO_FILE
+       if (log_file_name)
+               flush_log_file();
+#endif
+
+       pid = fork();
+
+       if (pid < 0) {
+               /* fork error */
+               log_message(LOG_INFO, "Failed fork process");
+               return -1;
+       }
+
+       if (pid) {
+               /* parent process */
+               thread_add_child(m, reload_check_child_thread, NULL, pid, 5 * TIMER_HZ);
+#ifdef _SCRIPT_DEBUG_
+               if (do_script_debug)
+                       log_message(LOG_INFO, "Running config_check pid %d", pid);
+#endif
+
+               return 0;
+       }
+
+       /* Child process */
+       our_pid = getpid();
+       reset_process_priorities();
+
+#ifdef _MEM_CHECK_
+       skip_mem_dump();
+#endif
+
+#ifdef HAVE_CLOSE_RANGE
+        /* We don't want anything past stderr here. This is belt
+         * and braces really, since all file desriptors should
+         * have FD_CLOEXEC set. CLOSE_RANGE_CLOEXEC uses fewer
+         * kernel resources that CLOSE_RANGE_UNSHARE. */
+       close_range(STDERR_FILENO + 1, ~0U,
+#if HAVE_DECL_CLOSE_RANGE_CLOEXEC
+                           CLOSE_RANGE_CLOEXEC
+#else
+                           CLOSE_RANGE_UNSHARE
+#endif
+                                              );
+#endif
+
+       signal_handler_script();
+       set_std_fd(false);
+
+       /* Defence in depth: drop loader and field-splitting overrides before exec. */
+       unsetenv("IFS");
+       unsetenv("LD_PRELOAD");
+       unsetenv("LD_LIBRARY_PATH");
+
+       /* If keepalived dies, we want the script to die */
+       prctl(PR_SET_PDEATHSIG, SIGTERM);
+
+       args.args = argv;       /* Note: we are casting away constness, since execve parameter type is wrong */
+       execve("/proc/self/exe", args.execve_args, environ);
+
+       /* error */
+       log_message(LOG_ALERT, "Error exec-ing '%s' for config check, error %d: %m", argv[0], errno);
+       exit(EXIT_FAILURE);
 }

 static void
 start_validate_reload_conf_child(void)
 {
-       notify_script_t script = { .path = NULL };
        int i;
        int ret;
        int argc;
@@ -997,32 +1076,16 @@ start_validate_reload_conf_child(void)
        char *config_test_str;
        char *config_fd_str = NULL;
        int fd;
-       int len;
-       char exe_buf[128];
        struct stat sb;

-       exe_buf[sizeof(exe_buf) - 1] = '\0';
-       ret = readlink("/proc/self/exe", exe_buf, sizeof(exe_buf));
-       if (ret == -1) {
-               /* How can this happen? What can we do? */
-               log_message(LOG_INFO, "readlink(\"/proc/self/exe\") failed - errno %d - config-test aborted", errno);
-               return;
-       } else if (ret == sizeof(exe_buf))
-               strcpy(exe_buf, "/proc/self/exe");
-       else {
-               exe_buf[ret] = '\0';
-               len = strlen(exe_buf);
-               /* If keepalived has been recompiled, the original file will
-                * be marked as deleted, but we can use the new one. */
-               if (len > 10 && !strcmp(exe_buf + len - 10, " (deleted)"))
-                       exe_buf[len - 10] = '\0';
-       }
-
        /* Inherits the original parameters and adds new parameters "--config-test and --config-fd" */
        sav_argv = get_cmd_line_options(&argc);
        argv = MALLOC((argc + 3) * sizeof(char *));
 
-       argv[0] = exe_buf;
+       /* If we were executed with a relative path name, and then have done chdir("/"),
+        * the relative path will proably no longer work, so we need to use the absolute
+        * path name. */
+       argv[0] = executable_full_path;
 
        /* copy old parameters */
        for (i = 1; i < argc; i++)
@@ -1046,14 +1109,6 @@ start_validate_reload_conf_child(void)
 
        argv[argc] = NULL;
 
-       script.args = argv;
-       script.num_args = argc;
-       script.flags = SC_EXECABLE;
-       script.user_id.uid = 0;
-       script.user_id.gid = 0;
-       script.user_id.num_sup_grp = 0;
-       script.user_id.sup_grp = NULL;
-
        if (truncate(global_data->reload_check_config, 0) && errno != ENOENT) {
                /* The file exists, but truncate failed. It might be a character
                 * device like /dev/null. truncate() returns EINVAL in this case. */
@@ -1064,9 +1119,8 @@ start_validate_reload_conf_child(void)
 
        create_reload_file();
 
-       /* Execute the script in a child process. Parent returns, child doesn't */
-       ret = system_call_script(master, reload_check_child_thread,
-                                 NULL, 5 * TIMER_HZ, &script);
+       /* Execute in a child process. Parent returns, child doesn't */
+       ret = call_self(master, argv);
 
        if (ret)
                log_message(LOG_INFO, "Could not run config-test");
@@ -2450,6 +2504,7 @@ keepalived_main(int argc, char **argv)
        int exit_code = KEEPALIVED_EXIT_OK;
        magic_t magic;
        unsigned script_flags;
+       char *tmp_full_path;
        struct rusage usage;
        struct rusage child_usage;
 #ifdef OLD_DEFAULT_CONFIG_FILE
@@ -2598,6 +2653,20 @@ keepalived_main(int argc, char **argv)
        get_stacksize(false);
 #endif
 
+       /* Save executable file name for argv[0] for execing ourself
+        * for config checks. If we are a process doing a config check,
+        * it is possible that the executable file may no longer exist,
+        * but we will have been exec'd using /proc/self/exe. */
+       tmp_full_path = realpath(argv[0], NULL);
+       if (tmp_full_path) {
+               executable_full_path = MALLOC(strlen(tmp_full_path) + 1);
+               strcpy(executable_full_path, tmp_full_path);
+               free(tmp_full_path);
+       } else {
+               executable_full_path = MALLOC(strlen(argv[0]) + 1);
+               strcpy(executable_full_path, argv[0]);
+       }
+
 #ifdef _REPRODUCIBLE_BUILD_
        /* We want to use our MALLOC functions */
        char *new_config_opts_str = STRDUP(config_opts);
@@ -2937,6 +3006,8 @@ end:
        free_parent_mallocs_exit();
        free_global_data(&global_data);
 
+       FREE_PTR(executable_full_path);
+
 #ifdef DO_STACKSIZE
        get_stacksize(true);
 #endif
diff --git a/lib/parser.c b/lib/parser.c
index 17a40768..be070b4a 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -1850,8 +1850,10 @@ check_conf_file(const char *conf_file)
        bool ret = true;
        unsigned num_matches = 0;
 
-       if (!open_and_check_glob(&globbuf, conf_file, INCLUDE))
+       if (!open_and_check_glob(&globbuf, conf_file, INCLUDE)) {
+               config_err = CONFIG_FILE_NOT_FOUND;
                return false;
+       }
 
        for (i = 0; i < globbuf.gl_pathc; i++) {
                if (!check_glob_file(globbuf.gl_pathv[i])) {

call_self() is essentially system_call_script() with the unnecessary parts stripped out.

I have also simplified specifying the process to execute by always using /proc/self/exe since we cannot rely on the original path of the executable, as found in argv[0] to still exist, or be the same executable.

check_conf_file() now returns an error if the config file doesn't exist, whereas previously the reload would be attempted, but that would then fail due to no config file.

The argv[0] that is passed to the config check process is now an absolute path name, in case the original path was a relative path. This is needed because keepalived executes chdir("/").

@new-sashok724 Please let me know if this works for you, or you think anything needs changing.

[new-sashok724]

@pqarmitage Thanks, this looks like a much better approach than mine. A few things I noticed:

1. call_self() no longer calls setpgid(0, 0), but handle_child_timeout() uses kill(-pid, ...) (i.e., kills the process group). Since the child isn't the process group leader, that would fail with ESRCH, so a config check that hangs would just be logged as "lost" and not killed.

2. Setting config_err in check_conf_file() covers the glob failure, but if check_glob_file() rejects every match (e.g. the config file is a directory or executable), the error is reported as CONFIG_OK unless include checking is enabled, so the config test still exits with status 0 and the reload goes ahead. It might be simpler to exit with KEEPALIVED_CHK_EXIT_CONFIG in keepalived_main() whenever check_conf_file() fails in config-test mode.

3. If keepalived is started by name via PATH, realpath(argv[0]) resolves relative to the current directory, so it would either fail or find the wrong file. Reading /proc/self/exe for argv[0] too would avoid that.

Nits: STRDUP could replace MALLOC + strcpy, there's a stray comma in the man page change ("vrrp_script, and MISC_CHECK"), and a few typos ("proably", "desriptors").

I've attached a patch (to be applied on top of your patch) that fixes these issues. Since this replaces my change, should I close this PR?