Re: [PATCH] Silence compilation warnings about setuid, setgid

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 13 Jun 2018 10:13:53 -0700
Eitan Adler <lists@xxxxxxxxxxxxxx> wrote:

> On 13 June 2018 at 08:08, Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>
> wrote:
> > This may have been isolated to a 1990s-era OS that is no longer at
> > issue.  Either way there's no particular reason to save and report
> > errno from the first call.  
> 
> See
> https://wiki.sei.cmu.edu/confluence/display/c/POS37-C.+Ensure+that+privilege+relinquishment+is+successful
> I'm not sure what current systems have these issues, but explicitly
> checking the results of getuid after priv-drop is still considered a
> "good idea"

There's no question of not testing the final result, the question is
whether we need the result of the first one that might erroneously
report success.  I've now get this.

pws

diff --git a/Src/options.c b/Src/options.c
index 590652e..a6e3216 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,15 +769,25 @@ dosetopt(int optno, int value, int force, char *new_opts)
     } else if(optno == PRIVILEGED && !value) {
 	/* unsetting PRIVILEGED causes the shell to make itself unprivileged */
 #ifdef HAVE_SETUID
-	setuid(getuid());
-	setgid(getgid());
-        if (setuid(getuid())) {
-            zwarn("failed to change user ID: %e", errno);
-            return -1;
-	} else if (setgid(getgid())) {
+	errno = 0;
+	/*
+	 * Set the GID first as if we set the UID to non-privileged it
+	 * might be impossible to restore the GID.
+	 *
+	 * Some OSes (possibly no longer around) have been known to
+	 * fail silently the first time, so we attempt the change twice.
+	 * If it fails we are guaranteed to pick this up the second
+	 * time, so ignore the first time.
+	 */
+	(void)setgid(getgid());
+	(void)setuid(getuid());
+	if (setgid(getgid())) {
             zwarn("failed to change group ID: %e", errno);
             return -1;
-        }
+        } else if (setuid(getuid())) {
+            zwarn("failed to change user ID: %e", errno);
+            return -1;
+	}
 #else
         zwarn("setuid not available");
         return -1;