Fix GTK GUI crash: don't reap children ettercap doesn't own (#1310)

plug-ins/remote_browser/remote_browser.c

@@ -28,6 +28,9 @@
 
 #include <stdlib.h>
 #include <string.h>
+#ifndef OS_WINDOWS
+   #include <sys/wait.h>
+#endif
 
 /* protos */
 
@@ -110,6 +113,7 @@ static void remote_browser(struct packet_object *po)
    char *command;
    char **param = NULL;
    int i = 0, k = 0;
+   pid_t child_pid;
 
    /* the client is making a request */
    if (po->DATA.disp_len != 0 && strstr((const char*)po->DATA.disp_data, "GET")) {
@@ -162,8 +166,22 @@ static void remote_browser(struct packet_object *po)
       /* NULL terminate the array */
       SAFE_REALLOC(param, (i + 1) * sizeof(char *));
       param[i] = NULL;
-      /* execute the script */ 
-      if (fork() == 0) {
+      /* execute the script */
+      /*
+       * use a double-fork so the browser (a long-running child we must not
+       * wait for) is reparented to init (PID 1) and reaped by it. The parent
+       * only waits for the short-lived intermediate child by its specific
+       * PID below. This avoids relying on a global SIGCHLD reaper, which
+       * would steal child processes spawned by linked libraries such as
+       * GTK/glycin and make their own waitpid() fail with ECHILD.
+       */
+      child_pid = fork();
+      if (child_pid == 0) {
+         pid_t gchild = fork();
+         if (gchild == -1)
+            _exit(-E_INVALID);
+         if (gchild) /* intermediate child: leave, init adopts the grandchild */
+            _exit(0);
          /* chrome won't start as root, changing UID in order to prevent this and for more security in the browser context */
          /* the following line has been commented since some Penetration Testing distros can run only as root */
          /*setuid(1000);*/
@@ -191,7 +209,13 @@ static void remote_browser(struct packet_object *po)
          WARN_MSG("Cannot launch the default browser (command: %s), please edit your etter.conf file and put a valid value in remote_browser field\n", EC_GBL_CONF->remote_browser);
          _exit(-E_INVALID);
       }
-
+
+      /* parent: reap the short-lived intermediate child by its specific PID */
+#ifndef OS_WINDOWS
+      if (child_pid > 0)
+         waitpid(child_pid, NULL, 0);
+#endif
+
       //to free the char **param
       for(k= 0; k < i; ++k)
     	  SAFE_FREE(param[k]);

src/ec_filter.c

@@ -28,6 +28,9 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#ifndef OS_WINDOWS
+   #include <sys/wait.h>
+#endif
 
 #define JIT_FAULT(x, ...) do { USER_MSG("JIT FILTER FAULT: " x "\n", ## __VA_ARGS__); return -E_FATAL; } while(0)
 
@@ -990,6 +993,17 @@ static int func_execreplace(struct filter_op *fop, struct packet_object *po)
    /* close pipe stream */
    close(child_stdout[0]);
 
+   /*
+    * reap our own child here instead of relying on a global SIGCHLD
+    * handler. The child is already done producing output at this point.
+    * Reaping the specific PID avoids stealing (and mis-reaping) child
+    * processes spawned by linked libraries such as GTK/glycin, which do
+    * their own waitpid() and would otherwise fail with ECHILD.
+    */
+#ifndef OS_WINDOWS
+   waitpid(child_pid, NULL, 0);
+#endif
+
    /* check if we are overflowing pcap buffer */
    if(EC_GBL_PCAP->snaplen - (po->L4.header - (po->packet + po->L2.len) + po->L4.len) <= po->DATA.len + (unsigned)offset)
       JIT_FAULT("replaced output too long");
@@ -1215,49 +1229,68 @@ static int func_exec(struct filter_op *fop)
 
    DEBUG_MSG("filter engine: func_exec: %s", fop->op.func.string);
 
-   /* 
+   /*
     * the command must be executed by a child.
-    * we are forwding packets, and we cannot wait 
-    * for the execution of the command 
+    * we are forwding packets, and we cannot wait
+    * for the execution of the command
+    *
+    * use a double-fork so the process that actually runs the command is
+    * reparented to init (PID 1) and reaped by it. This lets us reap our
+    * own short-lived intermediate child by its specific PID below, rather
+    * than relying on a global SIGCHLD handler that calls waitpid(-1) and
+    * would otherwise steal child processes spawned by linked libraries
+    * such as GTK/glycin (causing them to fail with ECHILD).
     */
    pid = fork();
-   
+
    /* check if the fork was successful */
    if (pid == -1)
       SEMIFATAL_ERROR("filter engine: fork() failed, cannot execute %s", fop->op.func.string);
-   
-   /* differentiate between the parent and the child */
+
+   /* differentiate between the parent and the intermediate child */
    if (!pid) {
       int k, param_length;
       char **param = NULL;
       char *q = (char*)fop->op.func.string;
       char *p;
       int i = 0;
+      pid_t gchild;
+
+      /*
+       * fork once more: the grandchild runs the command and is reparented
+       * to init when this intermediate child exits right below, so nobody
+       * has to wait() for the long-running command.
+       */
+      gchild = fork();
+      if (gchild == -1)
+         _exit(-1);
+      if (gchild) /* intermediate child: leave, init adopts the grandchild */
+         _exit(0);
 
       /* split the string */
       for (p = strsep(&q, " "); p != NULL; p = strsep(&q, " ")) {
          /* allocate the array */
          SAFE_REALLOC(param, (i + 1) * sizeof(char *));
-         
+
          /* copy the tokens in the array */
-         param[i++] = strdup(p); 
+         param[i++] = strdup(p);
       }
-      
+
       /* NULL terminate the array */
       SAFE_REALLOC(param, (i + 1) * sizeof(char *));
-      
+
       param[i] = NULL;
       param_length= i + 1; //because there is a SAFE_REALLOC after the for.
-     
-      /* 
+
+      /*
        * close input, output and error.
-       * we don't want to clobber the interface 
+       * we don't want to clobber the interface
        * with output from the child
        */
       close(fileno(stdin));
       close(fileno(stdout));
       close(fileno(stderr));
-      
+
       /* execute the command */
       execve(param[0], param, NULL);
 
@@ -1267,7 +1300,12 @@ static int func_exec(struct filter_op *fop)
 	   SAFE_FREE(param);
       _exit(-1);
    }
-
+
+   /* parent: reap the short-lived intermediate child by its specific PID */
+#ifndef OS_WINDOWS
+   waitpid(pid, NULL, 0);
+#endif
+
    return E_SUCCESS;
 }
 

src/ec_redirect.c

@@ -60,6 +60,7 @@ int ec_redirect(ec_redir_act_t action, char *name, ec_redir_proto_t proto,
    char asc_dport[16];
    char asc_destination[MAX_ASCII_ADDR_LEN];
    int  ret_val = 0;
+   pid_t child_pid;
    char *param[4];
    char *commands[2] = {NULL, NULL};
    char *command = NULL;
@@ -248,7 +249,8 @@ int ec_redirect(ec_redir_act_t action, char *name, ec_redir_proto_t proto,
    param[3] = NULL;
 
    /* execute the script */
-   switch (fork()) {
+   child_pid = fork();
+   switch (child_pid) {
       case 0:
          regain_privs();
          execvp(param[0], param);
@@ -265,7 +267,12 @@ int ec_redirect(ec_redir_act_t action, char *name, ec_redir_proto_t proto,
          SAFE_FREE(command);
          return -E_INVALID;
       default:
-         wait(&ret_val);
+         /*
+          * wait for our specific child, not any child (waitpid vs wait):
+          * a bare wait() could reap a child spawned by a linked library
+          * such as GTK/glycin and make its own waitpid() fail with ECHILD.
+          */
+         waitpid(child_pid, &ret_val, 0);
          if (WIFEXITED(ret_val) && WEXITSTATUS(ret_val)) {
             DEBUG_MSG("ec_redirect(): child exited with non-zero return "
                   "code: %d", WEXITSTATUS(ret_val));

src/ec_signals.c

@@ -46,7 +46,6 @@ void signal_handler(void);
 static handler_t *signal_handle(int signo, handler_t *handler, int flags);
 static void signal_SEGV(int sig);
 static void signal_TERM(int sig);
-static void signal_CHLD(int sig);
 static void signal_USR1(int sig);
 
 /*************************************/
@@ -57,7 +56,6 @@ void signal_handler(void)
 
 // fixing windows warnings
    (void) signal_SEGV;
-   (void) signal_CHLD;
 
 #ifdef SIGSEGV
    signal_handle(SIGSEGV, signal_SEGV, 0);
@@ -72,7 +70,18 @@ void signal_handler(void)
    signal_handle(SIGTERM, signal_TERM, 0);
 #endif
 #ifdef SIGCHLD
-   signal_handle(SIGCHLD, signal_CHLD, 0);
+   /*
+    * Keep SIGCHLD at its default disposition instead of installing a
+    * process-wide reaper. A handler that calls waitpid(-1, ...) reaps ANY
+    * child, including ones spawned by linked libraries (e.g. GTK/glycin
+    * forks a bwrap sandbox and waits on it). Stealing such a child makes
+    * the library's own waitpid() fail with ECHILD, which crashes the GTK
+    * GUI (see issue #1310). Every child ettercap forks is now reaped
+    * explicitly by its own PID at the fork site, so no global reaper is
+    * needed. SIG_DFL (not SIG_IGN) is important: SIG_IGN would auto-reap
+    * and break the libraries the same way.
+    */
+   signal_handle(SIGCHLD, SIG_DFL, 0);
 #endif
 #ifdef SIGPIPE
    /* needed by sslwrap */
@@ -236,21 +245,6 @@ static void signal_TERM(int sig)
 }
 
 
-/*
- * received when a child exits
- */
-static void signal_CHLD(int sig)
-{
-   /* variable not used */
-   (void) sig;
-#ifndef OS_WINDOWS
-   int stat;
-
-   /* wait for the child to return and not become a zombie */
-   while (waitpid (-1, &stat, WNOHANG) > 0);
-#endif
-}
-
 static void signal_USR1(int sig)
 {
    /* variable not used */

src/os/ec_linux.c

@@ -267,6 +267,7 @@ void disable_interface_offload(void)
 	char **param = NULL;
 	char *p;
 	int ret_val, i = 0;
+	pid_t child_pid;
 
 	SAFE_CALLOC(command, 100, sizeof(char));
 
@@ -286,7 +287,8 @@ void disable_interface_offload(void)
 	param[i] = NULL;
 	param_length= i + 1; //because there is a SAFE_REALLOC after the for.
 
-	switch(fork()) {
+	child_pid = fork();
+	switch(child_pid) {
 		case 0:
 #ifndef DEBUG
 			/* don't print on console if the ethtool cannot disable some offloads unless you are in debug mode */
@@ -301,8 +303,13 @@ void disable_interface_offload(void)
          break;
 		default:
 			safe_free_mem(param, &param_length, command);
-			wait(&ret_val);
-	} 	
+			/*
+			 * wait for our specific child, not any child (waitpid vs wait):
+			 * a bare wait() could reap a child spawned by a linked library
+			 * such as GTK/glycin and make its own waitpid() fail with ECHILD.
+			 */
+			waitpid(child_pid, &ret_val, 0);
+	}
 }
 
 #ifdef WITH_IPV6