[Openvpn-devel] Retain CAP_NET_ADMIN when dropping privileges

Message ID 20220329192934.1415-1-timo@rothenpieler.org
State Superseded
Headers show
Series
  • [Openvpn-devel] Retain CAP_NET_ADMIN when dropping privileges
Related show

Commit Message

Timo Rothenpieler March 29, 2022, 7:29 p.m.
---
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(-)

Comments

Timo Rothenpieler March 29, 2022, 11:45 p.m. | #1
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;
> +}
David Sommerseth March 30, 2022, 8:51 a.m. | #2
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>
David Sommerseth March 30, 2022, 9:11 a.m. | #3
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> 
>
Timo Rothenpieler March 30, 2022, 11:31 a.m. | #4
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.
Gert Doering March 30, 2022, 11:57 a.m. | #5
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
Antonio Quartulli March 30, 2022, 12:16 p.m. | #6
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!

Patch

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