Message ID | 1442271254-27897-4-git-send-email-azhou@nicira.com |
---|---|
State | Changes Requested |
Headers | show |
As we discussed off-line, Ben sent out a patch last year to drop most capabilities that was never merged: http://openvswitch.org/pipermail/dev/2014-July/042993.html It may be worth looking at for comparison's sake. There was a follow-up from Flavio that may be worth reading, too. --Justin > On Sep 14, 2015, at 3:54 PM, Andy Zhou <azhou@nicira.com> wrote: > > This patch adds an argument to daemon_become_new_user() API for the > caller specify whether the capability of accessing the kernel datapath > is needed. On Linux, daemons access the kernel datapath need to > retain some root privileges, such as CAP_NET_ADMIN. > > Current implementation (of retaining CAP_NET_ADMIN) requires libcap-ng. > This implementation only covers the Linux. > Other Unix platforms currently do not support kernel based datapath. > (but supports --user options for all daemons.) > On Windows, daemon_become_new_user() is a stub function that does > nothing. > > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- > lib/daemon-unix.c | 38 +++++++++++++++++++++++++++++++++++--- > lib/daemon.h | 5 ++--- > 2 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index f175037..f361165 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -27,6 +27,9 @@ > #include <sys/wait.h> > #include <sys/stat.h> > #include <unistd.h> > +#if HAVE_LIBCAPNG > +#include <cap-ng.h> > +#endif > #include "command-line.h" > #include "fatal-signal.h" > #include "dirs.h" > @@ -748,8 +751,28 @@ daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved, > } > } > > +static void > +daemon_become_new_user_with_datapath_capability(void) > +{ > +#if HAVE_LIBCAPNG > + if (capng_have_capabilities(CAPNG_SELECT_CAPS) > CAPNG_NONE) { > + capng_clear(CAPNG_SELECT_BOTH); > + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, > + CAP_NET_ADMIN); > + if (capng_change_id(uid, gid, > + CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING)) { > + VLOG_FATAL("%s: libcap-ng fail to switch to user and group " > + "%d:%d, aborting", pidfile, uid, gid); > + } > + } > +#else > + VLOG_FATAL("%s: fail to downgrade user using libcap-ng. (libcap-ng is " > + "not configured at compile time.) aborting", pidfile); > +#endif > +} > + > void > -daemon_become_new_user(void) > +daemon_become_new_user(bool access_kernel_datapath) > { > /* "Setuid Demystified" by Hao Chen, etc outlines some caveats of > * around unix system call setuid() and friends. This implementation > @@ -769,11 +792,20 @@ daemon_become_new_user(void) > * according to the paper above.) */ > > if (switch_to_new_user) { > + > + if (access_kernel_datapath) { > + daemon_become_new_user_with_datapath_capability(); > + return; > + } > + > + /* Using more portable APIs to switch uid:gid, when datapath > + * access is not required. On Linux systems, all capabilities > + * will be dropped. */ > 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); > + 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 fdd7b6a..f98ef16 100644 > --- a/lib/daemon.h > +++ b/lib/daemon.h > @@ -76,7 +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); > +void daemon_become_new_user(bool access_kernel_datapath); > pid_t read_pidfile(const char *name); > #else > #define DAEMON_OPTION_ENUMS \ > @@ -120,11 +120,10 @@ void control_handler(DWORD request); > void set_pipe_handle(const char *pipe_handle); > > static inline void > -daemon_become_new_user(void) > +daemon_become_new_user(bool access_kernel_datapath OVS_UNUSED) > { > /* Not implemented. */ > } > - > #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:07PM -0700, Andy Zhou wrote: > This patch adds an argument to daemon_become_new_user() API for the > caller specify whether the capability of accessing the kernel datapath > is needed. On Linux, daemons access the kernel datapath need to > retain some root privileges, such as CAP_NET_ADMIN. > > Current implementation (of retaining CAP_NET_ADMIN) requires libcap-ng. > This implementation only covers the Linux. > Other Unix platforms currently do not support kernel based datapath. > (but supports --user options for all daemons.) > On Windows, daemon_become_new_user() is a stub function that does > nothing. > > Signed-off-by: Andy Zhou <azhou@nicira.com> The manpage for capng_have_capabilities() says: The capabilities sets must be previ ously setup with calls to capng_get_caps_process, capng_get_caps_fd, or in some other way setup. but I don't see anything doing that before the call. The capng_change_id() call seems like it should include CAPNG_INIT_SUPP_GRP so that groups from the new uid get included, for consistency with the non-capng based version. With this patch, in the case where libcap-ng is available and capng_have_capabilities() returns false, daemon_become_new_user() does nothing at all. That means, as far as I can tell, that it silently fails in that case to change uid and gid to what was requested. I'm concerned that there are, after this patch, two different ways to switch to a new uid and gid on the same system, one of them used by some daemons and the other by other daemons, and that in some cases the method used by some daemons just won't be supported and will abort. That kind of complexity is going to cause confusion and in a security context that means it will cause security holes. What can we do to reduce the complexity? My suggestion is that we should always use libcap-ng in all cases on Linux. Then it's less nuanced and easier to explain and I think that it's more likely to be used correctly in practice. Thanks, Ben.
On Fri, Sep 18, 2015 at 12:40 PM, Ben Pfaff <blp@nicira.com> wrote: > On Mon, Sep 14, 2015 at 03:54:07PM -0700, Andy Zhou wrote: >> This patch adds an argument to daemon_become_new_user() API for the >> caller specify whether the capability of accessing the kernel datapath >> is needed. On Linux, daemons access the kernel datapath need to >> retain some root privileges, such as CAP_NET_ADMIN. >> >> Current implementation (of retaining CAP_NET_ADMIN) requires libcap-ng. >> This implementation only covers the Linux. >> Other Unix platforms currently do not support kernel based datapath. >> (but supports --user options for all daemons.) >> On Windows, daemon_become_new_user() is a stub function that does >> nothing. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > The manpage for capng_have_capabilities() says: > > The capabilities sets must be previ ously setup with calls to > capng_get_caps_process, capng_get_caps_fd, or in some other way > setup. > > but I don't see anything doing that before the call. I missed this. It should have been called. > > The capng_change_id() call seems like it should include > CAPNG_INIT_SUPP_GRP so that groups from the new uid get included, for > consistency with the non-capng based version. Good catch. > > With this patch, in the case where libcap-ng is available and > capng_have_capabilities() returns false, daemon_become_new_user() does > nothing at all. That means, as far as I can tell, that it silently > fails in that case to change uid and gid to what was requested. > It should fail. I will fix it. > I'm concerned that there are, after this patch, two different ways to > switch to a new uid and gid on the same system, one of them used by some > daemons and the other by other daemons, and that in some cases the > method used by some daemons just won't be supported and will abort. > That kind of complexity is going to cause confusion and in a security > context that means it will cause security holes. What can we do to > reduce the complexity? My suggestion is that we should always use > libcap-ng in all cases on Linux. Then it's less nuanced and easier to > explain and I think that it's more likely to be used correctly in > practice. Sure, make sense. I will use libcap-ng on Linux, setresuid() for other Unix platform Windows platform should not accept the --user option, at least not until it is supported on that platform. > > Thanks, > > Ben.
On Fri, Sep 18, 2015 at 03:41:10PM -0700, Andy Zhou wrote: > On Fri, Sep 18, 2015 at 12:40 PM, Ben Pfaff <blp@nicira.com> wrote: > > I'm concerned that there are, after this patch, two different ways to > > switch to a new uid and gid on the same system, one of them used by some > > daemons and the other by other daemons, and that in some cases the > > method used by some daemons just won't be supported and will abort. > > That kind of complexity is going to cause confusion and in a security > > context that means it will cause security holes. What can we do to > > reduce the complexity? My suggestion is that we should always use > > libcap-ng in all cases on Linux. Then it's less nuanced and easier to > > explain and I think that it's more likely to be used correctly in > > practice. > > Sure, make sense. I will use libcap-ng on Linux, setresuid() for > other Unix platform > Windows platform should not accept the --user option, at least not > until it is supported > on that platform. Perfect! Thank you.
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index f175037..f361165 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -27,6 +27,9 @@ #include <sys/wait.h> #include <sys/stat.h> #include <unistd.h> +#if HAVE_LIBCAPNG +#include <cap-ng.h> +#endif #include "command-line.h" #include "fatal-signal.h" #include "dirs.h" @@ -748,8 +751,28 @@ daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved, } } +static void +daemon_become_new_user_with_datapath_capability(void) +{ +#if HAVE_LIBCAPNG + if (capng_have_capabilities(CAPNG_SELECT_CAPS) > CAPNG_NONE) { + capng_clear(CAPNG_SELECT_BOTH); + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, + CAP_NET_ADMIN); + if (capng_change_id(uid, gid, + CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING)) { + VLOG_FATAL("%s: libcap-ng fail to switch to user and group " + "%d:%d, aborting", pidfile, uid, gid); + } + } +#else + VLOG_FATAL("%s: fail to downgrade user using libcap-ng. (libcap-ng is " + "not configured at compile time.) aborting", pidfile); +#endif +} + void -daemon_become_new_user(void) +daemon_become_new_user(bool access_kernel_datapath) { /* "Setuid Demystified" by Hao Chen, etc outlines some caveats of * around unix system call setuid() and friends. This implementation @@ -769,11 +792,20 @@ daemon_become_new_user(void) * according to the paper above.) */ if (switch_to_new_user) { + + if (access_kernel_datapath) { + daemon_become_new_user_with_datapath_capability(); + return; + } + + /* Using more portable APIs to switch uid:gid, when datapath + * access is not required. On Linux systems, all capabilities + * will be dropped. */ 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); + 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 fdd7b6a..f98ef16 100644 --- a/lib/daemon.h +++ b/lib/daemon.h @@ -76,7 +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); +void daemon_become_new_user(bool access_kernel_datapath); pid_t read_pidfile(const char *name); #else #define DAEMON_OPTION_ENUMS \ @@ -120,11 +120,10 @@ void control_handler(DWORD request); void set_pipe_handle(const char *pipe_handle); static inline void -daemon_become_new_user(void) +daemon_become_new_user(bool access_kernel_datapath OVS_UNUSED) { /* Not implemented. */ } - #endif /* _WIN32 */ bool get_detach(void);
This patch adds an argument to daemon_become_new_user() API for the caller specify whether the capability of accessing the kernel datapath is needed. On Linux, daemons access the kernel datapath need to retain some root privileges, such as CAP_NET_ADMIN. Current implementation (of retaining CAP_NET_ADMIN) requires libcap-ng. This implementation only covers the Linux. Other Unix platforms currently do not support kernel based datapath. (but supports --user options for all daemons.) On Windows, daemon_become_new_user() is a stub function that does nothing. Signed-off-by: Andy Zhou <azhou@nicira.com> --- lib/daemon-unix.c | 38 +++++++++++++++++++++++++++++++++++--- lib/daemon.h | 5 ++--- 2 files changed, 37 insertions(+), 6 deletions(-)