diff options
| -rw-r--r-- | ChangeLog | 9 | ||||
| -rw-r--r-- | configure.in | 3 | ||||
| -rw-r--r-- | contrib/linux-tor-prio.sh | 6 | ||||
| -rw-r--r-- | contrib/rc.subr | 4 | ||||
| -rw-r--r-- | contrib/tor.sh.in | 2 | ||||
| -rw-r--r-- | contrib/torctl.in | 6 | ||||
| -rw-r--r-- | doc/tor.1.in | 6 | ||||
| -rw-r--r-- | src/common/compat.c | 224 | ||||
| -rw-r--r-- | src/common/compat.h | 2 | ||||
| -rw-r--r-- | src/or/config.c | 18 |
10 files changed, 217 insertions, 63 deletions
@@ -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; |