From b5403415f794aea02a4dea659751f8ed38519037 Mon Sep 17 00:00:00 2001 From: nekral-guest Date: Sun, 5 Jun 2011 14:41:15 +0000 Subject: [PATCH] * NEWS, src/su.c: Do not forward the controlling terminal to commands executed with -c. This prevents tty hijacking which could lead to execution with the caller's privileges. This required to forward signals from the terminal (SIGINT, SIGQUIT, SIGTSTP) to the executed command. --- ChangeLog | 8 ++++++++ NEWS | 11 +++++++++-- src/su.c | 59 ++++++++++++++++++++++++++++++++++++++++++++----------- 3 files changed, 64 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index d6ad13c4..99fb6540 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2011-06-05 Nicolas François + + * NEWS, src/su.c: Do not forward the controlling terminal to + commands executed with -c. This prevents tty hijacking which could + lead to execution with the caller's privileges. This required to + forward signals from the terminal (SIGINT, SIGQUIT, SIGTSTP) to + the executed command. + 2011-06-05 Nicolas François * NEWS, src/userdel.c: Do not remove a group with the same name as diff --git a/NEWS b/NEWS index 3b731e96..f961b74c 100644 --- a/NEWS +++ b/NEWS @@ -2,7 +2,11 @@ $Id$ shadow-4.1.4.3 -> shadow-4.1.5 UNRELEASED -- general +*** security + * su -c could be abused by the executed command to invoke commands with + the caller privileges. See below. + +*** general * report usage error to stderr, but report usage help to stdout (and return zero) when explicitly requested (e.g. with --help). * initial support for tcb (http://openwall.com/tcb/) for useradd, @@ -39,6 +43,9 @@ shadow-4.1.4.3 -> shadow-4.1.5 UNRELEASED list of TTYs. * Fixed warning and support for CONSOLE_GROUPS for users member of more than 16 groups. + * Do not forward the controlling terminal to commands executed with -c. + This prevents tty hijacking which could lead to execution with the + caller's privileges. - newgrp, sg, groupmems * Fix parsing of gshadow entries. - useradd @@ -59,7 +66,7 @@ shadow-4.1.4.3 -> shadow-4.1.5 UNRELEASED shadow-4.1.4.2 -> shadow-4.1.4.3 2011-02-15 -*** security: +*** security - CVE-2011-0721: An insufficient input sanitation in chfn can be exploited to create users or groups in a NIS environment. diff --git a/src/su.c b/src/su.c index bd4e9432..f93db7dc 100644 --- a/src/su.c +++ b/src/su.c @@ -88,7 +88,7 @@ static bool change_environment; #ifdef USE_PAM static pam_handle_t *pamh = NULL; -static bool caught = false; +static int caught = 0; /* PID of the child, in case it needs to be killed */ static pid_t pid_child = 0; #endif @@ -235,9 +235,9 @@ static void execve_shell (const char *shellstr, #ifdef USE_PAM /* Signal handler for parent process later */ -static void catch_signals (unused int sig) +static void catch_signals (int sig) { - caught = true; + caught = sig; } /* This I ripped out of su.c from sh-utils after the Mandrake pam patch @@ -264,6 +264,11 @@ static void run_shell (const char *shellstr, char *args[], bool doshell, if (doshell) { (void) shell (shellstr, (char *) args[0], envp); } else { + /* There is no need for a controlling terminal. + * This avoids the callee to inject commands on + * the caller's tty. */ + (void) setsid (); + execve_shell (shellstr, (char **) args, envp); } @@ -283,9 +288,9 @@ static void run_shell (const char *shellstr, char *args[], bool doshell, (void) fprintf (stderr, _("%s: signal malfunction\n"), Prog); - caught = true; + caught = SIGTERM; } - if (!caught) { + if (0 == caught) { struct sigaction action; action.sa_handler = catch_signals; @@ -296,36 +301,66 @@ static void run_shell (const char *shellstr, char *args[], bool doshell, if ( (sigaddset (&ourset, SIGTERM) != 0) || (sigaddset (&ourset, SIGALRM) != 0) || (sigaction (SIGTERM, &action, NULL) != 0) + || ( !doshell /* handle SIGINT (Ctrl-C), SIGQUIT + * (Ctrl-\), and SIGTSTP (Ctrl-Z) + * since the child does not control + * the tty anymore. + */ + && ( (sigaddset (&ourset, SIGINT) != 0) + || (sigaddset (&ourset, SIGQUIT) != 0) + || (sigaddset (&ourset, SIGTSTP) != 0) + || (sigaction (SIGINT, &action, NULL) != 0) + || (sigaction (SIGQUIT, &action, NULL) != 0)) + || (sigaction (SIGTSTP, &action, NULL) != 0)) || (sigprocmask (SIG_UNBLOCK, &ourset, NULL) != 0) ) { fprintf (stderr, _("%s: signal masking malfunction\n"), Prog); - caught = true; + caught = SIGTERM; } } - if (!caught) { + if (0 == caught) { + bool stop = true; + do { pid_t pid; + stop = true; pid = waitpid (-1, &status, WUNTRACED); - if (((pid_t)-1 != pid) && (0 != WIFSTOPPED (status))) { + /* When interrupted by signal, the signal will be + * forwarded to the child, and termination will be + * forced later. + */ + if ( ((pid_t)-1 == pid) + && (EINTR == errno) + && (SIGTSTP == caught)) { + /* Except for SIGTSTP, which request to + * stop the child. + * We will SIGSTOP ourself on the next + * waitpid round. + */ + kill (child, SIGSTOP); + stop = false; + } else if ( ((pid_t)-1 != pid) + && (0 != WIFSTOPPED (status))) { /* The child (shell) was suspended. * Suspend su. */ kill (getpid (), SIGSTOP); /* wake child when resumed */ kill (pid, SIGCONT); + stop = false; } - } while (0 != WIFSTOPPED (status)); + } while (!stop); } - if (caught) { + if (0 != caught) { (void) fputs ("\n", stderr); (void) fputs (_("Session terminated, terminating shell..."), stderr); - (void) kill (child, SIGTERM); + (void) kill (child, caught); } ret = pam_close_session (pamh, 0); @@ -339,7 +374,7 @@ static void run_shell (const char *shellstr, char *args[], bool doshell, ret = pam_end (pamh, PAM_SUCCESS); - if (caught) { + if (0 != caught) { (void) signal (SIGALRM, kill_child); (void) alarm (2);