From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,RP_MATCHES_RCVD autolearn=ham autolearn_force=no version=3.4.1 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=cs.msu.ru; s=dkim; h=Subject:In-Reply-To:Content-Type:MIME-Version:References:Message-ID :Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=aeQQQ+TxgaHmkIOWbQaMAiowd54Zyw1MFJfhasmGEfM=; b=h7LfdHrOfvjmakjYUzP8gGUp8Y 6DCjIzvxYluEpICswuXBr98sJaTdaW7agVfAdoM9wcVBTbJ/PxW6owyzVN5SdYi2UaxD4meu4Grof fDjRzxH9b8P87jtDpJK3CAaiQWkxY4HFu+cpgos5YSb2BWvMCmLrVz1w5yYAXayP32Oq5M4J42Rsm Sq45Gprc2sYr5K/sFOoivfG5aLLstMhEVHWD4zxG6NRn310fYZdqXThtwEabUyAppl8CEQ3xTVH21 W6Kad/VnVQfs9pGywjuasjiG2A1+lyglLsx8DvfMyf908xcV3nc4NTrLZSaEP3NZ3pRXDOMS4AcE+ S1DPCUqA==; Date: Thu, 17 Sep 2020 16:11:40 +0300 From: Arseny Maslennikov To: Alex Gladkov , devel@lists.altlinux.org Message-ID: <20200917131140.GG286846@cello> References: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="4eRLI4hEmsdu6Npr" Content-Disposition: inline In-Reply-To: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> OpenPGP: url=http://grep.cs.msu.ru/~ar/pgp-key.asc X-SA-Exim-Connect-IP: 10.7.5.179 X-SA-Exim-Mail-From: ar@cs.msu.ru X-SA-Exim-Version: 4.2.1 X-SA-Exim-Scanned: Yes (on mail.cs.msu.ru) Cc: ldv@altlinux.org Subject: Re: [devel] [PATCH hasher-priv v1 1/3] caller_server.c, caller_task.c X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Sep 2020 13:11:45 -0000 Archived-At: List-Archive: List-Post: --4eRLI4hEmsdu6Npr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > + > + The part of the hasher-privd program. > + > + SPDX-License-Identifier: GPL-2.0-or-later > +*/ > + > +#include > + > +#include /* SOCK_CLOEXEC */ > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#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 > + > + The part of the hasher-privd program. > + > + SPDX-License-Identifier: GPL-2.0-or-later > +*/ > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#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 --4eRLI4hEmsdu6Npr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE56JD3UKTLEu/ddrm9dQjyAYL01AFAl9jYIsACgkQ9dQjyAYL 01CBng//cu7TByGzl/Zd0finDx9oZRFDoUA+nfrpsHIBvcl31FyIzk5h0Cc2eH+i B6pCUlO6IRhPCj8l7KwlY+Ug5MCzx2CzI7TxIGDtCrPvpuFAxCTEGh3ukAQdsk8X /2PDKCHmcDw03iBSqbovKrBm+bL5mA8y+NYIwa0rp2yMvGZ5hduQZBsILms46i9R dQ9cBuZTRyTRiLKWQqMh8ZNqSp1X1d2PlFu+xDaFgsjJ7VlE+LG5CiVRuqo2DNNM 2wXC+vSIZEI4BmGo1yuSfEgthVMPO8dMww8hQTUZQdiVJK72f7lxqAZyxjBWkGVF Rk8Ps5LiiICF9j5YR6S2ndIjoqfKclCYA4JHAjsbPv/S5cpot4YBR+u4k5c8Sefn 3AzjDetAkBYbbFb2M3WXDE9zUbVnYatVRSAOuX4yQrwWzCquz1Og2xk7CSUd1g/a BhDtcFhb+1CoY4H4CX3Ka/HD0GPeDsPn7XAJAi4C3ClnfGANThwIlYKod+UX7RaJ df43vGSJLFo7N3756gHg+VIIswuUe9COou8bNBPxuLEMgxGd7Qwj68I4xgkvbEDB hfDxHk2M4QGyFw2b2WeKRYQya3RJT0lfh3UzQPxwMv0Fe/a8cmIfkjt+wxPWbfZx wTZ+ce1aARXq17SAIaGH+p7DJwV5vzwltwpXmOmGgQtXas9KUjk= =Cda5 -----END PGP SIGNATURE----- --4eRLI4hEmsdu6Npr--