Suspend the child process and wait for SIGUSR when using ptrace.

This fixes a race condition in ptrace-based intercept mode when
running the command in a pty.  It was possible for the monitor to
receive SIGCHLD when the command sent itself SIGSTOP before the
main sudo process did.

src/exec_common.c

@@ -89,31 +89,6 @@ enable_intercept(char *envp[], const char *dso, int intercept_fd)
     debug_return_ptr(envp);
 }
 
-/*
- * Called right before execve(2).
- * The tracee will be suspended until the tracer resumes it.
- */
-static void
-enable_ptrace(void)
-{
-#ifdef HAVE_PTRACE_INTERCEPT
-    const pid_t pid = getpid();
-    debug_decl(enable_ptrace, SUDO_DEBUG_UTIL);
-
-    /*
-     * Parent will trace child and intercept execve(2).
-     * We stop the child here so the parent can seize control.
-     */
-    sudo_debug_printf(SUDO_DEBUG_INFO, "%s: suspending child %d",
-        __func__, (int)pid);
-    kill(pid, SIGSTOP);
-    sudo_debug_printf(SUDO_DEBUG_INFO, "%s: resuming child %d",
-        __func__, (int)pid);
-
-    debug_return;
-#endif /* HAVE_PTRACE_INTERCEPT */
-}
-
 /*
  * Like execve(2) but falls back to running through /bin/sh
  * ala execvp(3) if we get ENOEXEC.
@@ -129,10 +104,12 @@ sudo_execve(int fd, const char *path, char *const argv[], char *envp[],
     /* Modify the environment as needed to trap execve(). */
     if (ISSET(flags, CD_NOEXEC))
 	envp = disable_execute(envp, sudo_conf_noexec_path());
-    else if (ISSET(flags, CD_USE_PTRACE))
-	enable_ptrace();
-    else if (ISSET(flags, CD_INTERCEPT|CD_LOG_SUBCMDS))
-	envp = enable_intercept(envp, sudo_conf_intercept_path(), intercept_fd);
+    if (ISSET(flags, CD_INTERCEPT|CD_LOG_SUBCMDS)) {
+	if (!ISSET(flags, CD_USE_PTRACE)) {
+	    envp = enable_intercept(envp, sudo_conf_intercept_path(),
+		intercept_fd);
+	}
+    }
 
 #ifdef HAVE_FEXECVE
     if (fd != -1)

src/exec_monitor.c

@@ -536,6 +536,14 @@ fill_exec_closure_monitor(struct monitor_closure *mc,
     debug_return;
 }
 
+#ifdef HAVE_PTRACE_INTERCEPT
+static void
+handler(int signo)
+{
+    /* just return */
+}
+#endif /* HAVE_PTRACE_INTERCEPT */
+
 /*
  * Monitor process that creates a new session with the controlling tty,
  * resets signal handlers and forks a child to call exec_cmnd_pty().
@@ -626,11 +634,28 @@ exec_monitor(struct command_details *details, sigset_t *oset,
 	goto bad;
     case 0:
 	/* child */
-	sigprocmask(SIG_SETMASK, oset, NULL);
 	close(backchannel);
 	close(errpipe[0]);
 	if (io_fds[SFD_USERTTY] != -1)
 	    close(io_fds[SFD_USERTTY]);
+#ifdef HAVE_PTRACE_INTERCEPT
+	if (ISSET(details->flags, CD_USE_PTRACE)) {
+	    sigset_t set;
+
+	    /* Tracer will send us SIGUSR1 when it is time to proceed. */
+	    sa.sa_handler = handler;
+	    if (sudo_sigaction(SIGUSR1, &sa, NULL) != 0) {
+		sudo_warn(U_("unable to set handler for signal %d"),
+		    SIGUSR1);
+	    }
+
+	    /* Suspend child until tracer seizes control and sends SIGUSR1. */
+	    sigfillset(&set);
+	    sigdelset(&set, SIGUSR1);
+	    sigsuspend(&set);
+	}
+#endif /* HAVE_PTRACE_INTERCEPT */
+	sigprocmask(SIG_SETMASK, oset, NULL);
 	restore_signals();
 
 	/* setup tty and exec command */

src/exec_nopty.c

@@ -344,6 +344,14 @@ free_exec_closure_nopty(struct exec_closure_nopty *ec)
     debug_return;
 }
 
+#ifdef HAVE_PTRACE_INTERCEPT
+static void
+handler(int signo)
+{
+    /* just return */
+}
+#endif /* HAVE_PTRACE_INTERCEPT */
+
 /*
  * Execute a command and wait for it to finish.
  */
@@ -414,10 +422,29 @@ exec_nopty(struct command_details *details, struct command_status *cstat)
 	break;
     case 0:
 	/* child */
-	sigprocmask(SIG_SETMASK, &oset, NULL);
 	close(errpipe[0]);
 	if (intercept_sv[0] != -1)
 	    close(intercept_sv[0]);
+#ifdef HAVE_PTRACE_INTERCEPT
+	if (ISSET(details->flags, CD_USE_PTRACE)) {
+	    struct sigaction sa;
+
+	    /* Tracer will send us SIGUSR1 when it is time to proceed. */
+	    memset(&sa, 0, sizeof(sa));
+	    sigemptyset(&sa.sa_mask);
+	    sa.sa_flags = SA_RESTART;
+	    sa.sa_handler = handler;
+	    if (sudo_sigaction(SIGUSR1, &sa, NULL) != 0) {
+		sudo_warn(U_("unable to set handler for signal %d"),
+		    SIGUSR1);
+	    }
+
+	    /* Suspend child until tracer seizes control and sends SIGUSR1. */
+	    sigdelset(&set, SIGUSR1);
+	    sigsuspend(&set);
+	}
+#endif /* HAVE_PTRACE_INTERCEPT */
+	sigprocmask(SIG_SETMASK, &oset, NULL);
 	exec_cmnd(details, intercept_sv[1], errpipe[1]);
 	while (write(errpipe[1], &errno, sizeof(int)) == -1) {
 	    if (errno != EINTR)

src/exec_ptrace.c

@@ -310,7 +310,7 @@ ptrace_write_string(pid_t pid, long addr, const char *str)
 	}
 	if (ptrace(PTRACE_POKEDATA, pid, addr, u.word) == -1) {
 	    sudo_warn("ptrace(PTRACE_POKEDATA, %d, 0x%lx, %.*s)",
-		pid, addr, (int)sizeof(u.buf), u.buf);
+		(int)pid, addr, (int)sizeof(u.buf), u.buf);
 	    debug_return_size_t(-1);
 	}
 	addr += sizeof(long);
@@ -470,7 +470,7 @@ ptrace_fail_syscall(pid_t pid, struct user_pt_regs *regs, int ecode)
     /* Allow the syscall to continue and change return value to ecode. */
     ptrace(PTRACE_SYSCALL, pid, NULL, NULL);
     for (;;) {
-	if (waitpid(pid, &status, WUNTRACED) != -1)
+	if (waitpid(pid, &status, __WALL) != -1)
 	    break;
 	if (errno == EINTR)
 	    continue;
@@ -571,7 +571,6 @@ exec_ptrace_seize(pid_t child)
     const long ptrace_opts = PTRACE_O_TRACESECCOMP|PTRACE_O_TRACECLONE|
 			     PTRACE_O_TRACEFORK|PTRACE_O_TRACEVFORK;
     int status;
-    pid_t pid;
     debug_decl(exec_ptrace_seize, SUDO_DEBUG_UTIL);
 
     /* Seize control of the child process. */
@@ -580,12 +579,18 @@ exec_ptrace_seize(pid_t child)
 	    ptrace_opts);
 	debug_return_bool(false);
     }
+    /* The child is suspended waiting for SIGUSR1, wake it up. */
+    if (kill(child, SIGUSR1) == -1) {
+	sudo_warn("kill(%d, SIGUSR1)", child);
+	debug_return_bool(false);
+    }
 
-    /* The child will stop itself immediately before execve(2). */
-    do {
-	pid = waitpid(child, &status, WUNTRACED);
-    } while (pid == -1 && errno == EINTR);
-    if (pid == -1) {
+    /* Wait for the child to enter trace stop and continue it. */
+    for (;;) {
+	if (waitpid(child, &status, __WALL) != -1)
+	    break;
+	if (errno == EINTR)
+	    continue;
 	sudo_warn(U_("%s: %s"), __func__, "waitpid");
 	debug_return_bool(false);
     }
@@ -593,8 +598,8 @@ exec_ptrace_seize(pid_t child)
 	sudo_warnx(U_("process %d exited unexpectedly"), (int)child);
 	debug_return_bool(false);
     }
-    if (ptrace(PTRACE_CONT, child, NULL, NULL) == -1) {
-	sudo_warn("ptrace(PTRACE_CONT, %d, NULL, NULL)", (int)child);
+    if (ptrace(PTRACE_CONT, child, NULL, (long)SIGUSR1) == -1) {
+	sudo_warn("ptrace(PTRACE_CONT, %d, NULL, SIGUSR1)", (int)child);
 	debug_return_bool(false);
     }
 
@@ -687,7 +692,7 @@ ptrace_intercept_execve(pid_t pid, struct intercept_closure *closure)
 		/* Store string address as new_argv[i]. */
 		if (ptrace(PTRACE_POKEDATA, pid, sp, strtab) == -1) {
 		    sudo_warn("ptrace(PTRACE_POKEDATA, %d, 0x%lx, 0x%lx)",
-			pid, sp, strtab);
+			(int)pid, sp, strtab);
 		    goto done;
 		}
 		sp += sizeof(long);
@@ -800,11 +805,11 @@ exec_ptrace_handled(pid_t pid, int status, void *intercept)
 	 * until SIGCONT is received (simulate SIGSTOP, etc).
 	 */
 	if (ptrace(PTRACE_LISTEN, pid, NULL, 0L) == -1)
-	    sudo_warn("ptrace(PTRACE_LISTEN,, %d, NULL, %d", pid, stopsig);
+	    sudo_warn("ptrace(PTRACE_LISTEN, %d, NULL, %d)", (int)pid, stopsig);
     } else {
 	/* Restart child. */
 	if (ptrace(PTRACE_CONT, pid, NULL, signo) == -1)
-	    sudo_warn("ptrace(PTRACE_CONT, %d, NULL, %d", pid, stopsig);
+	    sudo_warn("ptrace(PTRACE_CONT, %d, NULL, %d)", (int)pid, stopsig);
     }
 
     debug_return_bool(signo == 0);