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=-1.9 required=5.0 tests=BAYES_00 autolearn=ham autolearn_force=no version=3.4.1 Date: Thu, 1 Oct 2020 21:47:49 +0200 From: Alexey Gladkov To: Arseny Maslennikov Message-ID: <20201001194749.nb573gpawu6ggihi@comp-core-i7-2640m-0182e6> References: <9bca7626b593f896de4283cba2d6290ec99eb4f2.1576183643.git.legion@altlinux.org> <20200917131140.GG286846@cello> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="xpu3ltj7kddbj23h" Content-Disposition: inline In-Reply-To: <20200917131140.GG286846@cello> Cc: devel@lists.altlinux.org, 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, 01 Oct 2020 19:47:58 -0000 Archived-At: List-Archive: List-Post: --xpu3ltj7kddbj23h Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > > + > > + 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 =3D 0, more_args =3D 0; > > + int rc =3D -1; > > + int argc =3D 0; > > + > > + while (argv && argv[argc]) > > + argc++; > > + > > + switch (task) { > > + case TASK_GETCONF: > > + case TASK_KILLUID: > > + case TASK_GETUGID1: > > + case TASK_GETUGID2: > > + required_args =3D 0; > > + break; > > + case TASK_CHROOTUID1: > > + case TASK_CHROOTUID2: > > + more_args =3D 1; > > + required_args =3D 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 =3D 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 !=3D uid || caller_gid !=3D gid) { > > + err("user (uid=3D%d) has no permissions to send commands" > > + " to the session of another user (uid=3D%d)", > > + uid, caller_uid); > > + return -1; > > + } > > + > > + struct task task =3D { 0 }; > > + int fds[3]; > > + > > + while (1) { > > + task_t type =3D TASK_NONE; > > + struct cmd hdr =3D { 0 }; > > + > > + if (xrecvmsg(conn, &hdr, sizeof(hdr)) < 0) > > + return cancel_task(conn, &task); > > + > > + switch (hdr.type) { > > + case CMD_TASK_BEGIN: > > + if (hdr.datalen !=3D sizeof(type)) > > + return cancel_task(conn, &task); > > + > > + if (xrecvmsg(conn, &type, hdr.datalen) < 0) > > + return cancel_task(conn, &task); > > + > > + task.type =3D type; > > + > > + break; > > + > > + case CMD_TASK_FDS: > > + if (hdr.datalen !=3D 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 =3D fds[0]; > > + task.stdout =3D fds[1]; > > + task.stderr =3D 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 =3D 0; > > + > > + umask(077); > > + > > + snprintf(socketpath, sizeof(socketpath), > > + "hasher-priv-%d-%u", > > + caller_uid, caller_num); > > + > > + int fd_conn =3D -1; > > + int fd_signal =3D -1; > > + int fd_ep =3D -1; > > + > > + if ((fd_conn =3D 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 =3D -1; > > + goto fail; > > + } > > + > > + /* Load config according to caller information. */ > > + configure(); > > + > > + sigset_t mask; > > + > > + sigfillset(&mask); > > + sigprocmask(SIG_SETMASK, &mask, NULL); > > + > > + if ((fd_signal =3D signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC)) <= 0) { > > + err("signalfd: %m"); > > + ret =3D -1; > > + goto fail; > > + } > > + > > + if ((fd_ep =3D epoll_create1(EPOLL_CLOEXEC)) < 0) { > > + err("epoll_create1: %m"); > > + ret =3D -1; > > + goto fail; > > + } > > + > > + if (epollin_add(fd_ep, fd_signal) < 0 || epollin_add(fd_ep, fd_conn) = < 0) { > > + err("epollin_add: failed"); > > + ret =3D -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 =3D 0; > > + int finish_server =3D 0; > > + > > + while (!finish_server) { > > + struct epoll_event ev[42]; > > + int fdcount; > > + > > + errno =3D 0; > > + if ((fdcount =3D epoll_wait(fd_ep, ev, ARRAY_SIZE(ev), 1000)) < 0) { > > + if (errno =3D=3D EINTR) > > + continue; > > + err("epoll_wait: %m"); > > + break; > > + } > > + > > + if (fdcount =3D=3D 0) { > > + nsec++; > > + > > + if (nsec >=3D server_session_timeout) > > + break; > > + > > + } else for (int i =3D 0; i < fdcount; i++) { > > + if (!(ev[i].events & EPOLLIN)) { > > + continue; > > + > > + } else if (ev[i].data.fd =3D=3D fd_signal) { > > + struct signalfd_siginfo fdsi; > > + ssize_t size; > > + > > + size =3D read_retry(fd_signal, &fdsi, sizeof(fdsi)); > > + > > + if (size !=3D sizeof(fdsi)) { > > + err("unable to read signal info"); > > + continue; > > + } > > + > > + int status; > > + > > + switch (fdsi.ssi_signo) { > > + case SIGINT: > > + case SIGTERM: > > + finish_server =3D 1; > > + break; > > + case SIGCHLD: > > + if (waitpid(-1, &status, 0) < 0) > > + err("waitpid: %m"); > > + break; > > + } > > + > > + } else if (ev[i].data.fd =3D=3D fd_conn) { > > + int conn; > > + > > + if ((conn =3D 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 =3D 0; > > + } > > + > > + close(conn); > > + } > > + } > > + } > > +fail: > > + epollin_remove(fd_ep, fd_signal); > > + epollin_remove(fd_ep, fd_conn); > > + > > + if (fd_ep >=3D 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 =3D fork(); > > + > > + if (pid !=3D 0) { > > + if (pid < 0) { > > + err("fork: %m"); > > + return -1; > > + } > > + return pid; > > + } > > + > > + caller_num =3D num; > > + > > + int ret =3D init_caller_data(uid, gid); > > + > > + if (ret < 0) { >=20 > This if-block is semantically nonsensical as written. >=20 > If this comparison is reversed (ret >=3D 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=3D%u: start session server", caller_user, caller_ui= d, caller_num); > > + ret =3D 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 =3D EXIT_FAILURE; > > + } else { > > + ret =3D 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 =3D 0; > > + pid_t pid =3D 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 =3D -1; > > + break; > > + } > > + > > + if (WIFEXITED(wstatus)) { > > + rc =3D 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 =3D EXIT_FAILURE; > > + int i =3D 0; > > + pid_t pid; > > + > > + if ((pid =3D fork()) !=3D 0) { > > + if (pid < 0) { > > + err("fork: %m"); > > + return -1; > > + } > > + return pid; > > + } >=20 > There are multiple snippets like this one in the patch. >=20 > This would look better, wouldn't it: >=20 > + if ((pid =3D fork()) < 0) { > + err("fork: %m"); > + return -1; > + } > + > + if (pid > 0) { > + return pid; > + } > + >=20 > > + > > + if ((rc =3D reopen_iostreams(task->stdin, task->stdout, task->stderr)= ) < 0) > > + exit(rc); > > + > > + /* cleanup environment to avoid side effects. */ > > + if (clearenv() !=3D 0) > > + fatal("clearenv: %m"); > > + > > + while (task->env && task->env[i]) { > > + if (putenv(task->env[i++]) !=3D 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 !=3D '/') > > + 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() !=3D 0) > > + fatal("clearenv: %m"); > > + > > + /* Finally, execute choosen task. */ > > + switch (task->type) { > > + case TASK_GETCONF: > > + rc =3D do_getconf(); > > + break; > > + case TASK_KILLUID: > > + rc =3D do_killuid(); > > + break; > > + case TASK_GETUGID1: > > + rc =3D do_getugid1(); > > + break; > > + case TASK_CHROOTUID1: > > + rc =3D !killprevious() > > + ? do_chrootuid1() > > + : EXIT_FAILURE; > > + break; > > + case TASK_GETUGID2: > > + rc =3D do_getugid2(); > > + break; > > + case TASK_CHROOTUID2: > > + rc =3D !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 =3D EXIT_FAILURE; > > + pid_t pid, cpid; > > + > > + if ((pid =3D fork()) !=3D 0) { > > + if (pid < 0) { > > + err("fork: %m"); > > + return -1; > > + } >=20 > Some resources (up to 3 file descriptors per task, memory for tasks) are > not reclaimed here, in the likely long-living hasherd session process. >=20 > 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'. >=20 > > + return 0; > > + } > > + > > + if ((cpid =3D caller_task(task)) > 0) { >=20 > There's quite a bunch of forks involved in fulfilling any possible > request. >=20 > I suggest to link the daemon with [1] and call setproctitle in all > non-execcing descendant processes of the daemon. >=20 > /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 > \_ ... >=20 > This looks better than a wall of "/usr/sbin/hasher-privd -f" >=20 > [1] http://git.altlinux.org/gears/s/setproctitle.git?p=3Dsetproctitle.git= ;a=3Dsummary >=20 > > + while (1) { > > + pid_t w; > > + int wstatus; > > + > > + if ((w =3D waitpid(cpid, &wstatus, WUNTRACED | WCONTINUED)) < 0) { > > + err("waitpid: %m"); > > + break; > > + } > > + > > + if (WIFEXITED(wstatus)) { > > + rc =3D WEXITSTATUS(wstatus); > > + info("%s: process %d exited, status=3D%d", task2str(task->type), c= pid, WEXITSTATUS(wstatus)); > > + break; > > + } > > + > > + if (WIFSIGNALED(wstatus)) { > > + info("%s: process %d killed by signal %d", task2str(task->type), c= pid, WTERMSIG(wstatus)); > > + break; > > + } > > + } > > + } > > + > > + if (task->env) { > > + free(task->env[0]); > > + free(task->env); > > + } > > + > > + if (task->argv) { > > + free(task->argv[0]); > > + free(task->argv); > > + } >=20 > 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. >=20 > Another decent option is to publish free_task() in caller_task.c >=20 > > + > > + /* Notify client about result */ > > + (rc =3D=3D EXIT_FAILURE) > > + ? send_command_response(conn, CMD_STATUS_FAILED, "command failed") > > + : send_command_response(conn, CMD_STATUS_DONE, NULL); > > + > > + exit(rc); > > +} > // Redundant --=20 Rgrds, legion --xpu3ltj7kddbj23h Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABECAB0WIQSuzIk+w2aWgaEZLHKOFEXcaOMeVAUCX3YyZQAKCRCOFEXcaOMe VB50AKCKJbrY/L82cUHzTOK1SKBtkqJB1ACgjHBYV01pBljXbvADtf8zsSfWVC8= =+YUg -----END PGP SIGNATURE----- --xpu3ltj7kddbj23h--