| Message ID | 1442271254-27897-2-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Sep 14, 2015 at 3:54 PM, Andy Zhou <azhou@nicira.com> wrote: > Added functions to drop daemon's root privileges at run time by > allowing it to run as a different user. Daemons all start > running as root, then drop to the user by invoking > daemon_become_new_user() function when they are ready to drop > root privileges. > > Future patch will make use of this function. > > Signed-off-by: Andy Zhou <azhou@nicira.com> > > --- > v2 : fix typos and white spaces > v3 : switching to user getresuid/gid for better portability. > --- > lib/daemon-unix.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- > lib/daemon.h | 10 +++++- > 2 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index eb95521..f175037 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -19,6 +19,7 @@ > #include "daemon-private.h" > #include <errno.h> > #include <fcntl.h> > +#include <grp.h> > #include <signal.h> > #include <stdlib.h> > #include <string.h> > @@ -64,6 +65,13 @@ static int daemonize_fd = -1; > * it dies due to an error signal? */ > static bool monitor; > > +/* --user: Only root can use this option. Switch to new uid:gid after > + * initially running as root. */ > +static bool switch_to_new_user = false; > +static uid_t uid; > +static gid_t gid; > +static char *user = NULL; > + > static void check_already_running(void); > static int lock_pidfile(FILE *, int command); > static pid_t fork_and_clean_up(void); > @@ -684,3 +692,89 @@ should_service_stop(void) > { > return false; > } > + > + > +static bool > +gid_equal(const gid_t expected, const gid_t value) Really minor since I am not sure if there are conventions that we have to follow, but this is not a true _equal function because gid_equal(a,b) != git_equal(b,a) if a == -1 and b ==1 I would: 1. rename it to something else, like gid_matches(); OR 2. make a function comment explaining this behavior. Feel free to leave it as if you don't think this is suggestion should apply. > +{ > + return (expected == -1) ? true : expected == value; > +} > + > +static bool > +gid_verify(const gid_t real, const gid_t effecitve, const gid_t saved) > +{ > + gid_t r, e, s; > + > + getresgid(&r, &e, &s); > + return (gid_equal(real, r) && > + gid_equal(effecitve, e) && gid_equal(saved, s)); > +} > + > +static void > +daemon_switch_group(const gid_t real, const gid_t effective, > + const gid_t saved) > +{ > + if ((setresgid(real, effective, saved) == -1) || > + !gid_verify(real, effective, saved)) { > + VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", > + pidfile, gid); s/fail/failed > + } > +} > + > +static bool > +uid_equal(const uid_t expected, const uid_t value) > +{ > + return (expected == -1) ? true : expected == value; > +} > + > +static bool > +uid_verify(const uid_t real, const uid_t effective, const uid_t saved) > +{ > + uid_t r, e, s; > + > + getresuid(&r, &e, &s); Would you mind to print getresuid() errno value in case of failure? > + return (uid_equal(real, r) && > + uid_equal(effective, e) && uid_equal(saved, s)); > +} > + > +static void > +daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved, > + const char *user) > +{ > + if ((setresuid(real, effective, saved) == -1) || > + !uid_verify(real, effective, saved)) { > + VLOG_FATAL("%s: fail to switch user to %s, aborting", > + pidfile, user); s/fail/failed Would you mind to pint setresuid() errno value in case of failure? > + } > +} > + > +void > +daemon_become_new_user(void) > +{ > + /* "Setuid Demystified" by Hao Chen, etc outlines some caveats of > + * around unix system call setuid() and friends. This implementation > + * mostly follow the advice given by the paper. The paper is > + * published in 2002, so things could have changed. */ > + > + /* Change both real and effective uid and gid will permanently > + * drop the process' privilege. "Setuid Demystified" suggested > + * that calling getuid() after each setuid() call to verify they > + * are actually set, because checking return code alone is not > + * sufficient. > + * > + * Linux also has per process file system uid, i.e. fsuid. Without > + * explicitly setting it, it follows the process' effective uid. > + * This implementation does not explicitly set fsuid for better > + * portability. (Although setresuid() is not available on Solaris, > + * according to the paper above.) */ > + > + if (switch_to_new_user) { > + daemon_switch_group(gid, gid, gid); > + > + if (user && initgroups(user, gid) == -1) { > + VLOG_FATAL("%s: fail to add supplementary group gid %d, aborting", > + pidfile, gid); Would you mind to print initgroups() errno as well instead of simply aborting? > + } > + daemon_switch_user(uid, uid, uid, user); > + } > +} > diff --git a/lib/daemon.h b/lib/daemon.h > index 959341d..fdd7b6a 100644 > --- a/lib/daemon.h > +++ b/lib/daemon.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -76,6 +76,7 @@ void set_detach(void); > void daemon_set_monitor(void); > void set_no_chdir(void); > void ignore_existing_pidfile(void); > +void daemon_become_new_user(void); > pid_t read_pidfile(const char *name); > #else > #define DAEMON_OPTION_ENUMS \ > @@ -117,6 +118,13 @@ pid_t read_pidfile(const char *name); > > void control_handler(DWORD request); > void set_pipe_handle(const char *pipe_handle); > + > +static inline void > +daemon_become_new_user(void) > +{ > + /* Not implemented. */ I think Ben had a comment that on Windows we should not silently ignore such errors. However, let me review rest of the patches. > +} > + > #endif /* _WIN32 */ > > bool get_detach(void); > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Mon, Sep 14, 2015 at 03:54:05PM -0700, Andy Zhou wrote: > Added functions to drop daemon's root privileges at run time by > allowing it to run as a different user. Daemons all start > running as root, then drop to the user by invoking > daemon_become_new_user() function when they are ready to drop > root privileges. > > Future patch will make use of this function. > > Signed-off-by: Andy Zhou <azhou@nicira.com> Thanks for working on this. I'm glad to see some progress in basic security. Many of these functions mark non-pointer parameters as "const". That's unusual and not very useful; it doesn't promise the caller anything in the same way that, say, declaring a parameter as type "const char *" promises that the function won't modify the caller's string. > +static bool > +gid_equal(const gid_t expected, const gid_t value) > +{ Usually I'd write this: > + return (expected == -1) ? true : expected == value; as just: return expected == -1 || expected == value; > +} s/effecitve/effective/ in the following function: > +static bool > +gid_verify(const gid_t real, const gid_t effecitve, const gid_t saved) > +{ > + gid_t r, e, s; Here, I'd check the return value of getresgid() and abort if it fails (or I guess you could just return false): > + getresgid(&r, &e, &s); > + return (gid_equal(real, r) && > + gid_equal(effecitve, e) && gid_equal(saved, s)); > +} > + > +static void > +daemon_switch_group(const gid_t real, const gid_t effective, > + const gid_t saved) > +{ > + if ((setresgid(real, effective, saved) == -1) || > + !gid_verify(real, effective, saved)) { > + VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", > + pidfile, gid); > + } > +} > + > +static bool > +uid_equal(const uid_t expected, const uid_t value) > +{ I'd usually write this with || as for gid_equal: > + return (expected == -1) ? true : expected == value; > +} > + > +static bool > +uid_verify(const uid_t real, const uid_t effective, const uid_t saved) > +{ > + uid_t r, e, s; Return value check here too: > + getresuid(&r, &e, &s); > + return (uid_equal(real, r) && > + uid_equal(effective, e) && uid_equal(saved, s)); > +} > + > +static void > +daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved, > + const char *user) > +{ > + if ((setresuid(real, effective, saved) == -1) || > + !uid_verify(real, effective, saved)) { > + VLOG_FATAL("%s: fail to switch user to %s, aborting", > + pidfile, user); > + } > +}
On Thu, Sep 17, 2015 at 10:05:22PM -0700, Ansis Atteka wrote: > > +static bool > > +gid_equal(const gid_t expected, const gid_t value) > > Really minor since I am not sure if there are conventions that we have > to follow, but this is not a true _equal function because > gid_equal(a,b) != git_equal(b,a) if a == -1 and b ==1 > > I would: > 1. rename it to something else, like gid_matches(); OR > 2. make a function comment explaining this behavior. I especially like suggestion #1.
On Thu, Sep 17, 2015 at 10:05:22PM -0700, Ansis Atteka wrote: > > +static inline void > > +daemon_become_new_user(void) > > +{ > > + /* Not implemented. */ > I think Ben had a comment that on Windows we should not silently > ignore such errors. However, let me review rest of the patches. I think my comment was at least partly wrong, because this function will get called unconditionally, whether switching to a new user has been requested or not. So let me modify my previous comment to say that, if the user requests switching to a new user, then on Windows OVS should either not recognize the option at all or fail in daemon_become_new_user(). I guess the latter is probably more user-friendly because the former would just leave the user puzzled about why the option was in the manual but not implemented.
On Fri, Sep 18, 2015 at 12:17 PM, Ben Pfaff <blp@nicira.com> wrote: > On Thu, Sep 17, 2015 at 10:05:22PM -0700, Ansis Atteka wrote: >> > +static inline void >> > +daemon_become_new_user(void) >> > +{ >> > + /* Not implemented. */ >> I think Ben had a comment that on Windows we should not silently >> ignore such errors. However, let me review rest of the patches. > > I think my comment was at least partly wrong, because this function will > get called unconditionally, whether switching to a new user has been > requested or not. So let me modify my previous comment to say that, if > the user requests switching to a new user, then on Windows OVS should > either not recognize the option at all or fail in > daemon_become_new_user(). I guess the latter is probably more > user-friendly because the former would just leave the user puzzled about > why the option was in the manual but not implemented. I agree, Fail on Windows is more user-friendly unless we can conditionally include man page sections. It can also serve as place holder for future enhancements.
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index eb95521..f175037 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,6 +19,7 @@ #include "daemon-private.h" #include <errno.h> #include <fcntl.h> +#include <grp.h> #include <signal.h> #include <stdlib.h> #include <string.h> @@ -64,6 +65,13 @@ static int daemonize_fd = -1; * it dies due to an error signal? */ static bool monitor; +/* --user: Only root can use this option. Switch to new uid:gid after + * initially running as root. */ +static bool switch_to_new_user = false; +static uid_t uid; +static gid_t gid; +static char *user = NULL; + static void check_already_running(void); static int lock_pidfile(FILE *, int command); static pid_t fork_and_clean_up(void); @@ -684,3 +692,89 @@ should_service_stop(void) { return false; } + + +static bool +gid_equal(const gid_t expected, const gid_t value) +{ + return (expected == -1) ? true : expected == value; +} + +static bool +gid_verify(const gid_t real, const gid_t effecitve, const gid_t saved) +{ + gid_t r, e, s; + + getresgid(&r, &e, &s); + return (gid_equal(real, r) && + gid_equal(effecitve, e) && gid_equal(saved, s)); +} + +static void +daemon_switch_group(const gid_t real, const gid_t effective, + const gid_t saved) +{ + if ((setresgid(real, effective, saved) == -1) || + !gid_verify(real, effective, saved)) { + VLOG_FATAL("%s: fail to switch group to gid as %d, aborting", + pidfile, gid); + } +} + +static bool +uid_equal(const uid_t expected, const uid_t value) +{ + return (expected == -1) ? true : expected == value; +} + +static bool +uid_verify(const uid_t real, const uid_t effective, const uid_t saved) +{ + uid_t r, e, s; + + getresuid(&r, &e, &s); + return (uid_equal(real, r) && + uid_equal(effective, e) && uid_equal(saved, s)); +} + +static void +daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved, + const char *user) +{ + if ((setresuid(real, effective, saved) == -1) || + !uid_verify(real, effective, saved)) { + VLOG_FATAL("%s: fail to switch user to %s, aborting", + pidfile, user); + } +} + +void +daemon_become_new_user(void) +{ + /* "Setuid Demystified" by Hao Chen, etc outlines some caveats of + * around unix system call setuid() and friends. This implementation + * mostly follow the advice given by the paper. The paper is + * published in 2002, so things could have changed. */ + + /* Change both real and effective uid and gid will permanently + * drop the process' privilege. "Setuid Demystified" suggested + * that calling getuid() after each setuid() call to verify they + * are actually set, because checking return code alone is not + * sufficient. + * + * Linux also has per process file system uid, i.e. fsuid. Without + * explicitly setting it, it follows the process' effective uid. + * This implementation does not explicitly set fsuid for better + * portability. (Although setresuid() is not available on Solaris, + * according to the paper above.) */ + + if (switch_to_new_user) { + daemon_switch_group(gid, gid, gid); + + if (user && initgroups(user, gid) == -1) { + VLOG_FATAL("%s: fail to add supplementary group gid %d, aborting", + pidfile, gid); + } + daemon_switch_user(uid, uid, uid, user); + } +} diff --git a/lib/daemon.h b/lib/daemon.h index 959341d..fdd7b6a 100644 --- a/lib/daemon.h +++ b/lib/daemon.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -76,6 +76,7 @@ void set_detach(void); void daemon_set_monitor(void); void set_no_chdir(void); void ignore_existing_pidfile(void); +void daemon_become_new_user(void); pid_t read_pidfile(const char *name); #else #define DAEMON_OPTION_ENUMS \ @@ -117,6 +118,13 @@ pid_t read_pidfile(const char *name); void control_handler(DWORD request); void set_pipe_handle(const char *pipe_handle); + +static inline void +daemon_become_new_user(void) +{ + /* Not implemented. */ +} + #endif /* _WIN32 */ bool get_detach(void);
Added functions to drop daemon's root privileges at run time by allowing it to run as a different user. Daemons all start running as root, then drop to the user by invoking daemon_become_new_user() function when they are ready to drop root privileges. Future patch will make use of this function. Signed-off-by: Andy Zhou <azhou@nicira.com> --- v2 : fix typos and white spaces v3 : switching to user getresuid/gid for better portability. --- lib/daemon-unix.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- lib/daemon.h | 10 +++++- 2 files changed, 104 insertions(+), 2 deletions(-)