On Mon, Sep 14, 2015 at 3:54 PM, Andy Zhou <[email protected]> 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 <[email protected]>
>
> ---
> 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
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
