[devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c
Alexey Gladkov
legion на altlinux.ru
Чт Окт 1 22:47:49 MSK 2020
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
----------- следующая часть -----------
Было удалено вложение не в текстовом формате...
Имя : signature.asc
Тип : application/pgp-signature
Размер : 195 байтов
Описание: отсутствует
Url : <http://lists.altlinux.org/pipermail/devel/attachments/20201001/5088b90a/attachment-0001.bin>
Подробная информация о списке рассылки Devel