From 9d68ed08e9689cde194475657fb09f9693c38e5c Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Fri, 7 Nov 2008 02:06:12 +0000 Subject: [PATCH] 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 | 7 ++ configure.in | 3 + doc/tor.1.in | 6 +- src/common/compat.c | 216 +++++++++++++++++++++++++++++++++++++------- src/common/compat.h | 2 +- src/or/config.c | 10 +- 6 files changed, 201 insertions(+), 43 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4fd317f4a9..3b7eebe0ed 100644 --- a/ChangeLog +++ b/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 diff --git a/configure.in b/configure.in index d383a627a1..878a8da97f 100644 --- a/configure.in +++ b/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, diff --git a/doc/tor.1.in b/doc/tor.1.in index 67eb17e2a6..67a7db4f9f 100644 --- a/doc/tor.1.in +++ b/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 diff --git a/src/common/compat.c b/src/common/compat.c index 31029844d1..3678913a08 100644 --- a/src/common/compat.c +++ b/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 user:group. 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; iuser 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) { + 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. */ if (user) { pw = getpwnam(user); if (pw == NULL) { - log_warn(LD_CONFIG,"User '%s' not found.", user); + 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; + } + + /* 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; + } + + /* 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 - /* 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; - } - - if (setgid(gr->gr_gid) != 0) { - log_warn(LD_GENERAL,"Error setting to configured GID: %s", - strerror(errno)); - return -1; - } - } else if (user) { - if (setgid(pw->pw_gid) != 0) { - log_warn(LD_GENERAL,"Error setting to user GID: %s", strerror(errno)); - return -1; - } - } - - /* 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; } diff --git a/src/common/compat.h b/src/common/compat.h index f0036d7764..748b4e6538 100644 --- a/src/common/compat.h +++ b/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 diff --git a/src/or/config.c b/src/or/config.c index bd42860011..66ee6cd52b 100644 --- a/src/or/config.c +++ b/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; } }