| Message ID | 20220329192934.1415-1-timo@rothenpieler.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Retain CAP_NET_ADMIN when dropping privileges | expand |
On 29.03.2022 21:29, Timo Rothenpieler wrote: > +static bool > +do_keep_caps(bool prepare) > +{ > + struct __user_cap_header_struct cap_hdr = { _LINUX_CAPABILITY_VERSION_3 }; > + struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3] = {}; > + > + if (syscall(SYS_capget, &cap_hdr, cap_data) < 0) > + { > + msg(M_NONFATAL | M_ERRNO, "failed getting capabilities"); > + return false; > + } > + > + if (prepare) > + { > + SET_CAP_HELPER(cap_data, permitted, CAP_NET_ADMIN); > + } > + else > + { > + SET_CAP_HELPER(cap_data, effective, CAP_NET_ADMIN); This is missing something like the following: > /* Clamp permitted capabilities to effective ones. > * Without doing this, the process can give itself root-like caps at any time. */ > for (int i = 0; i < sizeof(cap_data)/sizeof(cap_data[0]); i++) > { > cap_data[i].permitted = cap_data[i].effective; > } Without that, the permitted caps stay the full set of root caps, and the process can make them effective at any time. Patch on GitHub is updated with that. > + } > + > + if (syscall(SYS_capset, &cap_hdr, cap_data) < 0) > + { > + msg(M_NONFATAL | M_ERRNO, "failed setting %s capabilities", prepare ? "permitted" : "effective"); > + return false; > + } > + > + if (prepare && prctl(PR_SET_KEEPCAPS, 1) < 0) > + { > + msg(M_NONFATAL | M_ERRNO, "failed setting keepcaps"); > + return false; > + } > + > + return true; > +}
On 29/03/2022 21:29, Timo Rothenpieler wrote: > --- > This patch sits on top of the current dco branch, and will not apply to > latest master. > > It solves the issue of dropping root privileges breaking dco and sitnl > due to missing NET_ADMIN capabilities. > > > configure.ac | 3 ++ > src/openvpn/init.c | 22 +++++++++++++- > src/openvpn/platform.c | 65 +++++++++++++++++++++++++++++++++++++++++- > src/openvpn/platform.h | 2 +- > 4 files changed, 89 insertions(+), 3 deletions(-) > Thanks a lot! I've quickly looked through the code, and I have to NAK this approach: > +#ifdef HAVE_LINUX_CAPABILITIES > +#define SET_CAP_HELPER(data, set, cap) data[(cap)>>5].set |= 1<<((cap)&31) > + > +static bool > +do_keep_caps(bool prepare) > +{ > + struct __user_cap_header_struct cap_hdr = { _LINUX_CAPABILITY_VERSION_3 }; > + struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3] = {}; > + > + if (syscall(SYS_capget, &cap_hdr, cap_data) < 0) We should really use libcap or libcap-ng and not avoid using syscalls directly. I have used libcap-ng in openvpn3-linux, both for preserving capabilities and dropping root. It does all the right steps fairly easily. The configure.ac detection, which for OpenVPN 2.x can be restricted when DCO is going to be built into openvpn: <https://github.com/OpenVPN/openvpn3-linux/blob/master/configure.ac#L113> The code for preserving capabilities: <https://github.com/OpenVPN/openvpn3-linux/blob/c40218df43c8e652fedfa70304eae797b305e780/src/netcfg/openvpn3-service-netcfg.cpp#L82> And the code for dropping root, ensuring the capabilities are restricted properly: <https://github.com/OpenVPN/openvpn3-linux/blob/c40218df43c8e652fedfa70304eae797b305e780/src/netcfg/openvpn3-service-netcfg.cpp#L64>
On 30/03/2022 10:51, David Sommerseth wrote: > On 29/03/2022 21:29, Timo Rothenpieler wrote: >> --- >> This patch sits on top of the current dco branch, and will not apply to >> latest master. >> >> It solves the issue of dropping root privileges breaking dco and sitnl >> due to missing NET_ADMIN capabilities. >> >> >> configure.ac | 3 ++ >> src/openvpn/init.c | 22 +++++++++++++- >> src/openvpn/platform.c | 65 +++++++++++++++++++++++++++++++++++++++++- >> src/openvpn/platform.h | 2 +- >> 4 files changed, 89 insertions(+), 3 deletions(-) >> > > Thanks a lot! I've quickly looked through the code, and I have to NAK > this approach: > >> +#ifdef HAVE_LINUX_CAPABILITIES >> +#define SET_CAP_HELPER(data, set, cap) data[(cap)>>5].set |= >> 1<<((cap)&31) >> + >> +static bool >> +do_keep_caps(bool prepare) >> +{ >> + struct __user_cap_header_struct cap_hdr = { >> _LINUX_CAPABILITY_VERSION_3 }; >> + struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3] >> = {}; >> + >> + if (syscall(SYS_capget, &cap_hdr, cap_data) < 0) > > We should really use libcap or libcap-ng and not avoid using syscalls > directly. This did not come out well. Sorry about that. We should really avoid using syscalls directly, as that binds us to certain APIs and bindings. Newer kernels may also require additional adjustments in the future, to preserve the same behaviour. Which means we need to maintain this code and also pay more attention to the security aspects of privilege management, like new vulnerabilities and exploits. The libcap or libcap-ng libraries are used by far more applications, doing similar privilege management - and these libraries already pay attention to the security aspects of new vulnerabilities and exploits. The libcap-ng library is also recommended by more developers, due to its simpler API. It is possible to argue that sitnl does low-level calls to the kernel as well. But potential libraries had an API which was making everything far more complex on the OpenVPN side. For libcap-ng at least, that is not the case; as the API it provides is pretty simple. > I have used libcap-ng in openvpn3-linux, both for preserving > capabilities and dropping root. It does all the right steps fairly easily. > > The configure.ac detection, which for OpenVPN 2.x can be restricted when > DCO is going to be built into openvpn: > <https://github.com/OpenVPN/openvpn3-linux/blob/master/configure.ac#L113> > > The code for preserving capabilities: > <https://github.com/OpenVPN/openvpn3-linux/blob/c40218df43c8e652fedfa70304eae797b305e780/src/netcfg/openvpn3-service-netcfg.cpp#L82> > > > And the code for dropping root, ensuring the capabilities are restricted > properly: > <https://github.com/OpenVPN/openvpn3-linux/blob/c40218df43c8e652fedfa70304eae797b305e780/src/netcfg/openvpn3-service-netcfg.cpp#L64> >
On 30.03.2022 11:11, David Sommerseth wrote: > On 30/03/2022 10:51, David Sommerseth wrote: >> On 29/03/2022 21:29, Timo Rothenpieler wrote: >>> --- >>> This patch sits on top of the current dco branch, and will not apply to >>> latest master. >>> >>> It solves the issue of dropping root privileges breaking dco and sitnl >>> due to missing NET_ADMIN capabilities. >>> >>> >>> configure.ac | 3 ++ >>> src/openvpn/init.c | 22 +++++++++++++- >>> src/openvpn/platform.c | 65 +++++++++++++++++++++++++++++++++++++++++- >>> src/openvpn/platform.h | 2 +- >>> 4 files changed, 89 insertions(+), 3 deletions(-) >>> >> >> Thanks a lot! I've quickly looked through the code, and I have to NAK >> this approach: >> >>> +#ifdef HAVE_LINUX_CAPABILITIES >>> +#define SET_CAP_HELPER(data, set, cap) data[(cap)>>5].set |= >>> 1<<((cap)&31) >>> + >>> +static bool >>> +do_keep_caps(bool prepare) >>> +{ >>> + struct __user_cap_header_struct cap_hdr = { >>> _LINUX_CAPABILITY_VERSION_3 }; >>> + struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3] >>> = {}; >>> + >>> + if (syscall(SYS_capget, &cap_hdr, cap_data) < 0) >> >> We should really use libcap or libcap-ng and not avoid using syscalls >> directly. Is there any preference between the two? I initially used libcap, but wanted to avoid introducing another dependency. But both libcap and libcap-ng seem to be widely adopted by distros, and there isn't a huge difference in boilerplate between them for this purpose. > This did not come out well. Sorry about that. > > We should really avoid using syscalls directly, as that binds us to > certain APIs and bindings. > > Newer kernels may also require additional adjustments in the future, to > preserve the same behaviour. Which means we need to maintain this code > and also pay more attention to the security aspects of privilege > management, like new vulnerabilities and exploits. > > The libcap or libcap-ng libraries are used by far more applications, > doing similar privilege management - and these libraries already pay > attention to the security aspects of new vulnerabilities and exploits. > The libcap-ng library is also recommended by more developers, due to its > simpler API. > > It is possible to argue that sitnl does low-level calls to the kernel as > well. But potential libraries had an API which was making everything > far more complex on the OpenVPN side. For libcap-ng at least, that is > not the case; as the API it provides is pretty simple. Shouldn't caps support also be enabled when sitnl is in use? Given it also needs CAP_NET_ADMIN.
Hi, On Wed, Mar 30, 2022 at 01:31:24PM +0200, Timo Rothenpieler wrote: > > It is possible to argue that sitnl does low-level calls to the kernel as > > well. But potential libraries had an API which was making everything > > far more complex on the OpenVPN side. For libcap-ng at least, that is > > not the case; as the API it provides is pretty simple. > > Shouldn't caps support also be enabled when sitnl is in use? > Given it also needs CAP_NET_ADMIN. That was a misunderstanding. David explained why we are not using a library but directly talk to the netlink socket for SITNL. And yes, we want CAP_NET_ADMIN for sitnl+--user as well ;-) Thanks for your help on this, gert
Hi, On 30/03/2022 13:57, Gert Doering wrote: > Hi, > > On Wed, Mar 30, 2022 at 01:31:24PM +0200, Timo Rothenpieler wrote: >>> It is possible to argue that sitnl does low-level calls to the kernel as >>> well. But potential libraries had an API which was making everything >>> far more complex on the OpenVPN side. For libcap-ng at least, that is >>> not the case; as the API it provides is pretty simple. >> >> Shouldn't caps support also be enabled when sitnl is in use? >> Given it also needs CAP_NET_ADMIN. > > That was a misunderstanding. David explained why we are not using a > library but directly talk to the netlink socket for SITNL. > > And yes, we want CAP_NET_ADMIN for sitnl+--user as well ;-) One detail: using SITNL is a compile time decision, while using DCO is a runtime decision (assuming it was compiled in) Thanks!
diff --git a/configure.ac b/configure.ac index 7199483a..220c63e5 100644 --- a/configure.ac +++ b/configure.ac @@ -710,6 +710,9 @@ if test -z "${LIBPAM_LIBS}"; then ) fi +AC_CHECK_HEADERS([linux/capability.h sys/syscall.h sys/prctl.h], + [AC_DEFINE(HAVE_LINUX_CAPABILITIES, 1, [Linux capability support available])]) + case "${with_mem_check}" in valgrind) AC_CHECK_HEADERS( diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 8818ba6f..13c07ff0 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1138,6 +1138,25 @@ possibly_become_daemon(const struct options *options) return ret; } +/* + * Determine if we need to retain process capabilities. DCO and SITNL need it. + * Enforce it for DCO, but only try and soft-fail for SITNL to keep backwards compat. + */ +static int +get_need_keep_caps(struct context *c) +{ + if (dco_enabled(&c->options)) + { + return 1; + } + +#ifdef ENABLE_SITNL + return -1; +#else + return 0; +#endif +} + /* * Actually do UID/GID downgrade, chroot and SELinux context switching, if requested. */ @@ -1167,8 +1186,9 @@ do_uid_gid_chroot(struct context *c, bool no_delay) { if (no_delay) { + int keep_caps = get_need_keep_caps(c); platform_group_set(&c0->platform_state_group); - platform_user_set(&c0->platform_state_user); + platform_user_set(&c0->platform_state_user, keep_caps); } else if (c->first_time) { diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c index 450f28ba..680ebb5f 100644 --- a/src/openvpn/platform.c +++ b/src/openvpn/platform.c @@ -43,6 +43,12 @@ #include <direct.h> #endif +#ifdef HAVE_LINUX_CAPABILITIES +#include <linux/capability.h> +#include <sys/syscall.h> +#include <sys/prctl.h> +#endif + /* Redefine the top level directory of the filesystem * to restrict access to files for security */ void @@ -91,17 +97,74 @@ platform_user_get(const char *username, struct platform_state_user *state) return ret; } +#ifdef HAVE_LINUX_CAPABILITIES +#define SET_CAP_HELPER(data, set, cap) data[(cap)>>5].set |= 1<<((cap)&31) + +static bool +do_keep_caps(bool prepare) +{ + struct __user_cap_header_struct cap_hdr = { _LINUX_CAPABILITY_VERSION_3 }; + struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3] = {}; + + if (syscall(SYS_capget, &cap_hdr, cap_data) < 0) + { + msg(M_NONFATAL | M_ERRNO, "failed getting capabilities"); + return false; + } + + if (prepare) + { + SET_CAP_HELPER(cap_data, permitted, CAP_NET_ADMIN); + } + else + { + SET_CAP_HELPER(cap_data, effective, CAP_NET_ADMIN); + } + + if (syscall(SYS_capset, &cap_hdr, cap_data) < 0) + { + msg(M_NONFATAL | M_ERRNO, "failed setting %s capabilities", prepare ? "permitted" : "effective"); + return false; + } + + if (prepare && prctl(PR_SET_KEEPCAPS, 1) < 0) + { + msg(M_NONFATAL | M_ERRNO, "failed setting keepcaps"); + return false; + } + + return true; +} +#else +static bool +do_keep_caps(bool prepare) +{ + return false; +} +#endif + void -platform_user_set(const struct platform_state_user *state) +platform_user_set(const struct platform_state_user *state, int keep_caps) { #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID) if (state->username && state->pw) { + bool caps_prepared = keep_caps && do_keep_caps(true); + if (setuid(state->pw->pw_uid)) { msg(M_ERR, "setuid('%s') failed", state->username); } msg(M_INFO, "UID set to %s", state->username); + + if (caps_prepared && do_keep_caps(false)) + { + msg(M_INFO, "Capabilities retained"); + } + else if (keep_caps > 0) + { + msg(M_FATAL, "Failed retaining capabilities"); + } } #endif } diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h index a3eec298..ef06da93 100644 --- a/src/openvpn/platform.h +++ b/src/openvpn/platform.h @@ -79,7 +79,7 @@ struct platform_state_group { bool platform_user_get(const char *username, struct platform_state_user *state); -void platform_user_set(const struct platform_state_user *state); +void platform_user_set(const struct platform_state_user *state, int keep_caps); bool platform_group_get(const char *groupname, struct platform_state_group *state);