X-Git-Url: https://ruderich.org/simon/gitweb/?p=ptyas%2Fptyas.git;a=blobdiff_plain;f=ptyas.c;h=4b7b208856e88f53a43abc833ea4dde6a7c5bcdb;hp=f96962e857ec82cf2f8b51024f98f80675761cb7;hb=a39434b70dfe6428b3c8ef5061b6f0a1475974b5;hpb=468bfda91be14b76bb15ff8cd4d66bbc7d3db12e

diff --git a/ptyas.c b/ptyas.c
index f96962e..4b7b208 100644
--- a/ptyas.c
+++ b/ptyas.c
@@ -2,7 +2,7 @@
  * Run the login shell or command as the given user in a new pty to prevent
  * terminal injection attacks.
  *
- * Copyright (C) 2016  Simon Ruderich
+ * Copyright (C) 2016-2018  Simon Ruderich
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -39,12 +39,18 @@
 #include <termios.h>
 #include <unistd.h>
 
+/* Default PATH for new process.*/
+#ifndef PTYAS_DEFAULT_PATH
+/* Default user PATH from Debian's /etc/profile, change as needed. */
+# define PTYAS_DEFAULT_PATH "/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games"
+#endif
+
 
 static void die(const char *s) {
     perror(s);
     exit(EXIT_FAILURE);
 }
-static void die_msg(const char *fmt, ...) {
+static void die_fmt(const char *fmt, ...) {
     va_list ap;
 
     va_start(ap, fmt);
@@ -76,10 +82,11 @@ static void open_pty_or_die(int *pty_master, int *pty_slave, uid_t uid) {
     if (*pty_slave == -1) {
         die("open slave tty");
     }
-    /* The user must be able to write to the new TTY. Normally grantpt() would
+    /*
+     * The user must be able to write to the new TTY. Normally grantpt() would
      * do this for us, but we don't trust the user and thus don't want to pass
-     * the pty_master to a process running under that uid. */
-    // TODO: is this a problem?
+     * the pty_master to a process running under that uid.
+     */
     if (chown(slave_path, uid, (gid_t)-1) != 0) {
         die("chown slave tty");
     }
@@ -114,7 +121,7 @@ static void drop_privileges_or_die(uid_t uid, gid_t gid) {
         die("setgroups");
     }
     if (getgroups(0, NULL) != 0) {
-        die_msg("failed to drop all groups");
+        die_fmt("failed to drop all supplementary groups");
     }
 
     /* Dropping groups may require privileges, do that first. */
@@ -138,12 +145,12 @@ static void drop_privileges_or_die(uid_t uid, gid_t gid) {
         }
         if (       uid != ruid || uid != euid || uid != suid
                 || gid != rgid || gid != egid || gid != sgid) {
-            die_msg("failed to drop privileges");
+            die_fmt("failed to drop privileges");
         }
     }
     /* Just to be safe. */
     if (setuid(0) != -1) {
-        die_msg("failed to drop privileges (setuid)");
+        die_fmt("failed to drop privileges (setuid)");
     }
 }
 
@@ -206,19 +213,21 @@ static void proxy_input_between_ttys(int pty_master, int ctty, volatile pid_t *p
     while (*pid_to_wait_for != 0) {
         /*
          * If a signal happens here _and_ the child hasn't closed pty_slave,
-         * we will hang in poll(); therefore ppoll() is requred.
+         * we would hang in poll(); therefore ppoll() is necessary.
          */
         nfds_t nfds = sizeof(fds)/sizeof(*fds);
         if (ppoll(fds, nfds, NULL /* no timeout */, &sigset_old) == -1) {
             if (errno == EAGAIN || errno == EINTR) {
                 continue;
-            } else {
-                perror("poll");
             }
+            perror("poll");
             break;
         }
 
-        /* Handle errors first. */
+        /*
+         * Handle errors first. (Data available before the error occurred
+         * might be dropped, but shouldn't matter here.)
+         */
         if (fds[0].revents & (POLLERR | POLLNVAL)) {
             fprintf(stderr, "poll: error on master: %d\n", fds[0].revents);
             break;
@@ -228,7 +237,7 @@ static void proxy_input_between_ttys(int pty_master, int ctty, volatile pid_t *p
             break;
         }
 
-        /* Then read data if available. */
+        /* Read data if available. */
         if (fds[0].revents & POLLIN) {
             if (!read_from_write_to(pty_master, ctty)) {
                 perror("read from master write to ctty");
@@ -254,7 +263,10 @@ static void proxy_input_between_ttys(int pty_master, int ctty, volatile pid_t *p
 }
 
 
-/* Not sig_atomic_t but I don't know how to do that any other way. */
+/*
+ * Not sig_atomic_t (as required by POSIX) but I don't know how to do that any
+ * other way.
+ */
 static volatile pid_t pid_to_wait_for;
 static int pid_to_wait_for_status;
 
@@ -282,14 +294,14 @@ int main(int argc, char **argv) {
     } else if (argc > 2) {
         exec_argv = argv + 2;
     } else {
-        die_msg("%s <user> [<cmd>...]\n", argv[0]);
+        die_fmt("%s <user> [<cmd>...]\n", argv[0]);
     }
 
     const char *user = argv[1];
 
     struct passwd *passwd = getpwnam(user);
     if (!passwd) {
-        die_msg("unknown user name '%s'\n", user);
+        die_fmt("unknown user name '%s'\n", user);
     }
 
     uid_t uid = passwd->pw_uid;
@@ -321,6 +333,8 @@ int main(int argc, char **argv) {
     if (pid == -1) {
         die("fork parent");
     } else if (pid == 0) {
+        /* child, will become a session leader */
+
         if (sigprocmask(SIG_SETMASK, &sigset_old, NULL) != 0) {
             die("sigprocmask setmask child");
         }
@@ -349,6 +363,11 @@ int main(int argc, char **argv) {
         if (pid == -1) {
             die("fork child");
         } else if (pid == 0) {
+            /*
+             * Drop the privileges just now so that the other user doesn't get
+             * access to the master TTY or the session leader (which might
+             * have additional privileges).
+             */
             drop_privileges_or_die(uid, gid);
 
             dup2_or_die(pty_slave, STDIN_FILENO);
@@ -363,6 +382,14 @@ int main(int argc, char **argv) {
             }
             const char *home = passwd->pw_dir;
 
+            /*
+             * Ignore errors here as we don't want to die on non-existent home
+             * directories to allow running as any user (think "/nonexistent"
+             * as home) and an error message will be annoying to ignore when
+             * running this command in scripts.
+             */
+            chdir(home);
+
             char envp_user[strlen("USER=") + strlen(user) + 1];
             char envp_home[strlen("HOME=") + strlen(home) + 1];
             char envp_term[strlen("TERM=") + strlen(term) + 1];
@@ -371,7 +398,7 @@ int main(int argc, char **argv) {
             snprintf_or_assert(envp_term, sizeof(envp_term), "TERM=%s", term);
 
             char *exec_envp[] = {
-                "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
+                "PATH=" PTYAS_DEFAULT_PATH,
                 envp_user,
                 envp_home,
                 term_orig ? envp_term : NULL,
@@ -386,7 +413,7 @@ int main(int argc, char **argv) {
         close_or_die(STDOUT_FILENO);
         close_or_die(STDERR_FILENO);
 
-        // TODO: EINTR?
+        /* TODO: EINTR? */
         int status;
         if (waitpid(pid, &status, 0) <= 0) {
             die("waitpid child");
@@ -414,7 +441,7 @@ int main(int argc, char **argv) {
         die("tcgetattr");
     }
     term = old_term;
-    /* From man 3 cfmakeraw which is non-standard. */
+    /* From man 3 cfmakeraw; cfmakeraw is non-standard so set it manually. */
     term.c_iflag &= ~(tcflag_t)(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR | IGNCR | ICRNL | IXON);
     term.c_oflag &= ~(tcflag_t)(OPOST);
     term.c_lflag &= ~(tcflag_t)(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
@@ -431,8 +458,10 @@ int main(int argc, char **argv) {
         die("tcsetattr restore");
     }
 
-    /* Wait until we got the status code from our child. poll() might also
-     * exit after POLLHUP while we haven't collected the child yet. */
+    /*
+     * Wait until we got the status code from our child. poll() might already
+     * exit after POLLHUP while we haven't collected the child yet.
+     */
     if (sigprocmask(SIG_BLOCK, &sigset, &sigset_old) != 0) {
         die("sigprocmask block sigchld loop");
     }