[PATCH] Silence compilation warnings about setuid, setgid

[PATCH] Silence compilation warnings about setuid, setgid

[Sebastian Gniazdowski]

[-- Attachment #1.1: Type: text/plain, Size: 1690 bytes --]

Hello,
on a Linux box I see:

options.c:772:2: warning: ignoring return value of ‘setuid’, declared with
attribute warn_unused_res
ult [-Wunused-result]
  setuid(getuid());
  ^~~~~~~~~~~~~~~~
options.c:773:2: warning: ignoring return value of ‘setgid’, declared with
attribute warn_unused_res
ult [-Wunused-result]
  setgid(getgid());
  ^~~~~~~~~~~~~~~~

Looking at the source, the reported calls are "extra" ones, they are
followed by proper setuid, setgid calls. I've found some way out from this
situation, of using the report value and reporting it (gmail paste, proper
patch is attached):

diff --git a/Src/options.c b/Src/options.c
index 590652ea9..e085af796 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,13 +769,23 @@ 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());
+       int uerr = 0, gerr = 0;
+
+       if(setuid(getuid())) {
+           uerr = errno;
+       }
+       if(setgid(getgid())) {
+           gerr = errno;
+       }
         if (setuid(getuid())) {
             zwarn("failed to change user ID: %e", errno);
+            if (uerr)
+                zwarn("(error of additional preceding setuid() call: %e)",
uerr);
             return -1;
        } else if (setgid(getgid())) {
             zwarn("failed to change group ID: %e", errno);
+            if (gerr)
+                zwarn("(error of additional preceding setgid() call: %e)",
gerr);
             return -1;
         }
 #else

[-- Attachment #1.2: Type: text/html, Size: 2082 bytes --]

[-- Attachment #2: setuid.diff.txt --]
[-- Type: text/plain, Size: 968 bytes --]

diff --git a/Src/options.c b/Src/options.c
index 590652ea9..e085af796 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,13 +769,23 @@ 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());
+	int uerr = 0, gerr = 0;
+
+	if(setuid(getuid())) {
+	    uerr = errno;
+	}
+	if(setgid(getgid())) {
+	    gerr = errno;
+	}
         if (setuid(getuid())) {
             zwarn("failed to change user ID: %e", errno);
+            if (uerr)
+                zwarn("(error of additional preceding setuid() call: %e)", uerr);
             return -1;
 	} else if (setgid(getgid())) {
             zwarn("failed to change group ID: %e", errno);
+            if (gerr)
+                zwarn("(error of additional preceding setgid() call: %e)", gerr);
             return -1;
         }
 #else

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

[Eitan Adler]

On 7 May 2018 at 04:18, Sebastian Gniazdowski <sgniazdowski@gmail.com> wrote:
> Hello,
> on a Linux box I see:
> Looking at the source, the reported calls are "extra" ones, they are
> followed by proper setuid, setgid calls. I've found some way out from this
> situation, of using the report value and reporting it (gmail paste, proper
> patch is attached):
>

>  #ifdef HAVE_SETUID
> -       setuid(getuid());
> -       setgid(getgid());

While we're touching this code can we please correct the order of
setuid and setgid?

setgid must be called before setuid. If setuid is called first, on
some platforms, it no longer has privs to call setgid aymore.

See https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges
for additional details


-- 
Eitan Adler

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

[Peter Stephenson]

On Wed, 13 Jun 2018 04:49:39 -0700
Eitan Adler <lists@eitanadler.com> wrote:
> On 7 May 2018 at 04:18, Sebastian Gniazdowski
> <sgniazdowski@gmail.com> wrote:
> > Hello,
> > on a Linux box I see:
> > Looking at the source, the reported calls are "extra" ones, they are
> > followed by proper setuid, setgid calls. I've found some way out
> > from this situation, of using the report value and reporting it
> > (gmail paste, proper patch is attached):
> >  
> 
> >  #ifdef HAVE_SETUID
> > -       setuid(getuid());
> > -       setgid(getgid());  
> 
> While we're touching this code can we please correct the order of
> setuid and setgid?
> 
> setgid must be called before setuid. If setuid is called first, on
> some platforms, it no longer has privs to call setgid aymore.

Presumably that's a trivial swap?  I don't know if we need both
setgid()s before both setuid()s, because I don't know why they're
repeated --- but if the second case is simply to test for an error that's
not a big deal since if it worked properly there won't be one.

I didn't look at the original patch before now --- the obvious way to
fix it would simply be a cast to void.  There's no comment about why the
code is like that, so perhaps retaining the error number is safer.
However, I think it's just confusing except in the (few?)  cases where
the error number is different the first time.  I ended up with this...

diff --git a/Src/options.c b/Src/options.c
index 590652e..14d9c3c 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,15 +769,24 @@ 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())) {
+	int uerr = 0, gerr = 0;
+
+	errno = 0;
+	if (setgid(getgid()))
+	    gerr = errno;
+	if (setuid(getuid()))
+	    uerr = errno;
+	if (setgid(getgid())) {
             zwarn("failed to change group ID: %e", errno);
+            if (gerr && gerr != errno)
+                zwarn("(error of additional preceding setgid() call: %e)", gerr);
             return -1;
-        }
+        } else if (setuid(getuid())) {
+            zwarn("failed to change user ID: %e", errno);
+            if (uerr && uerr != errno)
+                zwarn("(error of additional preceding setuid() call: %e)", uerr);
+            return -1;
+	}
 #else
         zwarn("setuid not available");
         return -1;

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

[Bart Schaefer]

On Wed, Jun 13, 2018 at 6:10 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
> On Wed, 13 Jun 2018 04:49:39 -0700
> Eitan Adler <lists@eitanadler.com> wrote:
>>
>> setgid must be called before setuid. If setuid is called first, on
>> some platforms, it no longer has privs to call setgid aymore.
>
> Presumably that's a trivial swap?  I don't know if we need both
> setgid()s before both setuid()s, because I don't know why they're
> repeated --- but if the second case is simply to test for an error that's
> not a big deal since if it worked properly there won't be one.

IIRC there are some cases in which setgid/setuid fail silently, i.e.,
without changing anything but without setting errno.  The second calls
are to double-check that the first one was not rogue in that way.

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.

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

[Peter Stephenson]

On Wed, 13 Jun 2018 08:08:27 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> IIRC there are some cases in which setgid/setuid fail silently, i.e.,
> without changing anything but without setting errno.  The second calls
> are to double-check that the first one was not rogue in that way.
> 
> 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.

Thanks, I'll change to a cast to void.

pws

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

[Eitan Adler]

On 13 June 2018 at 08:08, Bart Schaefer <schaefer@brasslantern.com> 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"



-- 
Eitan Adler

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

[Peter Stephenson]

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

> On 13 June 2018 at 08:08, Bart Schaefer <schaefer@brasslantern.com>
> 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;

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

[dana]

On 13 Jun 2018, at 12:19, Peter Stephenson <p.stephenson@samsung.com> wrote:
>+	(void)setgid(getgid());

Casting to void doesn't actually silence this warning with (at least some
versions of) GCC/glibc. If the function has the warn_unused_result attribute,
you have to (a) disable the warning entirely, (b) actually use the result
somehow, or (c) use some hack like this:
http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/ignore-value.h

dana

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

[Peter Stephenson]

On Wed, 13 Jun 2018 13:41:25 -0500
dana <dana@dana.is> wrote:
> On 13 Jun 2018, at 12:19, Peter Stephenson <p.stephenson@samsung.com>
> wrote:
> >+	(void)setgid(getgid());  
> 
> Casting to void doesn't actually silence this warning with (at least
> some versions of) GCC/glibc. If the function has the
> warn_unused_result attribute, you have to (a) disable the warning
> entirely, (b) actually use the result somehow, or (c) use some hack
> like this:
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/ignore-value.h

Something like the following therefore ought to work generally.
Disabling an explicit cast to void is pretty broken behaviour, so I'm
gradually losing interest if for some reason this doesn't work.

pws

diff --git a/Src/options.c b/Src/options.c
index 590652e..600b649 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -769,15 +769,32 @@ 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())) {
+	int ignore_err;
+	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.
+	 *
+	 * Some versions of gcc make it hard to ignore the results the
+	 * first time, hence the following.  (These are probably not
+	 * systems that require the doubled calls.)
+	 */
+	ignore_err = setgid(getgid());
+	(void)ignore_err;
+	ignore_err = setuid(getuid());
+	(void)ignore_err;
+	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;

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

[dana]

On 14 Jun 2018, at 03:41, Peter Stephenson <p.stephenson@samsung.com> wrote:
>+	ignore_err = setgid(getgid());
>+	(void)ignore_err;

FWIW, that does seem to shut it up.

dana

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

[Peter Stephenson]

On Thu, 14 Jun 2018 05:31:49 -0500
dana <dana@dana.is> wrote:

> On 14 Jun 2018, at 03:41, Peter Stephenson <p.stephenson@samsung.com>
> wrote:
> >+	ignore_err = setgid(getgid());
> >+	(void)ignore_err;  
> 
> FWIW, that does seem to shut it up.

Yeah, that's basically what the GNU workaround did, translated into
standard C... hopefully standard enough for everyone.

pws