Re: Privilege dropping for rtadvd

[christos%astron.com@localhost (Christos Zoulas)]

In article <b3b090ca8034e2fa4774752055a94513%mail.marples.name@localhost>,
Roy Marples  <roy%marples.name@localhost> wrote:
>-=-=-=-=-=-
>
>On 27/06/2013 16:53, Roy Marples wrote:
>> On 27/06/2013 16:35, logan%elandsys.com@localhost wrote:
>>> Well, I've already starting working on a diff. Would you be 
>>> interested
>>> in reviewing it :-) ?
>>
>> Sure!
>
>I didn't review a diff :(
>
>Here's one I cooked up in my spare time.
>Comments?
>
>Any objections or I'll commit this soonish.
>
>Roy
>-=-=-=-=-=-
>Index: usr.sbin/rtadvd/rtadvd.c
>===================================================================
>RCS file: /cvsroot/src/usr.sbin/rtadvd/rtadvd.c,v
>retrieving revision 1.43
>diff -u -p -r1.43 rtadvd.c
>--- usr.sbin/rtadvd/rtadvd.c   28 Jun 2013 07:59:32 -0000      1.43
>+++ usr.sbin/rtadvd/rtadvd.c   28 Jun 2013 11:38:35 -0000
>@@ -58,6 +58,7 @@
> #include <util.h>
> #endif
> #include <poll.h>
>+#include <pwd.h>
> 
> #include "rtadvd.h"
> #include "rrenum.h"
>@@ -177,6 +178,7 @@ main(int argc, char *argv[])
>       struct timeval *timeout;
>       int i, ch;
>       int fflag = 0, logopt;
>+      struct passwd *pw;
> 
>       /* get command line options and arguments */
> #define OPTIONS "c:dDfM:Rs"
>@@ -260,6 +262,37 @@ main(int argc, char *argv[])
>       } else
>               set[1].fd = -1;
> 
>+      errno = 0;

You should not need that, getpwnam() sets errno. Anyway syslog(3) calls a
bunch of stuff that can trash errno anyway...

>+      syslog(LOG_INFO, "<%s> dropping privileges to %s",
>+              __func__, RTADVD_USER);

I don't think that the function name is useful in a syslog message.

>+      if ((pw = getpwnam(RTADVD_USER)) == NULL) {
>+              /* Preserve the old behaviour if the user does not exist */
>+              if (errno == 0) {
>+                      syslog(LOG_WARNING,
>+                          "<%s> user does not exist, not dropping privileges",

IBID

>+                          __func__);
>+                      goto setsig;
>+              }
>+              syslog(LOG_ERR, "getpwnam: %s: %m", RTADVD_USER);
>+              exit(1);
>+      }
>+      if (chroot(pw->pw_dir) == -1) {
>+              syslog(LOG_ERR, "chroot: %s: %m", pw->pw_dir);
>+              exit(1);
>+      }
>+      if (chdir("/") == -1) {
>+              syslog(LOG_ERR, "chdir(\"/\")");

It should be (for consistency):
                syslog(LOG_ERR, "chdir /: %m");

>+              exit(1);
>+      }
>+      if (setgroups(1, &pw->pw_gid) == -1 ||
>+          setgid(pw->pw_gid) == -1 || 
>+          setuid(pw->pw_uid) == -1)
>+      {
>+              syslog(LOG_ERR, "failed to drop privileges: %m");
>+              exit(1);
>+      }