[ovs-dev,v3,03/10] lib/daemon: add option to retain CAP_NET_ADMIN capability

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(-)

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.