Backport of changesets 17200, 17201, 17203-17206, 17228, 17232, 17236: Patch from Jacob Appelbaum and me to make User option more robust, properly set supplementary groups, deprecated the Group option, and log more information on credential switching. Fixes bugs 848 and 857

svn:r17255

10 files changed, 217 insertions, 63 deletions

diff --git a/ChangeLog b/ChangeLog
index 41b4432afa..8fb38da6fe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,13 @@
 Changes in version 0.2.0.32 - 2008-??-??
+  o Security fixes:
+    - The "User" and "Group" config options did not clear the
+      supplementary group entries for the Tor process. The "User" option
+      is now more robust, and we now set the groups to the specified
+      user's primary group. The "Group" option is now ignored. For more
+      detailed logging on credential switching, set CREDENTIAL_LOG_LEVEL
+      in common/compat.c to LOG_NOTICE or higher. Patch by Jacob Appelbaum
+      and Steven Murdoch. Bugfix on 0.0.2pre14. Fixes bug 848 and 857.
+
   o Major bugfixes:
     - Fix a DOS opportunity during the voting signature collection process
       at directory authorities. Spotted by rovv. Bugfix on 0.2.0.x.
diff --git a/configure.in b/configure.in
index ccf1005483..3e556252b4 100644
--- a/configure.in
+++ b/configure.in
@@ -616,6 +616,9 @@ syslog_facility="$withval", syslog_facility="LOG_DAEMON")
 AC_DEFINE_UNQUOTED(LOGFACILITY,$syslog_facility,[name of the syslog facility])
 AC_SUBST(LOGFACILITY)
 
+# Check if we have getresuid and getresgid
+AC_CHECK_FUNCS(getresuid getresgid)
+
 # Check for gethostbyname_r in all its glorious incompatible versions.
 #   (This logic is based on that in Python's configure.in)
 AH_TEMPLATE(HAVE_GETHOSTBYNAME_R,
diff --git a/contrib/linux-tor-prio.sh b/contrib/linux-tor-prio.sh
index 0ebb47564a..d7481668ca 100644
--- a/contrib/linux-tor-prio.sh
+++ b/contrib/linux-tor-prio.sh
@@ -9,8 +9,8 @@
 # This script provides prioritization of Tor traffic below other
 # traffic on a Linux server. It has two modes of operation: UID based 
 # and IP based. The UID based method requires that Tor be launched from 
-# a specific user ID. The "User" and "Group" Tor config settings are 
-# insufficient, as they set the UID after the socket is created.
+# a specific user ID. The "User" Tor config setting is
+# insufficient, as it sets the UID after the socket is created.
 # Here is a three line C wrapper you can use to execute Tor and drop 
 # privs to UID 501 before it creates any sockets. Change the UID 
 # to the UID for your tor server user, and compile with 
@@ -49,7 +49,7 @@
 
 DEV=eth0
 
-# NOTE! You must START Tor under this UID. Using the Tor User/Group 
+# NOTE! You must START Tor under this UID. Using the Tor User
 # config setting is NOT sufficient.
 TOR_UID=$(id -u tor)
 
diff --git a/contrib/rc.subr b/contrib/rc.subr
index 8852b60466..117ae71d47 100644
--- a/contrib/rc.subr
+++ b/contrib/rc.subr
@@ -14,7 +14,6 @@
 # tor_conf (str):       Points to your tor conf file
 #                       Default: /usr/local/etc/tor/torrc
 # tor_user (str):       Tor Daemon user. Default _tor
-# tor_groupr (str):     Tor Daemon group. Default _tor
 #
 
 . /etc/rc.subr
@@ -27,7 +26,6 @@ load_rc_config ${name}
 : ${tor_enable="NO"}
 : ${tor_conf="/usr/local/etc/tor/torrc"}
 : ${tor_user="_tor"}
-: ${tor_group="_tor"}
 : ${tor_pidfile="/var/run/tor/tor.pid"}
 : ${tor_logfile="/var/log/tor"}
 : ${tor_datadir="/var/run/tor"}
@@ -35,7 +33,7 @@ load_rc_config ${name}
 required_files=${tor_conf}
 required_dirs=${tor_datadir}
 command="/usr/local/bin/${name}"
-command_args="-f ${tor_conf} --pidfile ${tor_pidfile} --runasdaemon 1 --datadirectory ${tor_datadir} --user ${tor_user} --group ${tor_group}"
+command_args="-f ${tor_conf} --pidfile ${tor_pidfile} --runasdaemon 1 --datadirectory ${tor_datadir} --user ${tor_user}"
 extra_commands="log"
 log_cmd="${name}_log"
 
diff --git a/contrib/tor.sh.in b/contrib/tor.sh.in
index 362a455732..e169761a62 100644
--- a/contrib/tor.sh.in
+++ b/contrib/tor.sh.in
@@ -31,8 +31,6 @@ TORCTL=@BINDIR@/torctl
 # torctl will use these environment variables
 TORUSER=@TORUSER@
 export TORUSER
-TORGROUP=@TORGROUP@
-export TORGROUP
 
 if [ -x /bin/su ] ; then
     SUPROG=/bin/su
diff --git a/contrib/torctl.in b/contrib/torctl.in
index 4136bd9434..4cc137da46 100644
--- a/contrib/torctl.in
+++ b/contrib/torctl.in
@@ -41,22 +41,18 @@ TORDATA="@LOCALSTATEDIR@/lib/tor"
 TORARGS="--pidfile $PIDFILE --log \"notice file $LOGFILE\" --runasdaemon 1"
 TORARGS="$TORARGS --datadirectory $TORDATA"
 
-# If user and group names are set in the environment, then use them;
+# If user name is set in the environment, then use it;
 # otherwise run as the invoking user (or whatever user the config
 # file says)... unless the invoking user is root. The idea here is to
 # let an unprivileged user run tor for her own use using this script,
 # while still providing for it to be used as a system daemon.
 if [ "x`id -u`" = "x0" ]; then
     TORUSER=@TORUSER@
-    TORGROUP=@TORGROUP@
 fi
 
 if [ "x$TORUSER" != "x" ]; then
     TORARGS="$TORARGS --user $TORUSER"
 fi
-if [ "x$TORGROUP" != "x" ]; then
-    TORARGS="$TORARGS --group $TORGROUP"
-fi
 
 # We no longer wrap the Tor daemon startup in an su when running as
 # root, because it's too painful to make the use of su portable.
diff --git a/doc/tor.1.in b/doc/tor.1.in
index 54b4855842..14996b418b 100644
--- a/doc/tor.1.in
+++ b/doc/tor.1.in
@@ -259,10 +259,6 @@ script to enumerate Tor nodes that exit to certain addresses.
 (Default: 0)
 .LP
 .TP
-\fBGroup \fR\fIGID\fP
-On startup, setgid to this group.
-.LP
-.TP
 \fBHttpProxy\fR \fIhost\fR[:\fIport\fR]\fP
 Tor will make all its directory requests through this host:port
 (or host:80 if port is not specified),
@@ -345,7 +341,7 @@ about what sites a user might have visited. (Default: 1)
 .LP
 .TP
 \fBUser \fR\fIUID\fP
-On startup, setuid to this user.
+On startup, setuid to this user and setgid to their primary group.
 .LP
 .TP
 \fBHardwareAccel \fR\fB0\fR|\fB1\fP
diff --git a/src/common/compat.c b/src/common/compat.c
index c7bd0fb564..cfc73ff395 100644
--- a/src/common/compat.c
+++ b/src/common/compat.c
@@ -871,62 +871,220 @@ set_max_file_descriptors(rlim_t limit, int *max_out)
   return 0;
 }
 
-/** Call setuid and setgid to run as <b>user</b>:<b>group</b>.  Return 0 on
- * success.  On failure, log and return -1.
+/** Log details of current user and group credentials. Return 0 on
+ * success. Logs and return -1 on failure.
+ */
+static int
+log_credential_status(void)
+{
+#define CREDENTIAL_LOG_LEVEL LOG_INFO
+#ifndef MS_WINDOWS
+  /* Real, effective and saved UIDs */
+  uid_t ruid, euid, suid;
+  /* Read, effective and saved GIDs */
+  gid_t rgid, egid, sgid;
+  /* Supplementary groups */
+  gid_t sup_gids[NGROUPS_MAX + 1];
+  /* Number of supplementary groups */
+  int ngids;
+
+  /* log UIDs */
+#ifdef HAVE_GETRESUID
+  if (getresuid(&ruid, &euid, &suid) != 0 ) {
+    log_warn(LD_GENERAL, "Error getting changed UIDs: %s", strerror(errno));
+    return -1;
+  } else {
+    log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL,
+           "UID is %u (real), %u (effective), %u (saved)", ruid, euid, suid);
+  }
+#else
+  /* getresuid is not present on MacOS X, so we can't get the saved (E)UID */
+  ruid = getuid();
+  euid = geteuid();
+  (void)suid;
+
+  log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL,
+         "UID is %u (real), %u (effective), unknown (saved)", ruid, euid);
+#endif
+
+  /* log GIDs */
+#ifdef HAVE_GETRESGID
+  if (getresgid(&rgid, &egid, &sgid) != 0 ) {
+    log_warn(LD_GENERAL, "Error getting changed GIDs: %s", strerror(errno));
+    return -1;
+  } else {
+    log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL,
+           "GID is %u (real), %u (effective), %u (saved)", rgid, egid, sgid);
+  }
+#else
+  /* getresgid is not present on MacOS X, so we can't get the saved (E)GID */
+  rgid = getgid();
+  egid = getegid();
+  (void)sgid;
+  log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL,
+         "GID is %u (real), %u (effective), unknown (saved)", rgid, egid);
+#endif
+
+  /* log supplementary groups */
+  if ((ngids = getgroups(NGROUPS_MAX + 1, sup_gids)) < 0) {
+    log_warn(LD_GENERAL, "Error getting supplementary GIDs: %s",
+             strerror(errno));
+    return -1;
+  } else {
+    int i;
+    char *strgid;
+    char *s = NULL;
+    int formatting_error = 0;
+    smartlist_t *elts = smartlist_create();
+
+    for (i = 0; i<ngids; i++) {
+      strgid = tor_malloc(11);
+      if (tor_snprintf(strgid, 11, "%u", (unsigned)sup_gids[i]) == -1) {
+        log_warn(LD_GENERAL, "Error printing supplementary GIDs");
+        formatting_error = 1;
+        goto error;
+      }
+      smartlist_add(elts, strgid);
+    }
+
+    s = smartlist_join_strings(elts, " ", 0, NULL);
+
+    log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL, "Supplementary groups are: %s",s);
+
+   error:
+    tor_free(s);
+    SMARTLIST_FOREACH(elts, char *, cp,
+    {
+      tor_free(cp);
+    });
+    smartlist_free(elts);
+
+    if (formatting_error)
+      return -1;
+  }
+#endif
+
+  return 0;
+}
+
+/** Call setuid and setgid to run as <b>user</b> and switch to their
+ * primary group.  Return 0 on success.  On failure, log and return -1.
  */
 int
-switch_id(const char *user, const char *group)
+switch_id(const char *user)
 {
 #ifndef MS_WINDOWS
   struct passwd *pw = NULL;
-  struct group *gr = NULL;
+  uid_t old_uid;
+  gid_t old_gid;
+  static int have_already_switched_id = 0;
 
-  if (user) {
-    pw = getpwnam(user);
-    if (pw == NULL) {
-      log_warn(LD_CONFIG,"User '%s' not found.", user);
-      return -1;
-    }
+  tor_assert(user);
+
+  if (have_already_switched_id)
+    return 0;
+
+  /* Log the initial credential state */
+  if (log_credential_status())
+    return -1;
+
+  log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL, "Changing user and groups");
+
+  /* Get old UID/GID to check if we changed correctly */
+  old_uid = getuid();
+  old_gid = getgid();
+
+  /* Lookup the user and group information, if we have a problem, bail out. */
+  pw = getpwnam(user);
+  if (pw == NULL) {
+    log_warn(LD_CONFIG, "Error setting configured user: %s not found", user);
+    return -1;
   }
 
-  /* switch the group first, while we still have the privileges to do so */
-  if (group) {
-    gr = getgrnam(group);
-    if (gr == NULL) {
-      log_warn(LD_CONFIG,"Group '%s' not found.", group);
-      return -1;
-    }
+  /* Properly switch egid,gid,euid,uid here or bail out */
+  if (setgroups(1, &pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting groups to gid %d: \"%s\". "
+             "If you set the \"User\" option, you must start Tor as root.",
+             (int)pw->pw_gid, strerror(errno));
+    return -1;
+  }
 
-    if (setgid(gr->gr_gid) != 0) {
-      log_warn(LD_GENERAL,"Error setting to configured GID: %s",
-               strerror(errno));
+  if (setegid(pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting egid to %d: %s",
+             (int)pw->pw_gid, strerror(errno));
+    return -1;
+  }
+
+  if (setgid(pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting gid to %d: %s",
+             (int)pw->pw_gid, strerror(errno));
+    return -1;
+  }
+
+  if (setuid(pw->pw_uid)) {
+    log_warn(LD_GENERAL, "Error setting configured uid to %s (%d): %s",
+             user, (int)pw->pw_uid, strerror(errno));
+    return -1;
+  }
+
+  if (seteuid(pw->pw_uid)) {
+    log_warn(LD_GENERAL, "Error setting configured euid to %s (%d): %s",
+             user, (int)pw->pw_uid, strerror(errno));
+    return -1;
+  }
+
+  /* This is how OpenBSD rolls:
+  if (setgroups(1, &pw->pw_gid) || setegid(pw->pw_gid) ||
+      setgid(pw->pw_gid) || setuid(pw->pw_uid) || seteuid(pw->pw_uid)) {
+      setgid(pw->pw_gid) || seteuid(pw->pw_uid) || setuid(pw->pw_uid)) {
+    log_warn(LD_GENERAL, "Error setting configured UID/GID: %s",
+    strerror(errno));
+    return -1;
+  }
+  */
+
+  /* We've properly switched egid, gid, euid, uid, and supplementary groups if
+   * we're here. */
+
+#if !defined(CYGWIN) && !defined(__CYGWIN__)
+  /* If we tried to drop privilege to a group/user other than root, attempt to
+   * restore root (E)(U|G)ID, and abort if the operation succeeds */
+
+  /* Only check for privilege dropping if we were asked to be non-root */
+  if (pw->pw_uid) {
+    /* Try changing GID/EGID */
+    if (pw->pw_gid != old_gid &&
+        (setgid(old_gid) != -1 || setegid(old_gid) != -1)) {
+      log_warn(LD_GENERAL, "Was able to restore group credentials even after "
+               "switching GID: this means that the setgid code didn't work.");
       return -1;
     }
-  } else if (user) {
-    if (setgid(pw->pw_gid) != 0) {
-      log_warn(LD_GENERAL,"Error setting to user GID: %s", strerror(errno));
+
+    /* Try changing UID/EUID */
+    if (pw->pw_uid != old_uid &&
+        (setuid(old_uid) != -1 || seteuid(old_uid) != -1)) {
+      log_warn(LD_GENERAL, "Was able to restore user credentials even after "
+               "switching UID: this means that the setuid code didn't work.");
       return -1;
     }
   }
+#endif
 
-  /* now that the group is switched, we can switch users and lose
-     privileges */
-  if (user) {
-    if (setuid(pw->pw_uid) != 0) {
-      log_warn(LD_GENERAL,"Error setting UID: %s", strerror(errno));
-      return -1;
-    }
+  /* Check what really happened */
+  if (log_credential_status()) {
+    return -1;
   }
 
+  have_already_switched_id = 1; /* mark success so we never try again */
   return 0;
+
 #else
   (void)user;
-  (void)group;
-#endif
 
   log_warn(LD_CONFIG,
-           "User or group specified, but switching users is not supported.");
+           "User specified but switching users is unsupported on your OS.");
   return -1;
+#endif
 }
 
 #ifdef HAVE_PWD_H
diff --git a/src/common/compat.h b/src/common/compat.h
index ea66093568..8cf7302595 100644
--- a/src/common/compat.h
+++ b/src/common/compat.h
@@ -463,7 +463,7 @@ void set_uint32(char *cp, uint32_t v) ATTR_NONNULL((1));
 typedef unsigned long rlim_t;
 #endif
 int set_max_file_descriptors(rlim_t limit, int *max);
-int switch_id(const char *user, const char *group);
+int switch_id(const char *user);
 #ifdef HAVE_PWD_H
 char *get_user_homedir(const char *username);
 #endif
diff --git a/src/or/config.c b/src/or/config.c
index 5dc5f53aa2..71bab432e2 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -204,7 +204,7 @@ static config_var_t _option_vars[] = {
   V(GeoIPFile,                   STRING,
     SHARE_DATADIR PATH_SEPARATOR "tor" PATH_SEPARATOR "geoip"),
 #endif
-  V(Group,                       STRING,   NULL),
+  OBSOLETE("Group"),
   V(HardwareAccel,               BOOL,     "0"),
   V(HashedControlPassword,       LINELIST, NULL),
   V(HidServDirectoryV2,          BOOL,     "0"),
@@ -391,7 +391,6 @@ static config_var_description_t options_description[] = {
   /* { "FastFirstHopPK", "" }, */
   /* FetchServerDescriptors, FetchHidServDescriptors,
    * FetchUselessDescriptors */
-  { "Group", "On startup, setgid to this group." },
   { "HardwareAccel", "If set, Tor tries to use hardware crypto accelerators "
     "when it can." },
   /* HashedControlPassword */
@@ -1031,13 +1030,10 @@ options_act_reversible(or_options_t *old_options, char **msg)
 #endif
 
   /* Setuid/setgid as appropriate */
-  if (options->User || options->Group) {
-    /* XXXX021 We should only do this the first time through, not on
-     * every setconf. */
-    if (switch_id(options->User, options->Group) != 0) {
+  if (options->User) {
+    if (switch_id(options->User) != 0) {
       /* No need to roll back, since you can't change the value. */
-      *msg = tor_strdup("Problem with User or Group value. "
-                        "See logs for details.");
+      *msg = tor_strdup("Problem with User value. See logs for details.");
       goto done;
     }
   }
@@ -1868,9 +1864,9 @@ get_assigned_option(config_format_t *fmt, or_options_t *options,
         result->value = tor_strdup("");
       break;
     case CONFIG_TYPE_OBSOLETE:
-      log_warn(LD_CONFIG,
-               "You asked me for the value of an obsolete config option '%s'.",
-               key);
+      log_fn(LOG_PROTOCOL_WARN, LD_CONFIG,
+             "You asked me for the value of an obsolete config option '%s'.",
+             key);
       tor_free(result->key);
       tor_free(result);
       return NULL;