From 330dcf98682a478bd776f339ac2afafdccbdd790 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 14 Nov 2021 11:54:28 +0100 Subject: [PATCH] vipw: improve child error handling Always set SIGCHLD handler to default, even if the caller of vipw has set SIGCHLD to ignore. If SIGCHLD is ignored no zombie processes would be created, which in turn could mean that kill is called with an already recycled pid. Also improved error message if child process fails. Proof of Concept: 1. Compile nochld: -- #include #include int main(void) { char *argv[] = { "vipw", NULL }; signal(SIGCHLD, SIG_IGN); execvp("vipw", argv); return 1; } -- 2. Run nochld 3. Suspend child vi, which suspends vipw too: `kill -STOP childpid` 4. Kill vi: `kill -9 childpid` 5. You can see with ps that childpid is no zombie but disappeared 6. Bring vipw back into foreground `fg` 7. See misleading warning message You will get an improperly formatted warning message. Also the wake up kill call sent SIGCONT to "childpid" which could have been assigned to another process already. This is definitely not a vulnerability. It would take super user operations, at which point an attacker would have already elevated permissions. Signed-off-by: Tobias Stoeckmann --- login-utils/vipw.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/login-utils/vipw.c b/login-utils/vipw.c index f59948a443..5049706aee 100644 --- a/login-utils/vipw.c +++ b/login-utils/vipw.c @@ -114,6 +114,9 @@ static void pw_init(void) (void)signal(SIGTSTP, SIG_IGN); (void)signal(SIGTTOU, SIG_IGN); + /* Set SIGCHLD to default for waitpid. */ + (void)signal(SIGCHLD, SIG_DFL); + /* Create with exact permissions. */ (void)umask(0); } @@ -202,7 +205,7 @@ static void pw_edit(void) } for (;;) { pid = waitpid(pid, &pstat, WUNTRACED); - if (WIFSTOPPED(pstat)) { + if (pid != -1 && WIFSTOPPED(pstat)) { /* the editor suspended, so suspend us as well */ kill(getpid(), SIGSTOP); kill(pid, SIGCONT); @@ -210,8 +213,12 @@ static void pw_edit(void) break; } } - if (pid == -1 || !WIFEXITED(pstat) || WEXITSTATUS(pstat) != 0) + if (pid == -1) pw_error(editor, 1, 1); + else if (!WIFEXITED(pstat) || WEXITSTATUS(pstat) != 0) { + warnx("%s: unsuccessful execution", editor); + pw_error(editor, 0, 1); + } free(editor); } @@ -221,7 +228,7 @@ pw_error(char *name, int err, int eval) { if (err) { if (name) - warn("%s: ", name); + warn("%s", name); else warn(NULL); } -- 2.47.3