Bug 3084 - Because of the nesting of signal processing functions in SFTP process, many linux system processes in the system are killed.
Summary: Because of the nesting of signal processing functions in SFTP process, many l...
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sftp (show other bugs)
Version: 8.1p1
Hardware: All Linux
: P1 security
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_8_2
  Show dependency treegraph
 
Reported: 2019-10-23 22:17 AEDT by gao rui
Modified: 2021-03-04 09:52 AEDT (History)
2 users (show)

See Also:


Attachments
test local copy of sshpid (371 bytes, patch)
2019-10-27 12:07 AEDT, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description gao rui 2019-10-23 22:17:06 AEDT
SFTP receives SIGTERM and SIGCHLD signals at the same time, which may kill almost all processes of the linux system(kill SIGTERM -1).

1、A process sends SIGTERM to sftp
2、sigchld_handler is running,after executing "if (sshpid > 1)", sftp receive SIGCHLD 

static void killchild(int signo)
{
	if (sshpid > 1) {
                                         <-------- Receive SIGCHLD signal
		kill(sshpid, SIGTERM);
		(void) waitpid(sshpid, NULL, 0);
	}

	_exit(1);
}

3、subprocess(ssh) connection closed,it send SIGCHLD  to sftp
4、sigchld_handler start execution on the same cpu,so sshpid =-1

static void sigchld_handler(int sig)
{
 .......
		(void)write(STDERR_FILENO, msg, sizeof(msg) - 1);
		sshpid = -1;	<----
}

5、killchild continue to finish,and exec "kill(-1, SIGTERM);"


I think add a patch

diff --git a/sftp.c b/sftp.c
index b66037f..833e034 100644
--- a/sftp.c
+++ b/sftp.c
@@ -2297,6 +2297,7 @@ static void
 connect_to_server(char *path, char **args, int *in, int *out)
 {
        int c_in, c_out;
+    struct sigaction kact,sact;

 #ifdef USE_PIPES
        int pin[2], pout[2];
@@ -2343,12 +2344,21 @@ connect_to_server(char *path, char **args, int *in, int *out)
                _exit(1);
        }

-       signal(SIGTERM, killchild);
-       signal(SIGINT, killchild);
-       signal(SIGHUP, killchild);
-       signal(SIGTSTP, suspchild);
-       signal(SIGTTIN, suspchild);
-       signal(SIGTTOU, suspchild);
+    kact.sa_handler = killchild;
+    sigemptyset(&kact.sa_mask);
+    sigaddset(&kact.sa_mask, SIGCHLD);
+    sigaction(SIGTERM, &kact, NULL);
+    sigaction(SIGINT, &kact, NULL);
+    sigaction(SIGHUP, &kact, NULL);
+
+    sact.sa_handler = suspchild;
+    sigemptyset(&sact.sa_mask);
+    sigaddset(&sact.sa_mask, SIGCHLD);
+    sigaction(SIGTSTP, suspchild);
+    sigaction(SIGTTIN, suspchild);
+    sigaction(SIGTTOU, suspchild);
+
+
        signal(SIGCHLD, sigchld_handler);
        close(c_in);
        close(c_out);
Comment 1 Damien Miller 2019-10-27 12:07:11 AEDT
Created attachment 3337 [details]
test local copy of sshpid

IMO this is a better fix - take a local copy of sshpid and test/kill that rather than using a racy test,kill sequence
Comment 2 Darren Tucker 2019-10-27 15:08:43 AEDT
Comment on attachment 3337 [details]
test local copy of sshpid

ok dtucker

>+	pid_t pid;
>+
>+	pid = sshpid;

you could combine these into one line.

As a separate thing I'm working on replacing all of the signal() calls with sigaction(), but that can go in later when it's ready.
Comment 3 Darren Tucker 2019-10-27 15:10:07 AEDT
and for the record: strictly speaking I don't think there's any guarantee that pid_t is an atomic type, although in practice it probably is the same as sig_atomic_t.
Comment 4 Damien Miller 2019-11-01 14:54:46 AEDT
patch applied and will be in OpenSSH 8.2 - thanks!
Comment 5 Damien Miller 2021-03-04 09:52:19 AEDT
close bugs that were resolved in OpenSSH 8.5 release cycle