Re: [devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c

[Arseny Maslennikov <ar@cs.msu.ru>]

[-- Attachment #1: Type: text/plain, Size: 14672 bytes --]

On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote:
> diff --git a/hasher-priv/caller_server.c b/hasher-priv/caller_server.c
> new file mode 100644
> index 0000000..8182c69
> --- /dev/null
> +++ b/hasher-priv/caller_server.c
> @@ -0,0 +1,373 @@
> +
> +/*
> +  Copyright (C) 2019  Alexey Gladkov <legion@altlinux.org>
> +
> +  The part of the hasher-privd program.
> +
> +  SPDX-License-Identifier: GPL-2.0-or-later
> +*/
> +
> +#include <linux/un.h>
> +
> +#include <sys/socket.h> /* SOCK_CLOEXEC */
> +#include <sys/prctl.h>
> +#include <sys/signalfd.h>
> +#include <sys/wait.h>
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <grp.h>
> +
> +#include "priv.h"
> +#include "xmalloc.h"
> +#include "sockets.h"
> +#include "logging.h"
> +#include "epoll.h"
> +#include "communication.h"
> +
> +static char socketpath[UNIX_PATH_MAX];
> +
> +static int
> +validate_arguments(task_t task, char **argv)
> +{
> +	int required_args = 0, more_args = 0;
> +	int rc = -1;
> +	int argc = 0;
> +
> +	while (argv && argv[argc])
> +		argc++;
> +
> +	switch (task) {
> +		case TASK_GETCONF:
> +		case TASK_KILLUID:
> +		case TASK_GETUGID1:
> +		case TASK_GETUGID2:
> +			required_args = 0;
> +			break;
> +		case TASK_CHROOTUID1:
> +		case TASK_CHROOTUID2:
> +			more_args = 1;
> +			required_args = 2;
> +			break;
> +		default:
> +			err("unknown task type: %u", task);
> +			return rc;
> +	}
> +
> +	if (argc < 0)
> +		err("number of arguments must be a positive but got %d",
> +		    argc);
> +
> +	else if (argc < required_args)
> +		err("%s task requires at least %d arguments but got %d",
> +		    task2str(task), required_args, argc);
> +
> +	else if (argc > required_args && !more_args)
> +		err("%s task requires exactly %d arguments but got %d",
> +		    task2str(task), required_args, argc);
> +
> +	else
> +		rc = 0;
> +
> +	return rc;
> +}
> +
> +static void
> +free_task(struct task *task)
> +{
> +	if (task->env) {
> +		free(task->env[0]);
> +		free(task->env);
> +	}
> +
> +	if (task->argv) {
> +		free(task->argv[0]);
> +		free(task->argv);
> +	}
> +}
> +
> +static int
> +cancel_task(int conn, struct task *task)
> +{
> +	free_task(task);
> +	send_command_response(conn, CMD_STATUS_FAILED, "command failed");
> +	return 0;
> +}
> +
> +static int
> +process_task(int conn)
> +{
> +	uid_t uid;
> +	gid_t gid;
> +
> +	if (get_peercred(conn, NULL, &uid, &gid) < 0)
> +		return -1;
> +
> +	if (caller_uid != uid || caller_gid != gid) {
> +		err("user (uid=%d) has no permissions to send commands"
> +		    " to the session of another user (uid=%d)",
> +		    uid, caller_uid);
> +		return -1;
> +	}
> +
> +	struct task task = { 0 };
> +	int fds[3];
> +
> +	while (1) {
> +		task_t type = TASK_NONE;
> +		struct cmd hdr = { 0 };
> +
> +		if (xrecvmsg(conn, &hdr, sizeof(hdr)) < 0)
> +			return cancel_task(conn, &task);
> +
> +		switch (hdr.type) {
> +			case CMD_TASK_BEGIN:
> +				if (hdr.datalen != sizeof(type))
> +					return cancel_task(conn, &task);
> +
> +				if (xrecvmsg(conn, &type, hdr.datalen) < 0)
> +					return cancel_task(conn, &task);
> +
> +				task.type = type;
> +
> +				break;
> +
> +			case CMD_TASK_FDS:
> +				if (hdr.datalen != sizeof(int) * 3)
> +					return cancel_task(conn, &task);
> +
> +				if (task.stdin)
> +					close(task.stdin);
> +
> +				if (task.stdout)
> +					close(task.stdout);
> +
> +				if (task.stderr)
> +					close(task.stderr);
> +
> +				if (fds_recv(conn, fds, 3) < 0)
> +					return cancel_task(conn, &task);
> +
> +				task.stdin  = fds[0];
> +				task.stdout = fds[1];
> +				task.stderr = fds[2];
> +
> +				break;
> +
> +			case CMD_TASK_ARGUMENTS:
> +				if (task.argv) {
> +					free(task.argv[0]);
> +					free(task.argv);
> +				}
> +
> +				if (recv_list(conn, &task.argv, hdr.datalen) < 0)
> +					return cancel_task(conn, &task);
> +
> +				if (validate_arguments(task.type, task.argv) < 0)
> +					return cancel_task(conn, &task);
> +
> +				break;
> +
> +			case CMD_TASK_ENVIRON:
> +				if (task.env) {
> +					free(task.env[0]);
> +					free(task.env);
> +				}
> +
> +				if (recv_list(conn, &task.env, hdr.datalen) < 0)
> +					return cancel_task(conn, &task);
> +
> +				break;
> +
> +			case CMD_TASK_RUN:
> +				process_caller_task(conn, &task);
> +				free_task(&task);
> +				return 0;
> +
> +			default:
> +				err("unsupported command: %d", hdr.type);
> +		}
> +
> +		send_command_response(conn, CMD_STATUS_DONE, NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +caller_server(int cl_conn)
> +{
> +	int ret = 0;
> +
> +	umask(077);
> +
> +	snprintf(socketpath, sizeof(socketpath),
> +		"hasher-priv-%d-%u",
> +		caller_uid, caller_num);
> +
> +	int fd_conn   = -1;
> +	int fd_signal = -1;
> +	int fd_ep     = -1;
> +
> +	if ((fd_conn = srv_listen(SOCKETDIR, socketpath)) < 0)
> +		return -1;
> +
> +	snprintf(socketpath, sizeof(socketpath),
> +		"%s/hasher-priv-%d-%u",
> +		SOCKETDIR, caller_uid, caller_num);
> +
> +	if (chown(socketpath, caller_uid, caller_gid)) {
> +		err("fchown: %s: %m", socketpath);
> +		ret = -1;
> +		goto fail;
> +	}
> +
> +	/* Load config according to caller information. */
> +	configure();
> +
> +	sigset_t mask;
> +
> +	sigfillset(&mask);
> +	sigprocmask(SIG_SETMASK, &mask, NULL);
> +
> +	if ((fd_signal = signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC)) < 0) {
> +		err("signalfd: %m");
> +		ret = -1;
> +		goto fail;
> +	}
> +
> +	if ((fd_ep = epoll_create1(EPOLL_CLOEXEC)) < 0) {
> +		err("epoll_create1: %m");
> +		ret = -1;
> +		goto fail;
> +	}
> +
> +	if (epollin_add(fd_ep, fd_signal) < 0 || epollin_add(fd_ep, fd_conn) < 0) {
> +		err("epollin_add: failed");
> +		ret = -1;
> +		goto fail;
> +	}
> +
> +	/* Tell client that caller server is ready */
> +	send_command_response(cl_conn, CMD_STATUS_DONE, NULL);
> +	close(cl_conn);
> +
> +	unsigned long nsec = 0;
> +	int finish_server = 0;
> +
> +	while (!finish_server) {
> +		struct epoll_event ev[42];
> +		int fdcount;
> +
> +		errno = 0;
> +		if ((fdcount = epoll_wait(fd_ep, ev, ARRAY_SIZE(ev), 1000)) < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			err("epoll_wait: %m");
> +			break;
> +		}
> +
> +		if (fdcount == 0) {
> +			nsec++;
> +
> +			if (nsec >= server_session_timeout)
> +				break;
> +
> +		} else for (int i = 0; i < fdcount; i++) {
> +			if (!(ev[i].events & EPOLLIN)) {
> +				continue;
> +
> +			} else if (ev[i].data.fd == fd_signal) {
> +				struct signalfd_siginfo fdsi;
> +				ssize_t size;
> +
> +				size = read_retry(fd_signal, &fdsi, sizeof(fdsi));
> +
> +				if (size != sizeof(fdsi)) {
> +					err("unable to read signal info");
> +					continue;
> +				}
> +
> +				int status;
> +
> +				switch (fdsi.ssi_signo) {
> +					case SIGINT:
> +					case SIGTERM:
> +						finish_server = 1;
> +						break;
> +					case SIGCHLD:
> +						if (waitpid(-1, &status, 0) < 0)
> +							err("waitpid: %m");
> +						break;
> +				}
> +
> +			} else if (ev[i].data.fd == fd_conn) {
> +				int conn;
> +
> +				if ((conn = accept4(fd_conn, NULL, 0, SOCK_CLOEXEC)) < 0) {
> +					err("accept4: %m");
> +					continue;
> +				}
> +
> +				if (set_recv_timeout(conn, 3) < 0) {
> +					close(conn);
> +					continue;
> +				}
> +
> +				if (!process_task(conn)) {
> +					/* reset timer */
> +					nsec = 0;
> +				}
> +
> +				close(conn);
> +			}
> +		}
> +	}
> +fail:
> +	epollin_remove(fd_ep, fd_signal);
> +	epollin_remove(fd_ep, fd_conn);
> +
> +	if (fd_ep >= 0)
> +		close(fd_ep);
> +
> +	unlink(socketpath);
> +
> +	return ret;
> +}
> +
> +pid_t
> +fork_server(int cl_conn, uid_t uid, gid_t gid, unsigned num)
> +{
> +	pid_t pid = fork();
> +
> +	if (pid != 0) {
> +		if (pid < 0) {
> +			err("fork: %m");
> +			return -1;
> +		}
> +		return pid;
> +	}
> +
> +	caller_num = num;
> +
> +	int ret = init_caller_data(uid, gid);
> +
> +	if (ret < 0) {

This if-block is semantically nonsensical as written.

If this comparison is reversed (ret >= 0), the resulting code looks
semantically correct; it probably was a typo. It still would be better to refactor this
piece, imo.

> +		info("%s(%d) num=%u: start session server", caller_user, caller_uid, caller_num);
> +		ret = caller_server(cl_conn);
> +		info("%s(%d): finish session server", caller_user, caller_uid);
> +	}
> +
> +	if (ret < 0) {
> +		send_command_response(cl_conn, CMD_STATUS_FAILED, NULL);
> +		ret = EXIT_FAILURE;
> +	} else {
> +		ret = EXIT_SUCCESS;
> +	}
> +
> +	free_caller_data();
> +	close(cl_conn);
> +
> +	exit(ret);
> +}
> diff --git a/hasher-priv/caller_task.c b/hasher-priv/caller_task.c
> new file mode 100644
> index 0000000..d8f2dd5
> --- /dev/null
> +++ b/hasher-priv/caller_task.c
> @@ -0,0 +1,214 @@
> +
> +/*
> +  Copyright (C) 2019  Alexey Gladkov <legion@altlinux.org>
> +
> +  The part of the hasher-privd program.
> +
> +  SPDX-License-Identifier: GPL-2.0-or-later
> +*/
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <sys/param.h>
> +#include <sys/wait.h>
> +
> +#include <unistd.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <stdint.h>
> +
> +#include "communication.h"
> +#include "xmalloc.h"
> +#include "logging.h"
> +#include "sockets.h"
> +#include "priv.h"
> +
> +static int
> +killprevious(void)
> +{
> +	int rc = 0;
> +	pid_t pid = fork();
> +
> +	if (pid < 0) {
> +		err("fork: %m");
> +		return -1;
> +	}
> +
> +	if (!pid)
> +		exit(do_killuid());
> +
> +	while (1) {
> +		int wstatus;
> +
> +		if (waitpid(pid, &wstatus, WUNTRACED | WCONTINUED) < 0) {
> +			err("waitpid: %m");
> +			rc = -1;
> +			break;
> +		}
> +
> +		if (WIFEXITED(wstatus)) {
> +			rc = WEXITSTATUS(wstatus);
> +			break;
> +		}
> +	}
> +
> +	return rc;
> +}
> +
> +static int
> +reopen_fd(int oldfd, int newfd)
> +{
> +	if (oldfd < 0)
> +		return 0;
> +
> +	close(newfd);
> +
> +	if (dup2(oldfd, newfd) < 0) {
> +		err("dup2: %m");
> +		return -1;
> +	}
> +
> +	close(oldfd);
> +
> +	return 0;
> +}
> +
> +static int
> +reopen_iostreams(int stdin, int stdout, int stderr)
> +{
> +	return (reopen_fd(stdin, STDIN_FILENO) < 0 ||
> +	    reopen_fd(stdout, STDOUT_FILENO) < 0 ||
> +	    reopen_fd(stderr, STDERR_FILENO) < 0) ? -1 : 0;
> +}
> +
> +static int
> +caller_task(struct task *task)
> +{
> +	int rc = EXIT_FAILURE;
> +	int i = 0;
> +	pid_t pid;
> +
> +	if ((pid = fork()) != 0) {
> +		if (pid < 0) {
> +			err("fork: %m");
> +			return -1;
> +		}
> +		return pid;
> +	}

There are multiple snippets like this one in the patch.

This would look better, wouldn't it:

+	if ((pid = fork()) < 0) {
+		err("fork: %m");
+		return -1;
+	}
+
+	if (pid > 0) {
+		return pid;
+	}
+

> +
> +	if ((rc = reopen_iostreams(task->stdin, task->stdout, task->stderr)) < 0)
> +		exit(rc);
> +
> +	/* cleanup environment to avoid side effects. */
> +	if (clearenv() != 0)
> +		fatal("clearenv: %m");
> +
> +	while (task->env && task->env[i]) {
> +		if (putenv(task->env[i++]) != 0)
> +			fatal("putenv: %m");
> +	}
> +
> +	/* First, check and sanitize file descriptors. */
> +	sanitize_fds();
> +
> +	/* Second, parse task arguments. */
> +	parse_task_args(task->type, (const char **) task->argv);
> +
> +	if (chroot_path && *chroot_path != '/')
> +		fatal("%s: invalid chroot path", chroot_path);
> +
> +	/* Fourth, parse environment for config options. */
> +	parse_env();
> +
> +	/* We don't need environment variables any longer. */
> +	if (clearenv() != 0)
> +		fatal("clearenv: %m");
> +
> +	/* Finally, execute choosen task. */
> +	switch (task->type) {
> +		case TASK_GETCONF:
> +			rc = do_getconf();
> +			break;
> +		case TASK_KILLUID:
> +			rc = do_killuid();
> +			break;
> +		case TASK_GETUGID1:
> +			rc = do_getugid1();
> +			break;
> +		case TASK_CHROOTUID1:
> +			rc = !killprevious()
> +				? do_chrootuid1()
> +				: EXIT_FAILURE;
> +			break;
> +		case TASK_GETUGID2:
> +			rc = do_getugid2();
> +			break;
> +		case TASK_CHROOTUID2:
> +			rc = !killprevious()
> +				? do_chrootuid2()
> +				: EXIT_FAILURE;
> +			break;
> +		default:
> +			fatal("unknown task %d", task->type);
> +	}
> +
> +	/* Write of all user-space buffered data */
> +	fflush(stdout);
> +	fflush(stderr);
> +
> +	exit(rc);
> +}
> +
> +int
> +process_caller_task(int conn, struct task *task)
> +{
> +	int rc = EXIT_FAILURE;
> +	pid_t pid, cpid;
> +
> +	if ((pid = fork()) != 0) {
> +		if (pid < 0) {
> +			err("fork: %m");
> +			return -1;
> +		}

Some resources (up to 3 file descriptors per task, memory for tasks) are
not reclaimed here, in the likely long-living hasherd session process.

Fun fact: if you fix hasher-privd to run successfully until this point,
the following shell command
`echo $(/usr/libexec/hasher-priv/hasher-priv getconf)' ??? a common
pattern in hsh(1) ??? actually hangs until the session process dies. No
wonder: since there still are open writers on the pipe to shell when the
task finishes, there's no end-of-stream.
A more clear reproducer would be:
`/usr/libexec/hasher-priv/hasher-priv getconf | cat'.

> +		return 0;
> +	}
> +
> +	if ((cpid = caller_task(task)) > 0) {

There's quite a bunch of forks involved in fulfilling any possible
request.

I suggest to link the daemon with [1] and call setproctitle in all
non-execcing descendant processes of the daemon.

/usr/sbin/hasher-privd -f
 \_ hasher-privd: privileged helper for ar/500:0
     \_ hasher-privd: process_caller_task
         \_ hasher-privd: [500:0] task handler: chrootuid1
             \_ /bin/sh /usr/bin/fakeroot /.host/entry
                 \_ ...

This looks better than a wall of "/usr/sbin/hasher-privd -f"

[1] http://git.altlinux.org/gears/s/setproctitle.git?p=setproctitle.git;a=summary

> +		while (1) {
> +			pid_t w;
> +			int wstatus;
> +
> +			if ((w = waitpid(cpid, &wstatus, WUNTRACED | WCONTINUED)) < 0) {
> +				err("waitpid: %m");
> +				break;
> +			}
> +
> +			if (WIFEXITED(wstatus)) {
> +				rc = WEXITSTATUS(wstatus);
> +				info("%s: process %d exited, status=%d", task2str(task->type), cpid, WEXITSTATUS(wstatus));
> +				break;
> +			}
> +
> +			if (WIFSIGNALED(wstatus)) {
> +				info("%s: process %d killed by signal %d", task2str(task->type), cpid, WTERMSIG(wstatus));
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (task->env) {
> +		free(task->env[0]);
> +		free(task->env);
> +	}
> +
> +	if (task->argv) {
> +		free(task->argv[0]);
> +		free(task->argv);
> +	}

We have free_task() as a static function on the struct task* in a
neighbour file. Merge the files maybe? And close task->std(in|out|err)
if > 0.

Another decent option is to publish free_task() in caller_task.c

> +
> +	/* Notify client about result */
> +	(rc == EXIT_FAILURE)
> +		? send_command_response(conn, CMD_STATUS_FAILED, "command failed")
> +		: send_command_response(conn, CMD_STATUS_DONE, NULL);
> +
> +	exit(rc);
> +}
// Redundant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]