* 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.

diff --git a/ChangeLog b/ChangeLog
index d6ad13c46deef7c29cf4d06b4c163e9c19b9b51d..99fb654047b3401e87ae63c5ea63f316e3f9a3ec 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2011-06-05  Nicolas François  <nicolas.francois@centraliens.net>
+
+       * 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  <nicolas.francois@centraliens.net>
 
        * NEWS, src/userdel.c: Do not remove a group with the same name as

diff --git a/NEWS b/NEWS
index 3b731e96af033d959d6e24b05664079808335ef6..f961b74cf42d742ccfbeb1fe6d7000eacb8b4d7d 100644 (file)
--- 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 bd4e9432fe056d8d207f4159f9b1af80b4094f41..f93db7dc34bcac46301b9afb6e23aa3dcad6ec25 100644 (file)
--- 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);