Improve PRIVILEGED fixes (again)

* Pass RGID instead of passwd GID to initgroups()

* Clean up #ifdefs, avoid unnecessary checks

* Flatten conditions

1 files changed, 43 insertions, 49 deletions

diff --git a/Src/options.c b/Src/options.c
index 425e27dc2..e9577c863 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -781,91 +781,85 @@ dosetopt(int optno, int value, int force, char *new_opts)
     } else if(optno == PRIVILEGED && !value) {
 	/* unsetting PRIVILEGED causes the shell to make itself unprivileged */
 
+/* For simplicity's sake, require both setresgid() and setresuid() up-front. */
+#if !defined(HAVE_SETRESGID)
+	zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; setresgid() and friends not available");
+	return -1;
+#elif !defined(HAVE_SETRESUID)
+	zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; setresuid() and friends not available");
+	return -1;
+#else
 	/* If set, return -1 so lastval will be non-zero. */
 	int failed = 0;
-
-#ifdef HAVE_SETUID
 	const int orig_euid = geteuid();
-#endif
 	const int orig_egid = getegid();
 
 	/*
 	 * Set the GID first as if we set the UID to non-privileged it
 	 * might be impossible to restore the GID.
 	 */
-	{
-#ifndef HAVE_SETRESGID
-	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; setresgid() and friends not available");
+	if (setresgid(getgid(), getgid(), getgid())) {
+	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; failed to change group ID: %e", errno);
 	    return -1;
-#else
-	    int setgid_err;
-	    setgid_err = setresgid(getgid(), getgid(), getgid());
-	    if (setgid_err) {
-		zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; failed to change group ID: %e", errno);
-		return -1;
-	    }
-#endif
 	}
 
-	/* Set the UID second. */
-	{
-#ifndef HAVE_SETRESUID
-	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; setresuid() and friends not available");
-	    return -1;
-#else
-	    int setuid_err;
-
 # ifdef HAVE_INITGROUPS
-	    /* Set the supplementary groups list. */
-	    if (geteuid() == 0) {
-		struct passwd *pw = getpwuid(getuid());
-		if (pw == NULL) {
-		    zwarnnam("unsetopt", "can't drop privileges; failed to get user information for uid %L: %e",
-			    (long)getuid(), errno);
-		    failed = 1;
-		} else if (initgroups(pw->pw_name, pw->pw_gid)) {
-		    zwarnnam("unsetopt", "can't drop privileges; failed to set supplementary group list: %e", errno);
-		    return -1;
-		}
-	    } else if (getuid() != 0 &&
-		    (geteuid() != getuid() || orig_egid != getegid())) {
-		zwarnnam("unsetopt", "PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=%L",
-			(long)geteuid());
+	/* Set the supplementary groups list.
+	 *
+	 * Note that on macOS, FreeBSD, and possibly some other platforms,
+	 * initgroups() resets the EGID to its second argument (see setgroups(2) for
+	 * details). This has the potential to leave the EGID in an unexpected
+	 * state. However, it seems common in other projects that do this dance to
+	 * simply re-use the same GID that's going to become the EGID anyway, in
+	 * which case it doesn't matter. That's what we do here. It's therefore
+	 * possible, in some probably uncommon cases, that the shell ends up not
+	 * having the privileges of the RUID user's primary/passwd group. */
+	if (geteuid() == 0) {
+	    struct passwd *pw = getpwuid(getuid());
+	    if (pw == NULL) {
+		zwarnnam("unsetopt", "can't drop privileges; failed to get user information for uid %L: %e",
+		    (long)getuid(), errno);
 		failed = 1;
+	    /* This may behave strangely in the unlikely event that the same user
+	     * name appears with multiple UIDs in the passwd database */
+	    } else if (initgroups(pw->pw_name, getgid())) {
+		zwarnnam("unsetopt", "can't drop privileges; failed to set supplementary group list: %e", errno);
+		return -1;
 	    }
+	} else if (getuid() != 0 &&
+	    (geteuid() != getuid() || orig_egid != getegid())) {
+	    zwarnnam("unsetopt", "PRIVILEGED: supplementary group list not changed due to lack of permissions: EUID=%L",
+		(long)geteuid());
+	    failed = 1;
+	}
 # else
-	    /* initgroups() isn't in POSIX.  If it's not available on the system,
-	     * we silently skip it. */
+	/* initgroups() isn't in POSIX.  If it's not available on the system,
+	 * we silently skip it. */
 # endif
 
-	    setuid_err = setresuid(getuid(), getuid(), getuid());
-	    if (setuid_err) {
-		zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; failed to change user ID: %e", errno);
-		return -1;
-	    }
-#endif
+	/* Set the UID second. */
+	if (setresuid(getuid(), getuid(), getuid())) {
+	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; failed to change user ID: %e", errno);
+	    return -1;
 	}
 
-#ifdef HAVE_SETGID
 	if (getuid() != 0 && orig_egid != getegid() &&
 		(setgid(orig_egid) != -1 || setegid(orig_egid) != -1)) {
 	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; was able to restore the egid");
 	    return -1;
 	}
-#endif
 
-#ifdef HAVE_SETUID
 	if (getuid() != 0 && orig_euid != geteuid() &&
 		(setuid(orig_euid) != -1 || seteuid(orig_euid) != -1)) {
 	    zwarnnam("unsetopt", "PRIVILEGED: can't drop privileges; was able to restore the euid");
 	    return -1;
 	}
-#endif
 
 	if (failed) {
 	    /* A warning message has been printed. */
 	    return -1;
 	}
+#endif /* HAVE_SETRESGID && HAVE_SETRESUID */
 
 #ifdef JOB_CONTROL
     } else if (!force && optno == MONITOR && value) {