From: Tobias Stoeckmann <tobias@stoeckmann.org>
Date: Sun, 14 Nov 2021 10:54:28 +0000 (+0100)
Subject: vipw: improve child error handling
X-Git-Tag: v2.38-rc1~167
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=330dcf98682a478bd776f339ac2afafdccbdd790;p=thirdparty%2Futil-linux.git

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 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);
 	}