From 40be34049db1a2f39419b1fa2a371d68ad75439f Mon Sep 17 00:00:00 2001 From: smitsohu Date: Sat, 19 Jan 2019 23:50:47 +0100 Subject: signal handler fixes/improvements --- src/firejail/firejail.h | 1 + src/firejail/join.c | 21 +++++++++++++++++++- src/firejail/main.c | 48 ++++++++++++++++++++++++++++++++++++++-------- src/firejail/sandbox.c | 51 +++++++++++++++++++++++++++++++++++++------------ src/firejail/util.c | 31 ++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 21 deletions(-) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index a3d3dfcd4..03ad25f75 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -551,6 +551,7 @@ void disable_file_or_dir(const char *fname); void disable_file_path(const char *path, const char *file); int safe_fd(const char *path, int flags); int invalid_sandbox(const pid_t pid); +int has_handler(pid_t pid, int signal); void enter_network_namespace(pid_t pid); // Get info regarding the last kernel mount operation from /proc/self/mountinfo diff --git a/src/firejail/join.c b/src/firejail/join.c index 4259644f7..d05a4a465 100644 --- a/src/firejail/join.c +++ b/src/firejail/join.c @@ -41,7 +41,15 @@ static void signal_handler(int sig){ exit(sig); } - +static void install_handler(void) { + struct sigaction sga; + + // handle SIGTERM + sigemptyset(&sga.sa_mask); + sga.sa_handler = signal_handler; + sga.sa_flags = 0; + sigaction(SIGTERM, &sga, NULL); +} static void extract_x11_display(pid_t pid) { char *fname; @@ -465,8 +473,19 @@ void join(pid_t pid, int argc, char **argv, int index) { } int status = 0; + //***************************** + // following code is signal-safe + + install_handler(); + // wait for the child to finish waitpid(child, &status, 0); + + // restore default signal action + signal(SIGTERM, SIG_DFL); + + // end of signal-safe code + //***************************** flush_stdin(); if (WIFEXITED(status)) { diff --git a/src/firejail/main.c b/src/firejail/main.c index faa4972e2..61f507f36 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c @@ -157,12 +157,35 @@ static void myexit(int rv) { exit(rv); } -static void my_handler(int s){ - EUID_ROOT(); +static void my_handler(int s) { fmessage("\nParent received signal %d, shutting down the child process...\n", s); logsignal(s); - kill(child, SIGTERM); - myexit(1); + if (waitpid(child, NULL, WNOHANG) == 0) { + if (has_handler(child, s)) // signals are not delivered if there is no handler yet + kill(child, s); + else + kill(child, SIGKILL); + waitpid(child, NULL, 0); + } + myexit(s); +} + +static void install_handler(void) { + struct sigaction sga; + + // block SIGTERM while handling SIGINT + sigemptyset(&sga.sa_mask); + sigaddset(&sga.sa_mask, SIGTERM); + sga.sa_handler = my_handler; + sga.sa_flags = 0; + sigaction(SIGINT, &sga, NULL); + + // block SIGINT while handling SIGTERM + sigemptyset(&sga.sa_mask); + sigaddset(&sga.sa_mask, SIGINT); + sga.sa_handler = my_handler; + sga.sa_flags = 0; + sigaction(SIGTERM, &sga, NULL); } // return 1 if error, 0 if a valid pid was found @@ -2601,16 +2624,25 @@ int main(int argc, char **argv) { flock(lockfd_network, LOCK_UN); close(lockfd_network); } + EUID_USER(); + + int status = 0; + //***************************** + // following code is signal-safe // handle CTRL-C in parent - signal (SIGINT, my_handler); - signal (SIGTERM, my_handler); + install_handler(); // wait for the child to finish - EUID_USER(); - int status = 0; waitpid(child, &status, 0); + // restore default signal actions + signal(SIGTERM, SIG_DFL); + signal(SIGINT, SIG_DFL); + + // end of signal-safe code + //***************************** + // free globals if (cfg.profile) { ProfileEntry *prf = cfg.profile; diff --git a/src/firejail/sandbox.c b/src/firejail/sandbox.c index 475779f47..ba9a36250 100644 --- a/src/firejail/sandbox.c +++ b/src/firejail/sandbox.c @@ -53,6 +53,7 @@ static int force_nonewprivs = 0; static int monitored_pid = 0; static void sandbox_handler(int sig){ + usleep(10000); // don't race to print a message fmessage("\nChild received signal %d, shutting down the sandbox...\n", sig); // broadcast sigterm to all processes in the group @@ -81,10 +82,8 @@ static void sandbox_handler(int sig){ monsec--; } free(monfile); - } - // broadcast a SIGKILL kill(-1, SIGKILL); flush_stdin(); @@ -92,6 +91,23 @@ static void sandbox_handler(int sig){ exit(sig); } +static void install_handler(void) { + struct sigaction sga; + + // block SIGTERM while handling SIGINT + sigemptyset(&sga.sa_mask); + sigaddset(&sga.sa_mask, SIGTERM); + sga.sa_handler = sandbox_handler; + sga.sa_flags = 0; + sigaction(SIGINT, &sga, NULL); + + // block SIGINT while handling SIGTERM + sigemptyset(&sga.sa_mask); + sigaddset(&sga.sa_mask, SIGINT); + sga.sa_handler = sandbox_handler; + sga.sa_flags = 0; + sigaction(SIGTERM, &sga, NULL); +} static void set_caps(void) { if (arg_caps_drop_all) @@ -123,7 +139,6 @@ static void save_nogroups(void) { fprintf(stderr, "Error: cannot save nogroups state\n"); exit(1); } - } static void save_nonewprivs(void) { @@ -235,9 +250,17 @@ static void chk_chroot(void) { } static int monitor_application(pid_t app_pid) { + EUID_ASSERT(); monitored_pid = app_pid; - signal (SIGTERM, sandbox_handler); - EUID_USER(); + + // block signals and install handler + sigset_t oldmask, newmask; + sigemptyset(&oldmask); + sigemptyset(&newmask); + sigaddset(&newmask, SIGTERM); + sigaddset(&newmask, SIGINT); + sigprocmask(SIG_BLOCK, &newmask, &oldmask); + install_handler(); // handle --timeout int options = 0;; @@ -260,16 +283,25 @@ static int monitor_application(pid_t app_pid) { pid_t rv; do { + // handle signals asynchronously + sigprocmask(SIG_SETMASK, &oldmask, NULL); + rv = waitpid(-1, &status, options); - if (rv == -1) + + // block signals again + sigprocmask(SIG_BLOCK, &newmask, NULL); + + if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD) + sleep(1); break; + } // handle --timeout if (options) { if (--timeout == 0) { kill(-1, SIGTERM); - flush_stdin(); sleep(1); + flush_stdin(); _exit(1); } else @@ -279,11 +311,6 @@ static int monitor_application(pid_t app_pid) { while(rv != monitored_pid); if (arg_debug) printf("Sandbox monitor: waitpid %d retval %d status %d\n", monitored_pid, rv, status); - if (rv == -1) { // we can get here if we have processes joining the sandbox (ECHILD) - if (arg_debug) - perror("waitpid"); - sleep(1); - } DIR *dir; if (!(dir = opendir("/proc"))) { diff --git a/src/firejail/util.c b/src/firejail/util.c index 8c474f966..6b30ab5fe 100644 --- a/src/firejail/util.c +++ b/src/firejail/util.c @@ -1264,6 +1264,37 @@ int invalid_sandbox(const pid_t pid) { return 0; } +int has_handler(pid_t pid, int signal) { + if (signal > 0) { + char *fname; + if (asprintf(&fname, "/proc/%d/status", pid) == -1) + errExit("asprintf"); + EUID_ROOT(); + FILE *fp = fopen(fname, "re"); + EUID_USER(); + free(fname); + if (fp) { + char buf[BUFLEN]; + while (fgets(buf, BUFLEN, fp)) { + if (strncmp(buf, "SigCgt:", 7) == 0) { + char *ptr = buf + 7; + unsigned long long val; + if (sscanf(ptr, "%llx", &val) != 1) { + fprintf(stderr, "Error: cannot read /proc file\n"); + exit(1); + } + val >>= (signal - 1); + val &= 1; + fclose(fp); + return val; // 1 if process has a handler for the signal, else 0 + } + } + fclose(fp); + } + } + return 0; +} + void enter_network_namespace(pid_t pid) { // in case the pid is that of a firejail process, use the pid of the first child process pid_t child = switch_to_child(pid); -- cgit v1.2.3-54-g00ecf