63 lines
1.7 KiB
Diff
63 lines
1.7 KiB
Diff
From 624d57c08caceed306212d24c2147f6273f3fc4b Mon Sep 17 00:00:00 2001
|
|
From: Tobias Stoeckmann <tobias@stoeckmann.org>
|
|
Date: Sun, 14 Nov 2021 12:01:32 +0100
|
|
Subject: [PATCH] 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.
|
|
|
|
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`
|
|
|
|
The kill call sends SIGCONT to "childpid" which in turn could have been
|
|
already recycled for another process.
|
|
|
|
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>
|
|
|
|
Conflict: NA
|
|
Reference: https://github.com/shadow-maint/shadow/commit/624d57c08caceed306212d24c2147f6273f3fc4b
|
|
|
|
---
|
|
src/vipw.c | 3 +++
|
|
1 file changed, 3 insertions(+)
|
|
|
|
diff --git a/src/vipw.c b/src/vipw.c
|
|
index 94185c3df..1a69ef285 100644
|
|
--- a/src/vipw.c
|
|
+++ b/src/vipw.c
|
|
@@ -349,6 +349,9 @@ vipwedit (const char *file, int (*file_lock) (void), int (*file_unlock) (void))
|
|
sigprocmask(SIG_BLOCK, &mask, &omask);
|
|
}
|
|
|
|
+ /* set SIGCHLD to default for waitpid */
|
|
+ signal(SIGCHLD, SIG_DFL);
|
|
+
|
|
for (;;) {
|
|
pid = waitpid (pid, &status, WUNTRACED);
|
|
if ((pid != -1) && (WIFSTOPPED (status) != 0)) {
|