Patch from Jacob Appelbaum and me to make User option more robust, properly...

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

svn:r17200

ChangeLog

@@ -5,6 +5,13 @@ Changes in version 0.2.1.7-alpha - 2008-11-xx
       exit policy doesn't allow it, we would remember what IP address
       the relay said the destination address resolves to, even if it's
       an internal IP address. Bugfix on 0.2.0.7-alpha; patch by rovv.
+    - The "User" and "Group" config options did not clear the
+      supplementary group entries for the process. The "User" option
+      has been made more robust, and also now also sets 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.
 
   o Minor features:
     - Now NodeFamily and MyFamily config options allow spaces in

configure.in

@@ -630,6 +630,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,

doc/tor.1.in

@@ -264,10 +264,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),
@@ -350,7 +346,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

src/common/compat.c

@@ -920,61 +920,215 @@ 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.
  */
 int
-switch_id(const char *user, const char *group)
+log_credential_status()
+{
+#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 only switch to their
+ * primary group.  Return 0 on success.  On failure, log and return -1.
+ */
+int
+switch_id(const char *user)
 {
 #ifndef MS_WINDOWS
   struct passwd *pw = NULL;
-  struct group *gr = NULL;
+  uid_t old_uid;
+  gid_t old_gid;
+  
+  tor_assert(user);
 
+  /* Log the initial credential state */ 
   if (user) {
-    pw = getpwnam(user);
-    if (pw == NULL) {
-      log_warn(LD_CONFIG,"User '%s' not found.", user);
+    if (log_credential_status()) {
       return -1;
     }
   }
+  log_fn(CREDENTIAL_LOG_LEVEL, LD_GENERAL, "Changing user and groups");
 
-  /* 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);
+  /* 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. */
+  if (user) {
+    pw = getpwnam(user);
+    if (pw == NULL) {
+      log_warn(LD_CONFIG, "Error setting configured user: "
+      "'%s' not found.", user);
       return -1;
     }
+  } else {
+    /* We have no user supplied and so we'll bail out. */
+    log_warn(LD_CONFIG, "Error setting configured user: No user supplied.");
+    return -1;
+  }
 
-    if (setgid(gr->gr_gid) != 0) {
-      log_warn(LD_GENERAL,"Error setting to configured GID: %s",
-               strerror(errno));
+  /* Properly switch egid,gid,euid,uid here or bail out */
+  if (setgroups(1, &pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting configured groups: %s",
+    strerror(errno));
+    return -1;
+  }
+
+  if (setegid(pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting configured egid: %s",
+    strerror(errno));
+    return -1;
+  }
+
+  if (setgid(pw->pw_gid)) {
+    log_warn(LD_GENERAL, "Error setting configured gid: %s",
+    strerror(errno));
+    return -1;
+  }
+
+  if (setuid(pw->pw_uid)) {
+    log_warn(LD_GENERAL, "Error setting configured uid: %s",
+    strerror(errno));
+    return -1;
+  }
+
+  if (seteuid(pw->pw_uid)){
+    log_warn(LD_GENERAL, "Error setting configured euid: %s",
+    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");
       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");
       return -1;
     }
   }
+#endif
 
-  /* now that the group is switched, we can switch users and lose
-     privileges */
+  /* Check what really happened */
   if (user) {
-    if (setuid(pw->pw_uid) != 0) {
-      log_warn(LD_GENERAL,"Error setting UID: %s", strerror(errno));
+    if (log_credential_status()) {
       return -1;
     }
   }
 
   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;
 }
 

src/common/compat.h

@@ -447,7 +447,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

src/or/config.c

@@ -219,7 +219,7 @@ static config_var_t _option_vars[] = {
   V(GeoIPFile,                   FILENAME,
     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,     "1"),
@@ -434,7 +434,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 */
@@ -1084,13 +1083,12 @@ options_act_reversible(or_options_t *old_options, char **msg)
 #endif
 
   /* Setuid/setgid as appropriate */
-  if (options->User || options->Group) {
+  if (options->User) {
     /* XXXX021 We should only do this the first time through, not on
      * every setconf. */
-    if (switch_id(options->User, options->Group) != 0) {
+    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;
     }
   }