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

[Alexey Gladkov <legion@altlinux.ru>]

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

On Thu, Sep 17, 2020 at 04:11:40PM +0300, Arseny Maslennikov wrote:
> 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.

Yep. This is a typo.

> > +		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



-- 
Rgrds, legion


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