Commit 4db57cb4 authored by Ben Gamari's avatar Ben Gamari 🐢
Browse files

Call initgroups before setuid

Previously we would fail to call initgroups before setuid'ing. This
meant that our groups we not be reset to reflect those our new user
belongs to. Fix this.
parent 73ea41b3
......@@ -33,6 +33,10 @@ static long max_fd = 0;
extern void blockUserSignals(void);
extern void unblockUserSignals(void);
// These are arbitrarily chosen -- JP
#define forkSetgidFailed 124
#define forkSetuidFailed 125
// See #1593. The convention for the exit code when
// exec() fails seems to be 127 (gleened from C's
// system()), but there's no equivalent convention for
......@@ -40,9 +44,8 @@ extern void unblockUserSignals(void);
#define forkChdirFailed 126
#define forkExecFailed 127
// These are arbitrarily chosen -- JP
#define forkSetgidFailed 124
#define forkSetuidFailed 125
#define forkGetpwuidFailed 128
#define forkInitgroupsFailed 129
__attribute__((__noreturn__))
static void childFailed(int pipe, int failCode) {
......@@ -182,6 +185,23 @@ runInteractiveProcess (char *const args[],
}
if ( childUser) {
// Using setuid properly first requires that we initgroups.
// However, to do this we must know the username of the user we are
// switching to.
struct passwd pw;
struct passwd *res = NULL;
int buf_len = sysconf(_SC_GETPW_R_SIZE_MAX);
char *buf = malloc(buf_len);
gid_t suppl_gid = childGroup ? *childGroup : getgid();
if ( getpwuid_r(*childUser, &pw, buf, buf_len, &res) != 0) {
childFailed(forkCommunicationFds[1], forkGetpwuidFailed);
}
if ( res == NULL ) {
childFailed(forkCommunicationFds[1], forkGetpwuidFailed);
}
if ( initgroups(res->pw_name, suppl_gid) != 0) {
childFailed(forkCommunicationFds[1], forkInitgroupsFailed);
}
if ( setuid( *childUser) != 0) {
// ERROR
childFailed(forkCommunicationFds[1], forkSetuidFailed);
......@@ -330,6 +350,12 @@ runInteractiveProcess (char *const args[],
case forkSetuidFailed:
*failed_doing = "runInteractiveProcess: setuid";
break;
case forkGetpwuidFailed:
*failed_doing = "runInteractiveProcess: getpwuid";
break;
case forkInitgroupsFailed:
*failed_doing = "runInteractiveProcess: initgroups";
break;
default:
*failed_doing = "runInteractiveProcess: unknown";
break;
......
......@@ -2,6 +2,11 @@
## Unreleased changes
* Fix a potential privilege escalation issue (or, more precisely, privileges
not being dropped when this was the user's intent) where the groups of the
spawning process's user would be incorrectly retained due to a missing call to
`initgroups` [#149].
## 1.6.5.1 *June 2019*
* Version bound bumps
......
......@@ -21,6 +21,10 @@
#include <unistd.h>
#include <sys/types.h>
#if !(defined(_MSC_VER) || defined(__MINGW32__) || defined(_WIN32))
#include <pwd.h>
#include <grp.h>
#endif
#ifdef HAVE_FCNTL_H
#include <fcntl.h>
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment