openssh bugs - Oct 2019 - [Bug 3084] New: Because of the nesting of signal processing functions in SFTP process, many linux system processes in the system are killed.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

            Bug ID: 3084
           Summary: Because of the nesting of signal processing functions
                    in SFTP process, many linux system processes in the
                    system are killed.
           Product: Portable OpenSSH
           Version: 8.1p1
          Hardware: Other
                OS: Windows 7
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sftp
          Assignee: unassigned-bugs at mindrot.org
          Reporter: 15328076128 at 163.com

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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

gao rui <15328076128 at 163.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|Windows 7                   |Linux
           Priority|P5                          |P1
           Hardware|Other                       |All
           Severity|enhancement                 |security

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
                 CC|                            |djm at mindrot.org,
                   |                            |dtucker at dtucker.net
             Status|NEW                         |ASSIGNED
   Attachment #3337|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Created attachment 3337
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3337&action=edit
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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3079


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3079
[Bug 3079] Tracking bug for 8.2 release
-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3337|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

--- Comment #2 from Darren Tucker <dtucker at dtucker.net> ---
Comment on attachment 3337
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3337
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

--- Comment #3 from Darren Tucker <dtucker at dtucker.net> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
patch applied and will be in OpenSSH 8.2 - thanks!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3084

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
close bugs that were resolved in OpenSSH 8.5 release cycle

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.