[devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c
Arseny Maslennikov
ar на cs.msu.ru
Чт Сен 17 16:11:40 MSK 2020
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
----------- следующая часть -----------
Было удалено вложение не в текстовом формате...
Имя : signature.asc
Тип : application/pgp-signature
Размер : 833 байтов
Описание: отсутствует
Url : <http://lists.altlinux.org/pipermail/devel/attachments/20200917/5556daa8/attachment.bin>
Подробная информация о списке рассылки Devel