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 <signal.h>
 #include <unistd.h>
 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 <tobias@stoeckmann.org>

diff --git a/login-utils/vipw.c b/login-utils/vipw.c
index f59948a443fcdf9391935f74abe435d08cf2fab3..5049706aeefb8d5e9bb2732d3bc9dfbb2f03f2fb 100644 (file)
--- 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);
        }