[ovs-dev,2/3] lib: Add --user for daemon
diff mbox

Message ID 1441323223-11889-2-git-send-email-azhou@nicira.com
State Changes Requested
Headers show

Commit Message

Andy Zhou Sept. 3, 2015, 11:33 p.m. UTC
Allow daemon running as root to accept --user option, that accepts
"user:group" string as input. Performs sanity check on the input,
and store the converted uid and gid.

daemon_become_new_user() needs to be called to make the actual
switch.

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 lib/daemon-unix.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/daemon.h      | 27 +++++++++++++++------
 2 files changed, 91 insertions(+), 7 deletions(-)

Comments

Ansis Atteka Sept. 8, 2015, 12:39 a.m. UTC | #1
On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote:

> Allow daemon running as root to accept --user option, that accepts
> "user:group" string as input. Performs sanity check on the input,
> and store the converted uid and gid.
>
> daemon_become_new_user() needs to be called to make the actual
> switch.


> Signed-off-by: Andy Zhou <azhou@nicira.com>
> ---
>  lib/daemon-unix.c | 71
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/daemon.h      | 27 +++++++++++++++------
>  2 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index d52ac2d..44eb800 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -20,6 +20,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <grp.h>
> +#include <pwd.h>
>  #include <signal.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -693,6 +694,76 @@ should_service_stop(void)
>      return false;
>  }
>
> +void daemon_set_new_user(const char *user_spec)
> +{
> +    char *pos = strchr(user_spec, ':');
> +
> +    uid = getuid();
> +    gid = getgid();
> +
> +    if (gid || uid) {
> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
> +    }
> +
> +    user_spec += strspn(user_spec, " \t\r\n");
> +    int len = pos ? pos - user_spec : strlen(user_spec);
> +    struct passwd pwd, *res;
> +    char buf[4096];
> +
> +    if (len) {
> +        user = xzalloc(len + 1);
> +        strncpy(user, user_spec, len);
> +
> +        if (getpwnam_r(user, &pwd, buf, 4096, &res)) {
>
instead of using 4096 I would use "sizeof buf" here and in other places.

However, from where did you get this "4096" number in the first place? I
see that in the GETPWNAM man page the documented way to get the expected
buffer size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason
you are not using that approach?



> +            VLOG_FATAL("%s: Invalid --user option %s (no such user %s)",
> +                       pidfile, user_spec, user);
> +        }
> +    } else {
> +        /* User is not specified, use current user.  */
> +        if (getpwuid_r(uid, &pwd, buf, 4096, &res)) {
> +            VLOG_FATAL("%s: Invalid --user option %s (failed to lookup "
> +                       "current user with uid %d)", pidfile, user_spec,
> uid);
> +        }
> +        user = strdup(pwd.pw_name);
> +    }
> +
> +    uid = pwd.pw_uid;
> +    gid = pwd.pw_gid;
> +
> +    if (pos) {
> +        char *grpstr = pos + 1;
> +        grpstr += strspn(grpstr, " \t\r\n");
> +
> +        if (*grpstr) {
> +            struct group grp, *res;
> +
> +            if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) {
> +                VLOG_FATAL("%s: Invalid --user option %s (unknown group
> %s)",
> +                           pidfile, user_spec, grpstr);
> +            }
> +
> +            if(gid != grp.gr_gid) {
> +                char **mem;
> +
> +                for(mem = grp.gr_mem; *mem; ++mem) {
> +                    if (!strcmp(*mem, user)) {
> +                        break;
> +                    }
> +                }
> +
> +                if (!*mem) {
> +                    VLOG_FATAL("%s: Invalid --user option %s (user %s is "
> +                               "not in group %s)", pidfile, user_spec,
> +                               user, grpstr);
> +                }
> +                gid = grp.gr_gid;
> +            }
> +        }
> +    }
> +
> +    switch_to_new_user = true;
> +}
> +
>  void
>  daemon_become_new_user(void)
>  {
> diff --git a/lib/daemon.h b/lib/daemon.h
> index fb97cde..4b25f46 100644
> --- a/lib/daemon.h
> +++ b/lib/daemon.h
> @@ -42,14 +42,16 @@
>      OPT_NO_CHDIR,                               \
>      OPT_OVERWRITE_PIDFILE,                      \
>      OPT_PIDFILE,                                \
> -    OPT_MONITOR
> +    OPT_MONITOR,                                \
> +    OPT_USER_GROUP
>
> -#define DAEMON_LONG_OPTIONS                                             \
> -        {"detach",            no_argument, NULL, OPT_DETACH},           \
> -        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},         \
> -        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},    \
> +#define DAEMON_LONG_OPTIONS                                              \
> +        {"detach",            no_argument, NULL, OPT_DETACH},            \
> +        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},          \
> +        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},     \
>          {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, \
> -        {"monitor",           no_argument, NULL, OPT_MONITOR}
> +        {"monitor",           no_argument, NULL, OPT_MONITOR},           \
> +        {"user",              required_argument, NULL, OPT_USER_GROUP}
>
>  #define DAEMON_OPTION_HANDLERS                  \
>          case OPT_DETACH:                        \
> @@ -70,6 +72,10 @@
>                                                  \
>          case OPT_MONITOR:                       \
>              daemon_set_monitor();               \
> +            break;                              \
> +                                                \
> +        case OPT_USER_GROUP:                    \
> +            daemon_set_new_user(optarg);        \
>              break;
>
>  void set_detach(void);
> @@ -77,6 +83,7 @@ void daemon_set_monitor(void);
>  void set_no_chdir(void);
>  void ignore_existing_pidfile(void);
>  void daemon_become_new_user(void);
> +void daemon_set_new_user(const char *);
>  pid_t read_pidfile(const char *name);
>  #else
>  #define DAEMON_OPTION_ENUMS                    \
> @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name);
>      OPT_PIDFILE,                               \
>      OPT_PIPE_HANDLE,                           \
>      OPT_SERVICE,                               \
> -    OPT_SERVICE_MONITOR
> +    OPT_SERVICE_MONITOR                        \
>
>  #define DAEMON_LONG_OPTIONS
>  \
>          {"detach",             no_argument, NULL, OPT_DETACH},
> \
> @@ -120,6 +127,12 @@ void control_handler(DWORD request);
>  void set_pipe_handle(const char *pipe_handle);
>
>  static inline void
> +daemon_set_new_user(const char *)
> +{
> +    /* Not implemented. */
> +}
> +
> +static inline void
>  daemon_become_new_user(void)
>  {
>      /* Not implemented. */
> --
>
> I was able to downgrade ovsdb-server to a non-root user (aatteka in this
case):

aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb
aatteka  20116   985  0 17:14 ?        00:00:00 ovsdb-server: monitoring
pid 20117 (healthy)




aatteka  20117 20116  0 17:14 ?        00:00:00 ovsdb-server /tmp/conf.db
-vconsole:emer -vsyslog:err -vfile:info
--remote=punix:/var/run/openvswitch/db.sock
--private-key=db:Open_vSwitch,SSL,private_key
--certificate=db:Open_vSwitch,SSL,certificate
--bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir
--log-file=/var/log/openvswitch/ovsdb-server.log
--pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor
--user=aatteka


1. However, before being able to do that I had to: sudo chown
aatteka:aatteka /var/run/openvswitch. I think you need to get right
permissions for this directory
2. Should the monitor process still be run as root? There is a precedent
that dnsmasq does that. There are some pros and cons of implementing it
that way if the child process on restart would need to acquire certain
resources.
Andy Zhou Sept. 8, 2015, 8:09 p.m. UTC | #2
On Mon, Sep 7, 2015 at 5:39 PM, Ansis Atteka <ansisatteka@gmail.com> wrote:
>
>
> On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote:
>>
>> Allow daemon running as root to accept --user option, that accepts
>> "user:group" string as input. Performs sanity check on the input,
>> and store the converted uid and gid.
>>
>> daemon_become_new_user() needs to be called to make the actual
>> switch.
>>
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>> ---
>>  lib/daemon-unix.c | 71
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/daemon.h      | 27 +++++++++++++++------
>>  2 files changed, 91 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
>> index d52ac2d..44eb800 100644
>> --- a/lib/daemon-unix.c
>> +++ b/lib/daemon-unix.c
>> @@ -20,6 +20,7 @@
>>  #include <errno.h>
>>  #include <fcntl.h>
>>  #include <grp.h>
>> +#include <pwd.h>
>>  #include <signal.h>
>>  #include <stdlib.h>
>>  #include <string.h>
>> @@ -693,6 +694,76 @@ should_service_stop(void)
>>      return false;
>>  }
>>
>> +void daemon_set_new_user(const char *user_spec)
>> +{
>> +    char *pos = strchr(user_spec, ':');
>> +
>> +    uid = getuid();
>> +    gid = getgid();
>> +
>> +    if (gid || uid) {
>> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
>> +    }
>> +
>> +    user_spec += strspn(user_spec, " \t\r\n");
>> +    int len = pos ? pos - user_spec : strlen(user_spec);
>> +    struct passwd pwd, *res;
>> +    char buf[4096];
>> +
>> +    if (len) {
>> +        user = xzalloc(len + 1);
>> +        strncpy(user, user_spec, len);
>> +
>> +        if (getpwnam_r(user, &pwd, buf, 4096, &res)) {
>
> instead of using 4096 I would use "sizeof buf" here and in other places.
>
> However, from where did you get this "4096" number in the first place? I see
> that in the GETPWNAM man page the documented way to get the expected buffer
> size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason you are
> not using that approach?
Using sysconf() call will be better,  Will fix the next version.
>
>
>>
>> +            VLOG_FATAL("%s: Invalid --user option %s (no such user %s)",
>> +                       pidfile, user_spec, user);
>> +        }
>> +    } else {
>> +        /* User is not specified, use current user.  */
>> +        if (getpwuid_r(uid, &pwd, buf, 4096, &res)) {
>> +            VLOG_FATAL("%s: Invalid --user option %s (failed to lookup "
>> +                       "current user with uid %d)", pidfile, user_spec,
>> uid);
>> +        }
>> +        user = strdup(pwd.pw_name);
>> +    }
>> +
>> +    uid = pwd.pw_uid;
>> +    gid = pwd.pw_gid;
>> +
>> +    if (pos) {
>> +        char *grpstr = pos + 1;
>> +        grpstr += strspn(grpstr, " \t\r\n");
>> +
>> +        if (*grpstr) {
>> +            struct group grp, *res;
>> +
>> +            if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) {
>> +                VLOG_FATAL("%s: Invalid --user option %s (unknown group
>> %s)",
>> +                           pidfile, user_spec, grpstr);
>> +            }
>> +
>> +            if(gid != grp.gr_gid) {
>> +                char **mem;
>> +
>> +                for(mem = grp.gr_mem; *mem; ++mem) {
>> +                    if (!strcmp(*mem, user)) {
>> +                        break;
>> +                    }
>> +                }
>> +
>> +                if (!*mem) {
>> +                    VLOG_FATAL("%s: Invalid --user option %s (user %s is
>> "
>> +                               "not in group %s)", pidfile, user_spec,
>> +                               user, grpstr);
>> +                }
>> +                gid = grp.gr_gid;
>> +            }
>> +        }
>> +    }
>> +
>> +    switch_to_new_user = true;
>> +}
>> +
>>  void
>>  daemon_become_new_user(void)
>>  {
>> diff --git a/lib/daemon.h b/lib/daemon.h
>> index fb97cde..4b25f46 100644
>> --- a/lib/daemon.h
>> +++ b/lib/daemon.h
>> @@ -42,14 +42,16 @@
>>      OPT_NO_CHDIR,                               \
>>      OPT_OVERWRITE_PIDFILE,                      \
>>      OPT_PIDFILE,                                \
>> -    OPT_MONITOR
>> +    OPT_MONITOR,                                \
>> +    OPT_USER_GROUP
>>
>> -#define DAEMON_LONG_OPTIONS                                             \
>> -        {"detach",            no_argument, NULL, OPT_DETACH},           \
>> -        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},         \
>> -        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},    \
>> +#define DAEMON_LONG_OPTIONS
>> \
>> +        {"detach",            no_argument, NULL, OPT_DETACH},
>> \
>> +        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},
>> \
>> +        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},
>> \
>>          {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE},
>> \
>> -        {"monitor",           no_argument, NULL, OPT_MONITOR}
>> +        {"monitor",           no_argument, NULL, OPT_MONITOR},
>> \
>> +        {"user",              required_argument, NULL, OPT_USER_GROUP}
>>
>>  #define DAEMON_OPTION_HANDLERS                  \
>>          case OPT_DETACH:                        \
>> @@ -70,6 +72,10 @@
>>                                                  \
>>          case OPT_MONITOR:                       \
>>              daemon_set_monitor();               \
>> +            break;                              \
>> +                                                \
>> +        case OPT_USER_GROUP:                    \
>> +            daemon_set_new_user(optarg);        \
>>              break;
>>
>>  void set_detach(void);
>> @@ -77,6 +83,7 @@ void daemon_set_monitor(void);
>>  void set_no_chdir(void);
>>  void ignore_existing_pidfile(void);
>>  void daemon_become_new_user(void);
>> +void daemon_set_new_user(const char *);
>>  pid_t read_pidfile(const char *name);
>>  #else
>>  #define DAEMON_OPTION_ENUMS                    \
>> @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name);
>>      OPT_PIDFILE,                               \
>>      OPT_PIPE_HANDLE,                           \
>>      OPT_SERVICE,                               \
>> -    OPT_SERVICE_MONITOR
>> +    OPT_SERVICE_MONITOR                        \
>>
>>  #define DAEMON_LONG_OPTIONS
>> \
>>          {"detach",             no_argument, NULL, OPT_DETACH},
>> \
>> @@ -120,6 +127,12 @@ void control_handler(DWORD request);
>>  void set_pipe_handle(const char *pipe_handle);
>>
>>  static inline void
>> +daemon_set_new_user(const char *)
>> +{
>> +    /* Not implemented. */
>> +}
>> +
>> +static inline void
>>  daemon_become_new_user(void)
>>  {
>>      /* Not implemented. */
>> --
>>
> I was able to downgrade ovsdb-server to a non-root user (aatteka in this
> case):
>
> aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb
> aatteka  20116   985  0 17:14 ?        00:00:00 ovsdb-server: monitoring pid
> 20117 (healthy)
> aatteka  20117 20116  0 17:14 ?        00:00:00 ovsdb-server /tmp/conf.db
> -vconsole:emer -vsyslog:err -vfile:info
> --remote=punix:/var/run/openvswitch/db.sock
> --private-key=db:Open_vSwitch,SSL,private_key
> --certificate=db:Open_vSwitch,SSL,certificate
> --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir
> --log-file=/var/log/openvswitch/ovsdb-server.log
> --pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor
> --user=aatteka
>
Thanks for testing it! I am not user how to write a unit test for this feature.
>
> 1. However, before being able to do that I had to: sudo chown
> aatteka:aatteka /var/run/openvswitch. I think you need to get right
> permissions for this directory
Many Unix systems, such as ubuntu mounts /var/run to tmpfs. So those directories
have to be created with the right user/group and permissions. One possible
solution here is for the system to have an OVS group, and have
/var/run/oopenvswitch owned
by the OVS group with read/write permissions to all OVS groups. We can
expand the discussion
for when we work on the init script changes.

> 2. Should the monitor process still be run as root? There is a precedent
> that dnsmasq does that. There are some pros and cons of implementing it that
> way if the child process on restart would need to acquire certain resources.
>
OVSDB restart does not need to root. I can't think of any benefits of
running the monitoring process
as root, thus it seems better to switch to non-root user as soon as
possible, including the monitoring
process.
Ben Pfaff Sept. 9, 2015, 12:29 a.m. UTC | #3
On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
> Allow daemon running as root to accept --user option, that accepts
> "user:group" string as input. Performs sanity check on the input,
> and store the converted uid and gid.
> 
> daemon_become_new_user() needs to be called to make the actual
> switch.
> 
> Signed-off-by: Andy Zhou <azhou@nicira.com>

Thanks for implementing this.

There should be a new-line after void here:
> +void daemon_set_new_user(const char *user_spec)
> +{

I think the ability to use setuid() and friends only depends on having
uid 0, not gid 0:
> +    uid = getuid();
> +    gid = getgid();
> +
> +    if (gid || uid) {
> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
> +    }

Here in daemon_set_new_user() I would use xmemdup0() instead:
> +    if (len) {
> +        user = xzalloc(len + 1);
> +        strncpy(user, user_spec, len);

and this should be xstrdup():
> +        user = strdup(pwd.pw_name);
> +    }

I think that the parsing in daemon_set_new_user() assumes that white
space, if present, will precede ':'.  If not, then 'len' will end up
negative, which looks bad to me.  I think I'd just not bother worrying
about white space in the parameter.

I am not sure that it is valuable to check that the user belongs to the
specified group.  I don't think that other software tends to perform
such a check; I don't see one in Apache, for example.

I might have other comments when I look at the final patch.
Ben Pfaff Sept. 9, 2015, 12:37 a.m. UTC | #4
On Tue, Sep 08, 2015 at 05:29:24PM -0700, Ben Pfaff wrote:
> On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
> > Allow daemon running as root to accept --user option, that accepts
> > "user:group" string as input. Performs sanity check on the input,
> > and store the converted uid and gid.
> > 
> > daemon_become_new_user() needs to be called to make the actual
> > switch.
> > 
> > Signed-off-by: Andy Zhou <azhou@nicira.com>
>
> I might have other comments when I look at the final patch.

One more concern.  I believe that this series of patches makes all
daemons accept --user, but only ovsdb-server actually implements it and
the others just treat it as a no-op.  I think that this is a bad idea: a
server should only accept --user if it implements it.

Thanks,

Ben.
Ansis Atteka Sept. 9, 2015, 12:42 a.m. UTC | #5
On 8 September 2015 at 13:09, Andy Zhou <azhou@nicira.com> wrote:

> On Mon, Sep 7, 2015 at 5:39 PM, Ansis Atteka <ansisatteka@gmail.com>
> wrote:
> >
> >
> > On 3 September 2015 at 16:33, Andy Zhou <azhou@nicira.com> wrote:
> >>
> >> Allow daemon running as root to accept --user option, that accepts
> >> "user:group" string as input. Performs sanity check on the input,
> >> and store the converted uid and gid.
> >>
> >> daemon_become_new_user() needs to be called to make the actual
> >> switch.
> >>
> >>
> >> Signed-off-by: Andy Zhou <azhou@nicira.com>
> >> ---
> >>  lib/daemon-unix.c | 71
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  lib/daemon.h      | 27 +++++++++++++++------
> >>  2 files changed, 91 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> >> index d52ac2d..44eb800 100644
> >> --- a/lib/daemon-unix.c
> >> +++ b/lib/daemon-unix.c
> >> @@ -20,6 +20,7 @@
> >>  #include <errno.h>
> >>  #include <fcntl.h>
> >>  #include <grp.h>
> >> +#include <pwd.h>
> >>  #include <signal.h>
> >>  #include <stdlib.h>
> >>  #include <string.h>
> >> @@ -693,6 +694,76 @@ should_service_stop(void)
> >>      return false;
> >>  }
> >>
> >> +void daemon_set_new_user(const char *user_spec)
> >> +{
> >> +    char *pos = strchr(user_spec, ':');
> >> +
> >> +    uid = getuid();
> >> +    gid = getgid();
> >> +
> >> +    if (gid || uid) {
> >> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
> >> +    }
> >> +
> >> +    user_spec += strspn(user_spec, " \t\r\n");
> >> +    int len = pos ? pos - user_spec : strlen(user_spec);
> >> +    struct passwd pwd, *res;
> >> +    char buf[4096];
> >> +
> >> +    if (len) {
> >> +        user = xzalloc(len + 1);
> >> +        strncpy(user, user_spec, len);
> >> +
> >> +        if (getpwnam_r(user, &pwd, buf, 4096, &res)) {
> >
> > instead of using 4096 I would use "sizeof buf" here and in other places.
> >
> > However, from where did you get this "4096" number in the first place? I
> see
> > that in the GETPWNAM man page the documented way to get the expected
> buffer
> > size is: sysconf(_SC_GETPW_R_SIZE_MAX); Perhaps there is a reason you are
> > not using that approach?
> Using sysconf() call will be better,  Will fix the next version.
> >
> >
> >>
> >> +            VLOG_FATAL("%s: Invalid --user option %s (no such user
> %s)",
> >> +                       pidfile, user_spec, user);
> >> +        }
> >> +    } else {
> >> +        /* User is not specified, use current user.  */
> >> +        if (getpwuid_r(uid, &pwd, buf, 4096, &res)) {
> >> +            VLOG_FATAL("%s: Invalid --user option %s (failed to lookup
> "
> >> +                       "current user with uid %d)", pidfile, user_spec,
> >> uid);
> >> +        }
> >> +        user = strdup(pwd.pw_name);
> >> +    }
> >> +
> >> +    uid = pwd.pw_uid;
> >> +    gid = pwd.pw_gid;
> >> +
> >> +    if (pos) {
> >> +        char *grpstr = pos + 1;
> >> +        grpstr += strspn(grpstr, " \t\r\n");
> >> +
> >> +        if (*grpstr) {
> >> +            struct group grp, *res;
> >> +
> >> +            if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) {
> >> +                VLOG_FATAL("%s: Invalid --user option %s (unknown group
> >> %s)",
> >> +                           pidfile, user_spec, grpstr);
> >> +            }
> >> +
> >> +            if(gid != grp.gr_gid) {
> >> +                char **mem;
> >> +
> >> +                for(mem = grp.gr_mem; *mem; ++mem) {
> >> +                    if (!strcmp(*mem, user)) {
> >> +                        break;
> >> +                    }
> >> +                }
> >> +
> >> +                if (!*mem) {
> >> +                    VLOG_FATAL("%s: Invalid --user option %s (user %s
> is
> >> "
> >> +                               "not in group %s)", pidfile, user_spec,
> >> +                               user, grpstr);
> >> +                }
> >> +                gid = grp.gr_gid;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    switch_to_new_user = true;
> >> +}
> >> +
> >>  void
> >>  daemon_become_new_user(void)
> >>  {
> >> diff --git a/lib/daemon.h b/lib/daemon.h
> >> index fb97cde..4b25f46 100644
> >> --- a/lib/daemon.h
> >> +++ b/lib/daemon.h
> >> @@ -42,14 +42,16 @@
> >>      OPT_NO_CHDIR,                               \
> >>      OPT_OVERWRITE_PIDFILE,                      \
> >>      OPT_PIDFILE,                                \
> >> -    OPT_MONITOR
> >> +    OPT_MONITOR,                                \
> >> +    OPT_USER_GROUP
> >>
> >> -#define DAEMON_LONG_OPTIONS
>  \
> >> -        {"detach",            no_argument, NULL, OPT_DETACH},
>  \
> >> -        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},
>  \
> >> -        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},
>   \
> >> +#define DAEMON_LONG_OPTIONS
> >> \
> >> +        {"detach",            no_argument, NULL, OPT_DETACH},
> >> \
> >> +        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},
> >> \
> >> +        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},
> >> \
> >>          {"overwrite-pidfile", no_argument, NULL,
> OPT_OVERWRITE_PIDFILE},
> >> \
> >> -        {"monitor",           no_argument, NULL, OPT_MONITOR}
> >> +        {"monitor",           no_argument, NULL, OPT_MONITOR},
> >> \
> >> +        {"user",              required_argument, NULL, OPT_USER_GROUP}
> >>
> >>  #define DAEMON_OPTION_HANDLERS                  \
> >>          case OPT_DETACH:                        \
> >> @@ -70,6 +72,10 @@
> >>                                                  \
> >>          case OPT_MONITOR:                       \
> >>              daemon_set_monitor();               \
> >> +            break;                              \
> >> +                                                \
> >> +        case OPT_USER_GROUP:                    \
> >> +            daemon_set_new_user(optarg);        \
> >>              break;
> >>
> >>  void set_detach(void);
> >> @@ -77,6 +83,7 @@ void daemon_set_monitor(void);
> >>  void set_no_chdir(void);
> >>  void ignore_existing_pidfile(void);
> >>  void daemon_become_new_user(void);
> >> +void daemon_set_new_user(const char *);
> >>  pid_t read_pidfile(const char *name);
> >>  #else
> >>  #define DAEMON_OPTION_ENUMS                    \
> >> @@ -85,7 +92,7 @@ pid_t read_pidfile(const char *name);
> >>      OPT_PIDFILE,                               \
> >>      OPT_PIPE_HANDLE,                           \
> >>      OPT_SERVICE,                               \
> >> -    OPT_SERVICE_MONITOR
> >> +    OPT_SERVICE_MONITOR                        \
> >>
> >>  #define DAEMON_LONG_OPTIONS
> >> \
> >>          {"detach",             no_argument, NULL, OPT_DETACH},
> >> \
> >> @@ -120,6 +127,12 @@ void control_handler(DWORD request);
> >>  void set_pipe_handle(const char *pipe_handle);
> >>
> >>  static inline void
> >> +daemon_set_new_user(const char *)
> >> +{
> >> +    /* Not implemented. */
> >> +}
> >> +
> >> +static inline void
> >>  daemon_become_new_user(void)
> >>  {
> >>      /* Not implemented. */
> >> --
> >>
> > I was able to downgrade ovsdb-server to a non-root user (aatteka in this
> > case):
> >
> > aatteka@aatteka-MacBookPro:~/Git/ovs$ ps -Af | grep ovsdb
> > aatteka  20116   985  0 17:14 ?        00:00:00 ovsdb-server: monitoring
> pid
> > 20117 (healthy)
> > aatteka  20117 20116  0 17:14 ?        00:00:00 ovsdb-server /tmp/conf.db
> > -vconsole:emer -vsyslog:err -vfile:info
> > --remote=punix:/var/run/openvswitch/db.sock
> > --private-key=db:Open_vSwitch,SSL,private_key
> > --certificate=db:Open_vSwitch,SSL,certificate
> > --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert --no-chdir
> > --log-file=/var/log/openvswitch/ovsdb-server.log
> > --pidfile=/var/run/openvswitch/ovsdb-server.pid --detach --monitor
> > --user=aatteka
> >
> Thanks for testing it! I am not user how to write a unit test for this
> feature.
> >
> > 1. However, before being able to do that I had to: sudo chown
> > aatteka:aatteka /var/run/openvswitch. I think you need to get right
> > permissions for this directory
> Many Unix systems, such as ubuntu mounts /var/run to tmpfs. So those
> directories
> have to be created with the right user/group and permissions. One possible
> solution here is for the system to have an OVS group, and have
> /var/run/oopenvswitch owned
> by the OVS group with read/write permissions to all OVS groups. We can
> expand the discussion
> for when we work on the init script changes.
>
> > 2. Should the monitor process still be run as root? There is a precedent
> > that dnsmasq does that. There are some pros and cons of implementing it
> that
> > way if the child process on restart would need to acquire certain
> resources.
> >
> OVSDB restart does not need to root. I can't think of any benefits of
> running the monitoring process
> as root, thus it seems better to switch to non-root user as soon as
> possible, including the monitoring
> process.
>

Sorry, I did not use the right word. Instead of "restart" I meant "crash".
The scenario I thought about is that if a child process died then the
monitor process would have to start the next child process as root so that
next child process would be able to acquire privileged resources if it has
to.

I understand that you don't needs this for ovsdb-server through.
Andy Zhou Sept. 9, 2015, 6:36 a.m. UTC | #6
On Tue, Sep 8, 2015 at 5:37 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Tue, Sep 08, 2015 at 05:29:24PM -0700, Ben Pfaff wrote:
>> On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
>> > Allow daemon running as root to accept --user option, that accepts
>> > "user:group" string as input. Performs sanity check on the input,
>> > and store the converted uid and gid.
>> >
>> > daemon_become_new_user() needs to be called to make the actual
>> > switch.
>> >
>> > Signed-off-by: Andy Zhou <azhou@nicira.com>
>>
>> I might have other comments when I look at the final patch.
>
> One more concern.  I believe that this series of patches makes all
> daemons accept --user, but only ovsdb-server actually implements it and
> the others just treat it as a no-op.  I think that this is a bad idea: a
> server should only accept --user if it implements it.

It seems having all daemons accept --user would be a useful feature in
the long run. OVSDB
happens to be the easiest to add support for since it does not really
root privilege to run.

Sure, I will work on a way to block this option (and map page) for
other daemons.
Andy Zhou Sept. 9, 2015, 7:06 a.m. UTC | #7
On Tue, Sep 8, 2015 at 5:29 PM, Ben Pfaff <blp@nicira.com> wrote:
> On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
>> Allow daemon running as root to accept --user option, that accepts
>> "user:group" string as input. Performs sanity check on the input,
>> and store the converted uid and gid.
>>
>> daemon_become_new_user() needs to be called to make the actual
>> switch.
>>
>> Signed-off-by: Andy Zhou <azhou@nicira.com>
>
> Thanks for implementing this.
>
> There should be a new-line after void here:
>> +void daemon_set_new_user(const char *user_spec)
>> +{
>
oops. Will fix.
> I think the ability to use setuid() and friends only depends on having
> uid 0, not gid 0:
>> +    uid = getuid();
>> +    gid = getgid();
>> +
>> +    if (gid || uid) {
>> +        VLOG_FATAL("%s: only root can use --user option", pidfile);
>> +    }
>
You are right. I will fix this and check for effective uid Instead.

> Here in daemon_set_new_user() I would use xmemdup0() instead:
>> +    if (len) {
>> +        user = xzalloc(len + 1);
>> +        strncpy(user, user_spec, len);
>
> and this should be xstrdup():
>> +        user = strdup(pwd.pw_name);
>> +    }
>
Thanks for pointing out. Will change.
> I think that the parsing in daemon_set_new_user() assumes that white
> space, if present, will precede ':'.  If not, then 'len' will end up
> negative, which looks bad to me.  I think I'd just not bother worrying
> about white space in the parameter.
>
Sure, I can drop the check.  Why would len end up being negative? If pos is set,
meaning : is part of the string, then strspn should not look beyond pos, right?

> I am not sure that it is valuable to check that the user belongs to the
> specified group.  I don't think that other software tends to perform
> such a check; I don't see one in Apache, for example.
>
I got that idea from the "daemon" program. Would it otherwise be a security risk
of creating an illegal combination of user/group?

> I might have other comments when I look at the final patch.
Ben Pfaff Sept. 9, 2015, 3:50 p.m. UTC | #8
On Wed, Sep 09, 2015 at 12:06:56AM -0700, Andy Zhou wrote:
> On Tue, Sep 8, 2015 at 5:29 PM, Ben Pfaff <blp@nicira.com> wrote:
> > On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
> >> Allow daemon running as root to accept --user option, that accepts
> >> "user:group" string as input. Performs sanity check on the input,
> >> and store the converted uid and gid.
> >>
> >> daemon_become_new_user() needs to be called to make the actual
> >> switch.
> >>
> >> Signed-off-by: Andy Zhou <azhou@nicira.com>
>
> > I think that the parsing in daemon_set_new_user() assumes that white
> > space, if present, will precede ':'.  If not, then 'len' will end up
> > negative, which looks bad to me.  I think I'd just not bother worrying
> > about white space in the parameter.
> >
> Sure, I can drop the check.  Why would len end up being negative? If
> pos is set, meaning : is part of the string, then strspn should not
> look beyond pos, right?

Oops, you're right, sorry.

> > I am not sure that it is valuable to check that the user belongs to the
> > specified group.  I don't think that other software tends to perform
> > such a check; I don't see one in Apache, for example.
> >
> I got that idea from the "daemon" program. Would it otherwise be a
> security risk of creating an illegal combination of user/group?

I didn't know there was precedent for such a check; I hadn't ever seen
one before.  Leave it in, then, and we can reconsider if it happens to
cause trouble for someone.

Patch
diff mbox

diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index d52ac2d..44eb800 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -20,6 +20,7 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <grp.h>
+#include <pwd.h>
 #include <signal.h>
 #include <stdlib.h>
 #include <string.h>
@@ -693,6 +694,76 @@  should_service_stop(void)
     return false;
 }
 
+void daemon_set_new_user(const char *user_spec)
+{
+    char *pos = strchr(user_spec, ':');
+
+    uid = getuid();
+    gid = getgid();
+
+    if (gid || uid) {
+        VLOG_FATAL("%s: only root can use --user option", pidfile);
+    }
+
+    user_spec += strspn(user_spec, " \t\r\n");
+    int len = pos ? pos - user_spec : strlen(user_spec);
+    struct passwd pwd, *res;
+    char buf[4096];
+
+    if (len) {
+        user = xzalloc(len + 1);
+        strncpy(user, user_spec, len);
+
+        if (getpwnam_r(user, &pwd, buf, 4096, &res)) {
+            VLOG_FATAL("%s: Invalid --user option %s (no such user %s)",
+                       pidfile, user_spec, user);
+        }
+    } else {
+        /* User is not specified, use current user.  */
+        if (getpwuid_r(uid, &pwd, buf, 4096, &res)) {
+            VLOG_FATAL("%s: Invalid --user option %s (failed to lookup "
+                       "current user with uid %d)", pidfile, user_spec, uid);
+        }
+        user = strdup(pwd.pw_name);
+    }
+
+    uid = pwd.pw_uid;
+    gid = pwd.pw_gid;
+
+    if (pos) {
+        char *grpstr = pos + 1;
+        grpstr += strspn(grpstr, " \t\r\n");
+
+        if (*grpstr) {
+            struct group grp, *res;
+
+            if(getgrnam_r(grpstr, &grp, buf, 4096, &res)) {
+                VLOG_FATAL("%s: Invalid --user option %s (unknown group %s)",
+                           pidfile, user_spec, grpstr);
+            }
+
+            if(gid != grp.gr_gid) {
+                char **mem;
+
+                for(mem = grp.gr_mem; *mem; ++mem) {
+                    if (!strcmp(*mem, user)) {
+                        break;
+                    }
+                }
+
+                if (!*mem) {
+                    VLOG_FATAL("%s: Invalid --user option %s (user %s is "
+                               "not in group %s)", pidfile, user_spec,
+                               user, grpstr);
+                }
+                gid = grp.gr_gid;
+            }
+        }
+    }
+
+    switch_to_new_user = true;
+}
+
 void
 daemon_become_new_user(void)
 {
diff --git a/lib/daemon.h b/lib/daemon.h
index fb97cde..4b25f46 100644
--- a/lib/daemon.h
+++ b/lib/daemon.h
@@ -42,14 +42,16 @@ 
     OPT_NO_CHDIR,                               \
     OPT_OVERWRITE_PIDFILE,                      \
     OPT_PIDFILE,                                \
-    OPT_MONITOR
+    OPT_MONITOR,                                \
+    OPT_USER_GROUP
 
-#define DAEMON_LONG_OPTIONS                                             \
-        {"detach",            no_argument, NULL, OPT_DETACH},           \
-        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},         \
-        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},    \
+#define DAEMON_LONG_OPTIONS                                              \
+        {"detach",            no_argument, NULL, OPT_DETACH},            \
+        {"no-chdir",          no_argument, NULL, OPT_NO_CHDIR},          \
+        {"pidfile",           optional_argument, NULL, OPT_PIDFILE},     \
         {"overwrite-pidfile", no_argument, NULL, OPT_OVERWRITE_PIDFILE}, \
-        {"monitor",           no_argument, NULL, OPT_MONITOR}
+        {"monitor",           no_argument, NULL, OPT_MONITOR},           \
+        {"user",              required_argument, NULL, OPT_USER_GROUP}
 
 #define DAEMON_OPTION_HANDLERS                  \
         case OPT_DETACH:                        \
@@ -70,6 +72,10 @@ 
                                                 \
         case OPT_MONITOR:                       \
             daemon_set_monitor();               \
+            break;                              \
+                                                \
+        case OPT_USER_GROUP:                    \
+            daemon_set_new_user(optarg);        \
             break;
 
 void set_detach(void);
@@ -77,6 +83,7 @@  void daemon_set_monitor(void);
 void set_no_chdir(void);
 void ignore_existing_pidfile(void);
 void daemon_become_new_user(void);
+void daemon_set_new_user(const char *);
 pid_t read_pidfile(const char *name);
 #else
 #define DAEMON_OPTION_ENUMS                    \
@@ -85,7 +92,7 @@  pid_t read_pidfile(const char *name);
     OPT_PIDFILE,                               \
     OPT_PIPE_HANDLE,                           \
     OPT_SERVICE,                               \
-    OPT_SERVICE_MONITOR
+    OPT_SERVICE_MONITOR                        \
 
 #define DAEMON_LONG_OPTIONS                                               \
         {"detach",             no_argument, NULL, OPT_DETACH},            \
@@ -120,6 +127,12 @@  void control_handler(DWORD request);
 void set_pipe_handle(const char *pipe_handle);
 
 static inline void
+daemon_set_new_user(const char *)
+{
+    /* Not implemented. */
+}
+
+static inline void
 daemon_become_new_user(void)
 { 
     /* Not implemented. */