[Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

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

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

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

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;
> +}

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

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>


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

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


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

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.

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

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

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Antonio Quartulli]

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!

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

---
 configure.ac                              | 18 ++++++
 distro/systemd/openvpn-client@...2221... |  2 +-
 distro/systemd/openvpn-server@...2221... |  2 +-
 src/openvpn/init.c                        | 25 ++++++-
 src/openvpn/platform.c                    | 79 +++++++++++++++++++++++
 src/openvpn/platform.h                    |  5 ++
 6 files changed, 127 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 7199483a..5832b62f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -794,6 +794,24 @@ dnl
 	esac
 fi
 
+dnl
+dnl Depend on libcap-ng on Linux
+dnl
+case "$host" in
+	*-*-linux*)
+		PKG_CHECK_MODULES([LIBCAPNG],
+				  [libcap-ng],
+				  [have_libcapng="yes"],
+				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
+		)
+
+		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
+		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
+		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
+	;;
+esac
+
+
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
index cbcef653..159fb4dc 100644
--- a/distro/systemd/openvpn-client@...2221...
+++ b/distro/systemd/openvpn-client@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/client
 ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
index d1cc72cb..6e8e7d94 100644
--- a/distro/systemd/openvpn-server@...2221...
+++ b/distro/systemd/openvpn-server@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/server
 ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 8818ba6f..705eb92e 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,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
         {
             if (no_delay)
             {
-                platform_group_set(&c0->platform_state_group);
-                platform_user_set(&c0->platform_state_user);
+                int keep_caps = get_need_keep_caps(c);
+                platform_user_group_set(&c0->platform_state_user,
+                                        &c0->platform_state_group,
+                                        keep_caps);
             }
             else if (c->first_time)
             {
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 450f28ba..e67844ad 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -43,6 +43,10 @@
 #include <direct.h>
 #endif
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#endif
+
 /* Redefine the top level directory of the filesystem
  * to restrict access to files for security */
 void
@@ -155,6 +159,81 @@ platform_group_set(const struct platform_state_group *state)
 #endif
 }
 
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps)
+{
+    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
+#ifdef HAVE_LIBCAPNG
+    int new_gid = -1, new_uid = -1;
+    int res;
+
+    if (keep_caps == 0)
+    {
+        goto fallback;
+    }
+
+    /*
+     * new_uid/new_gid defaults to -1, which will not make
+     * libcap-ng change the UID/GID unless configured
+     */
+    if (group_state->groupname && group_state->gr)
+    {
+        new_gid = group_state->gr->gr_gid;
+    }
+    if (user_state->username && user_state->pw)
+    {
+        new_uid = user_state->pw->pw_uid;
+    }
+
+    /* Prepare capabilities before dropping UID/GID */
+    capng_clear(CAPNG_SELECT_BOTH);
+    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
+    if (res < 0)
+    {
+        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
+        goto fallback;
+    }
+
+    /* Change to new UID/GID */
+    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
+    if (res == -4 || res == -6)
+    {
+        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
+            user_state->username, group_state->groupname, res);
+    }
+    else if (res < 0)
+    {
+        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
+            user_state->username, group_state->groupname, res);
+        goto fallback;
+    }
+
+    if (new_uid >= 0)
+    {
+         msg(M_INFO, "UID set to %s", user_state->username);
+    }
+    if (new_gid >= 0)
+    {
+         msg(M_INFO, "GID set to %s", group_state->groupname);
+    }
+
+    msg(M_INFO, "Capabilities retained");
+
+    return;
+fallback:
+#endif  /* HAVE_LIBCAPNG */
+    if (keep_caps)
+    {
+        msg(err_flags, "Unable to retain capabilities");
+    }
+
+    platform_group_set(group_state);
+    platform_user_set(user_state);
+}
+
+
+
 /* Change process priority */
 void
 platform_nice(int niceval)
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a3eec298..b163f093 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -85,6 +85,11 @@ bool platform_group_get(const char *groupname, struct platform_state_group *stat
 
 void platform_group_set(const struct platform_state_group *state);
 
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps);
+
+
 /*
  * Extract UID or GID
  */
-- 
2.25.1

[Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

---
Using libcap-ng now


 configure.ac                              | 19 +++++
 distro/systemd/openvpn-client@...2221... |  2 +-
 distro/systemd/openvpn-server@...2221... |  2 +-
 src/openvpn/init.c                        | 25 ++++++-
 src/openvpn/platform.c                    | 91 +++++++++++++++++++++++
 src/openvpn/platform.h                    |  5 ++
 6 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 7199483a..168360d4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -794,6 +794,25 @@ dnl
 	esac
 fi
 
+dnl
+dnl Depend on libcap-ng on Linux
+dnl
+case "$host" in
+	*-*-linux*)
+		PKG_CHECK_MODULES([LIBCAPNG],
+				  [libcap-ng],
+				  [have_libcapng="yes"],
+				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
+		)
+		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
+
+		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
+		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
+		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
+	;;
+esac
+
+
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
index cbcef653..159fb4dc 100644
--- a/distro/systemd/openvpn-client@...2221...
+++ b/distro/systemd/openvpn-client@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/client
 ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
index d1cc72cb..6e8e7d94 100644
--- a/distro/systemd/openvpn-server@...2221...
+++ b/distro/systemd/openvpn-server@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/server
 ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 8818ba6f..705eb92e 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,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
         {
             if (no_delay)
             {
-                platform_group_set(&c0->platform_state_group);
-                platform_user_set(&c0->platform_state_user);
+                int keep_caps = get_need_keep_caps(c);
+                platform_user_group_set(&c0->platform_state_user,
+                                        &c0->platform_state_group,
+                                        keep_caps);
             }
             else if (c->first_time)
             {
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 450f28ba..4fce5a83 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -43,6 +43,11 @@
 #include <direct.h>
 #endif
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#include <sys/prctl.h>
+#endif
+
 /* Redefine the top level directory of the filesystem
  * to restrict access to files for security */
 void
@@ -155,6 +160,92 @@ platform_group_set(const struct platform_state_group *state)
 #endif
 }
 
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps)
+{
+    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
+#ifdef HAVE_LIBCAPNG
+    int new_gid = -1, new_uid = -1;
+    int res;
+
+    if (keep_caps == 0)
+    {
+        goto fallback;
+    }
+
+    /*
+     * new_uid/new_gid defaults to -1, which will not make
+     * libcap-ng change the UID/GID unless configured
+     */
+    if (group_state->groupname && group_state->gr)
+    {
+        new_gid = group_state->gr->gr_gid;
+    }
+    if (user_state->username && user_state->pw)
+    {
+        new_uid = user_state->pw->pw_uid;
+    }
+
+    /* Prepare capabilities before dropping UID/GID */
+    capng_clear(CAPNG_SELECT_BOTH);
+    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
+    if (res < 0)
+    {
+        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
+        goto fallback;
+    }
+
+    /* Change to new UID/GID.
+     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
+     */
+    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
+    if (res == -4 || res == -6)
+    {
+        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
+            user_state->username, group_state->groupname, res);
+    }
+    else if (res < 0)
+    {
+        if (res == -3)
+        {
+            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
+        }
+        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
+            user_state->username, group_state->groupname, res);
+        goto fallback;
+    }
+
+    if (new_uid >= 0)
+    {
+         msg(M_INFO, "UID set to %s", user_state->username);
+    }
+    if (new_gid >= 0)
+    {
+         msg(M_INFO, "GID set to %s", group_state->groupname);
+    }
+
+    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
+
+    return;
+fallback:
+    /* capng_change_id() can leave this flag clobbered on failure */
+    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
+    {
+        msg(M_ERR, "Clearing KEEPCAPS flag failed");
+    }
+#endif  /* HAVE_LIBCAPNG */
+    if (keep_caps)
+    {
+        msg(err_flags, "Unable to retain capabilities");
+    }
+
+    platform_group_set(group_state);
+    platform_user_set(user_state);
+}
+
+
+
 /* Change process priority */
 void
 platform_nice(int niceval)
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a3eec298..b163f093 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -85,6 +85,11 @@ bool platform_group_get(const char *groupname, struct platform_state_group *stat
 
 void platform_group_set(const struct platform_state_group *state);
 
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps);
+
+
 /*
  * Extract UID or GID
  */
-- 
2.25.1

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Jan Just Keijser]

Hi,

On 30/03/22 22:55, Timo Rothenpieler wrote:
> ---
> Using libcap-ng now
sorry to butt in late, but I've got a nasty feeling about this... the 
whole purpose of using
   --user
is, according to the man page
        --user user
               Change the user ID of the OpenVPN process to user after  
initialization,  dropping  privileges  in  the
               process.  This  option is useful to protect the system in 
the event that some hostile party was able to
               gain control of an OpenVPN session. Though OpenVPN's 
security features make this unlikely, it  is  pro‐
               vided as a second line of defense.

               By  setting  user  to  nobody or somebody similarly 
unprivileged, the hostile party would be limited in
               what damage they could cause. Of course once you take 
away privileges, you cannot  return  them  to an
               OpenVPN  session.  This  means, for example, that if you 
want to reset an OpenVPN daemon with a SIGUSR1
               signal (for example in response to a DHCP reset), you 
should make use of one or more of  the  --persist
               options  to  ensure  that OpenVPN doesn't need to execute 
any privileged operations in order to restart
               (such as re-reading key files or running ifconfig on the 
TUN device).

yet with this patch, the openvpn process remains capable of

        CAP_NET_ADMIN
               Perform various network-related operations:
               * interface configuration;
               * administration of IP firewall, masquerading, and
                 accounting;
               * modify routing tables;
               * bind to any address for transparent proxying;
               * set type-of-service (TOS);
               * clear driver statistics;
               * set promiscuous mode;
               * enabling multicasting;
               * use setsockopt(2) to set the following socket options:
                 SO_DEBUG, SO_MARK, SO_PRIORITY (for a priority outside
                 the range 0 to 6), SO_RCVBUFFORCE, and SO_SNDBUFFORCE.

so this "second line of defense" it getting *VERY* leaky in my opinion 
(and warrants a manpage update, at the very least).

The proper solution would be to have openvpn fork on itself, keep a 
"barebones" process running as root, but with the actual control and 
data channels running in the forked process using truly minimal privileges.

JM2CW,

JJK


>
>   configure.ac                              | 19 +++++
>   distro/systemd/openvpn-client@...2221... |  2 +-
>   distro/systemd/openvpn-server@...2221... |  2 +-
>   src/openvpn/init.c                        | 25 ++++++-
>   src/openvpn/platform.c                    | 91 +++++++++++++++++++++++
>   src/openvpn/platform.h                    |  5 ++
>   6 files changed, 140 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7199483a..168360d4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -794,6 +794,25 @@ dnl
>   	esac
>   fi
>   
> +dnl
> +dnl Depend on libcap-ng on Linux
> +dnl
> +case "$host" in
> +	*-*-linux*)
> +		PKG_CHECK_MODULES([LIBCAPNG],
> +				  [libcap-ng],
> +				  [have_libcapng="yes"],
> +				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
> +		)
> +		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
> +
> +		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
> +		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
> +		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
> +	;;
> +esac
> +
> +
>   if test "${with_crypto_library}" = "openssl"; then
>   	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
>   	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
> diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
> index cbcef653..159fb4dc 100644
> --- a/distro/systemd/openvpn-client@...2221...
> +++ b/distro/systemd/openvpn-client@...2221...
> @@ -11,7 +11,7 @@ Type=notify
>   PrivateTmp=true
>   WorkingDirectory=/etc/openvpn/client
>   ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
>   LimitNPROC=10
>   DeviceAllow=/dev/null rw
>   DeviceAllow=/dev/net/tun rw
> diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
> index d1cc72cb..6e8e7d94 100644
> --- a/distro/systemd/openvpn-server@...2221...
> +++ b/distro/systemd/openvpn-server@...2221...
> @@ -11,7 +11,7 @@ Type=notify
>   PrivateTmp=true
>   WorkingDirectory=/etc/openvpn/server
>   ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
>   LimitNPROC=10
>   DeviceAllow=/dev/null rw
>   DeviceAllow=/dev/net/tun rw
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 8818ba6f..705eb92e 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,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
>           {
>               if (no_delay)
>               {
> -                platform_group_set(&c0->platform_state_group);
> -                platform_user_set(&c0->platform_state_user);
> +                int keep_caps = get_need_keep_caps(c);
> +                platform_user_group_set(&c0->platform_state_user,
> +                                        &c0->platform_state_group,
> +                                        keep_caps);
>               }
>               else if (c->first_time)
>               {
> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
> index 450f28ba..4fce5a83 100644
> --- a/src/openvpn/platform.c
> +++ b/src/openvpn/platform.c
> @@ -43,6 +43,11 @@
>   #include <direct.h>
>   #endif
>   
> +#ifdef HAVE_LIBCAPNG
> +#include <cap-ng.h>
> +#include <sys/prctl.h>
> +#endif
> +
>   /* Redefine the top level directory of the filesystem
>    * to restrict access to files for security */
>   void
> @@ -155,6 +160,92 @@ platform_group_set(const struct platform_state_group *state)
>   #endif
>   }
>   
> +void platform_user_group_set(const struct platform_state_user *user_state,
> +                             const struct platform_state_group *group_state,
> +                             int keep_caps)
> +{
> +    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
> +#ifdef HAVE_LIBCAPNG
> +    int new_gid = -1, new_uid = -1;
> +    int res;
> +
> +    if (keep_caps == 0)
> +    {
> +        goto fallback;
> +    }
> +
> +    /*
> +     * new_uid/new_gid defaults to -1, which will not make
> +     * libcap-ng change the UID/GID unless configured
> +     */
> +    if (group_state->groupname && group_state->gr)
> +    {
> +        new_gid = group_state->gr->gr_gid;
> +    }
> +    if (user_state->username && user_state->pw)
> +    {
> +        new_uid = user_state->pw->pw_uid;
> +    }
> +
> +    /* Prepare capabilities before dropping UID/GID */
> +    capng_clear(CAPNG_SELECT_BOTH);
> +    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
> +    if (res < 0)
> +    {
> +        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
> +        goto fallback;
> +    }
> +
> +    /* Change to new UID/GID.
> +     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
> +     */
> +    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
> +    if (res == -4 || res == -6)
> +    {
> +        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
> +            user_state->username, group_state->groupname, res);
> +    }
> +    else if (res < 0)
> +    {
> +        if (res == -3)
> +        {
> +            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
> +        }
> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
> +            user_state->username, group_state->groupname, res);
> +        goto fallback;
> +    }
> +
> +    if (new_uid >= 0)
> +    {
> +         msg(M_INFO, "UID set to %s", user_state->username);
> +    }
> +    if (new_gid >= 0)
> +    {
> +         msg(M_INFO, "GID set to %s", group_state->groupname);
> +    }
> +
> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
> +
> +    return;
> +fallback:
> +    /* capng_change_id() can leave this flag clobbered on failure */
> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
> +    {
> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
> +    }
> +#endif  /* HAVE_LIBCAPNG */
> +    if (keep_caps)
> +    {
> +        msg(err_flags, "Unable to retain capabilities");
> +    }
> +
> +    platform_group_set(group_state);
> +    platform_user_set(user_state);
> +}
> +
> +
> +
>   /* Change process priority */
>   void
>   platform_nice(int niceval)
> diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
> index a3eec298..b163f093 100644
> --- a/src/openvpn/platform.h
> +++ b/src/openvpn/platform.h
> @@ -85,6 +85,11 @@ bool platform_group_get(const char *groupname, struct platform_state_group *stat
>   
>   void platform_group_set(const struct platform_state_group *state);
>   
> +void platform_user_group_set(const struct platform_state_user *user_state,
> +                             const struct platform_state_group *group_state,
> +                             int keep_caps);
> +
> +
>   /*
>    * Extract UID or GID
>    */

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

On 31/03/2022 08:53, Jan Just Keijser wrote:
> Hi,
> 
> On 30/03/22 22:55, Timo Rothenpieler wrote:
>> ---
>> Using libcap-ng now
> sorry to butt in late, but I've got a nasty feeling about this... the 
> whole purpose of using
>    --user
> is, according to the man page
>         --user user
>                Change the user ID of the OpenVPN process to user after 
> initialization,  dropping  privileges  in  the
>                process.  This  option is useful to protect the system in 
> the event that some hostile party was able to
>                gain control of an OpenVPN session. Though OpenVPN's 
> security features make this unlikely, it  is  pro‐
>                vided as a second line of defense.
> 
>                By  setting  user  to  nobody or somebody similarly 
> unprivileged, the hostile party would be limited in
>                what damage they could cause. Of course once you take 
> away privileges, you cannot  return  them  to an
>                OpenVPN  session.  This  means, for example, that if you 
> want to reset an OpenVPN daemon with a SIGUSR1
>                signal (for example in response to a DHCP reset), you 
> should make use of one or more of  the  --persist
>                options  to  ensure  that OpenVPN doesn't need to execute 
> any privileged operations in order to restart
>                (such as re-reading key files or running ifconfig on the 
> TUN device).
> 
> yet with this patch, the openvpn process remains capable of
> 
>         CAP_NET_ADMIN
>                Perform various network-related operations:
>                * interface configuration;
>                * administration of IP firewall, masquerading, and
>                  accounting;
>                * modify routing tables;
>                * bind to any address for transparent proxying;
>                * set type-of-service (TOS);
>                * clear driver statistics;
>                * set promiscuous mode;
>                * enabling multicasting;
>                * use setsockopt(2) to set the following socket options:
>                  SO_DEBUG, SO_MARK, SO_PRIORITY (for a priority outside
>                  the range 0 to 6), SO_RCVBUFFORCE, and SO_SNDBUFFORCE.
> 
> so this "second line of defense" it getting *VERY* leaky in my opinion 
> (and warrants a manpage update, at the very least).
> 
> The proper solution would be to have openvpn fork on itself, keep a 
> "barebones" process running as root, but with the actual control and 
> data channels running in the forked process using truly minimal privileges.

Hi,

You have some valid points about locking down OpenVPN.  However, without 
this change and having OpenVPN run completely without privileges it will 
not work well in certain situations with ovpn-dco.  Because it will not 
have the capabilities needed to interact with the kernel module. 
CAP_NET_ADMIN does give this possibility.

In regards to man-page update, I agree.  And we should do that as a 
additional patch.

And when it comes to this patch, currently it only use the capabilities 
feature when DCO is available on the system and not disabled in the 
configuration.  This in options.c which calls 
dco_check_option_conflict(), which is implemented in dco.c.

There is however another related challenge in OpenVPN 2.x, which became 
even clearer than be fore with the sitnl implementation we switched over 
to on Linux by default with v2.5.  When using --user/--group without 
--persist-tun, a reconnect would tear down the interface but could not 
recover again and the connection dies.  Using --persist-tun, it could 
work a bit better *unless* it needs to change the IP address of the tun 
interface.  I'm not sure how well, OpenVPN 2.x works if new routes are 
being pushed (OpenVPN 3 supports that as well).  This challenge is also 
resolved by granting the process CAP_NET_ADMIN capabilities.

For now, my opinion is that it is currently acceptable to have 
CAP_NET_ADMIN available when running with ovpn-dco; to have a smooth 
user experience.  OpenVPN is after all a network related process.



As a way forward after this, the aspect of how much to trust, 
capabilities and privileges you put into a single process needs to be 
better defined.  OpenVPN 2.x has a monolithic design, and the 
architecture of privilege separation is lacking at best.

We have not done any attempts improving this, as this will not be a 
trivial refactoring.  With ovpn-dco, the data-channel handling is 
already handled outside of the master OpenVPN process; so here there is 
some improvements indirectly.  Now the control-channel handling + 
network configuration (with CAP_NET_ADMIN) runs in the same process scope.

Arne and I discussed a while back to look into if the Network 
Configuration service (openvpn3-service-netcfg) from OpenVPN 3 Linux can 
be implemented in a way where it can be used by OpenVPN 2.x as well. 
Theoretically, this is possible but not trivial.

In OpenVPN 3 Linux, privilege separation is part of the design.  The 
client process (openvpn3-serivce-client) runs completely unprivileged, 
but gets its tun/ovpn-dco interface created by openvpn3-service-netcfg 
and the client passes VPN IP address, routes and DNS settings to this 
netcfg service which applies these changes.  The netcfg service runs as 
openvpn:openvpn with CAP_NET_ADMIN (and a few optional other ones, 
depending on the system setup) and can only be approached by 
openvpn3-service-client processes.  The rest of the OpenVPN 3 Linux 
stack runs completely unprivileged.  On systems with SELinux enabled, 
both openvpn3-service-client and openvpn3-service-netcfg runs confined 
in their own separate SELinux contexts, with a strict policy what each 
of them can do.

I am willing to work on making the netcfg service even less "OpenVPN 3 
centric", and it has a potential to grow towards a generic VPN API on 
Linux.  The current D-Bus interface it uses is highly inspired by the 
Android VPN API.  But this won't happen in a short time and not in time 
for the OpenVPN 2.6 release.  This is probably something which is more 
realistic for OpenVPN 2.8.  But this needs to be discussed more 
thoroughly (next hackathon?).


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Arne Schwabe]

> I am willing to work on making the netcfg service even less "OpenVPN 3 
> centric", and it has a potential to grow towards a generic VPN API on 
> Linux.  The current D-Bus interface it uses is highly inspired by the 
> Android VPN API.  But this won't happen in a short time and not in time 
> for the OpenVPN 2.6 release.  This is probably something which is more 
> realistic for OpenVPN 2.8.  But this needs to be discussed more 
> thoroughly (next hackathon?).

The current interface and interface design is ill suited for a server 
and I would even go so far as saying that extending the interface to 
support the full server functionality will go against the goal of the 
interface design.

A limited client mode might work but even that is something were you 
loose features that OpenVPN support in client mode. On platforms like 
ANdroid/iOS were you need to use an API like that you can hand wave that 
away as limitation of the platform but on Linux that is harder.

Also it is a very linux specific solution that will not work on macOS or 
Windows. So we might consider approaches as well.

Arne

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

Hi,

On Thu, Mar 31, 2022 at 12:06:06PM +0200, David Sommerseth wrote:
> There is however another related challenge in OpenVPN 2.x, which became 
> even clearer than be fore with the sitnl implementation we switched over 
> to on Linux by default with v2.5.  When using --user/--group without 
> --persist-tun, a reconnect would tear down the interface but could not 
> recover again and the connection dies.  Using --persist-tun, it could 
> work a bit better *unless* it needs to change the IP address of the tun 
> interface.  I'm not sure how well, OpenVPN 2.x works if new routes are 
> being pushed (OpenVPN 3 supports that as well).  This challenge is also 
> resolved by granting the process CAP_NET_ADMIN capabilities.

For most non-trivial stuff, OpenVPN with --user will run into problems,
be it route teardown, installing of new routes at renegotiation time,
...

So most people today just run 2.x as root, not getting any security 
benefits.

> For now, my opinion is that it is currently acceptable to have 
> CAP_NET_ADMIN available when running with ovpn-dco; to have a smooth 
> user experience.  OpenVPN is after all a network related process.

I'd even go for "keep CAP_NET_ADMIN for DCO and sitnl" - because it
means "all the route/interface manipulation *and cleanup* stuff can
be done properly, without having to carry root privileges".

> As a way forward after this, the aspect of how much to trust, 
> capabilities and privileges you put into a single process needs to be 
> better defined.  OpenVPN 2.x has a monolithic design, and the 
> architecture of privilege separation is lacking at best.

You might be surprised at what we have in 2.x :-) - with the service
pipe, we can run OpenVPN fully unprivileged, and do so on Windows.  

We just never had anyone bother to implement a backend for this for
"Unixy" platforms...

The benefit of that, securitywise, wouldn't be very large anyway,
compared to "CAP_NET_ADMIN + --user nobody" - the service is still
able to mess up routing and interface config, and that's about what
privileges remain in that combo... - so, dubious benefits, lots of
work.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 31.03.2022 13:02, Gert Doering wrote:
> Hi,
> 
> On Thu, Mar 31, 2022 at 12:06:06PM +0200, David Sommerseth wrote:
>> There is however another related challenge in OpenVPN 2.x, which became
>> even clearer than be fore with the sitnl implementation we switched over
>> to on Linux by default with v2.5.  When using --user/--group without
>> --persist-tun, a reconnect would tear down the interface but could not
>> recover again and the connection dies.  Using --persist-tun, it could
>> work a bit better *unless* it needs to change the IP address of the tun
>> interface.  I'm not sure how well, OpenVPN 2.x works if new routes are
>> being pushed (OpenVPN 3 supports that as well).  This challenge is also
>> resolved by granting the process CAP_NET_ADMIN capabilities.
> 
> For most non-trivial stuff, OpenVPN with --user will run into problems,
> be it route teardown, installing of new routes at renegotiation time,
> ...
> 
> So most people today just run 2.x as root, not getting any security
> benefits.
> 
>> For now, my opinion is that it is currently acceptable to have
>> CAP_NET_ADMIN available when running with ovpn-dco; to have a smooth
>> user experience.  OpenVPN is after all a network related process.
> 
> I'd even go for "keep CAP_NET_ADMIN for DCO and sitnl" - because it
> means "all the route/interface manipulation *and cleanup* stuff can
> be done properly, without having to carry root privileges".

That's exactly what the patch does.
Only difference is that for sitnl, to avoid breaking existing setups, it 
will fall back to the old approach of switching user if the capability 
retaining approach failed.

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

Hi,

On Thu, Mar 31, 2022 at 01:29:28PM +0200, Timo Rothenpieler wrote:
> That's exactly what the patch does.

Which I very much like :-)  (I said that on IRC already, repeating here
for the list archive)

> Only difference is that for sitnl, to avoid breaking existing setups, it 
> will fall back to the old approach of switching user if the capability 
> retaining approach failed.

I'm a bit undecided if this is really something to worry about... but
then, in an existing and working systemd environment with reduced
capabilities it might break setups going 2.5 -> 2.6, so maybe "being
careful about things" is the better way :-)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 931 bytes --]

On 31/03/2022 13:34, Gert Doering wrote:
> Hi,
> 
> On Thu, Mar 31, 2022 at 01:29:28PM +0200, Timo Rothenpieler wrote:
>> That's exactly what the patch does.
> 
> Which I very much like :-)  (I said that on IRC already, repeating here
> for the list archive)
> 
>> Only difference is that for sitnl, to avoid breaking existing setups, it
>> will fall back to the old approach of switching user if the capability
>> retaining approach failed.
> 
> I'm a bit undecided if this is really something to worry about... but
> then, in an existing and working systemd environment with reduced
> capabilities it might break setups going 2.5 -> 2.6, so maybe "being
> careful about things" is the better way :-)

Yeah, I agree with this.  For v2.6, the time is too short to be dare too 
much potential breakage now.  But we can consider further steps with v2.7.


-- 
kind regards,

David Sommerseth
OpenVPN Inc


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 2600 bytes --]

On 30/03/2022 22:55, Timo Rothenpieler wrote:
> ---
> Using libcap-ng now
> 
> 
>   configure.ac                              | 19 +++++
>   distro/systemd/openvpn-client@...2221... |  2 +-
>   distro/systemd/openvpn-server@...2221... |  2 +-
>   src/openvpn/init.c                        | 25 ++++++-
>   src/openvpn/platform.c                    | 91 +++++++++++++++++++++++
>   src/openvpn/platform.h                    |  5 ++
>   6 files changed, 140 insertions(+), 4 deletions(-)
> 
Since I worked closely with Timo on this patch version, I don't feel I 
should give it an ACK verdict alone.  But I believe this is the right 
patch to include.

I will just suggest a commit message:

----------------------------------------------
platform: Retain CAP_NET_ADMIN when dropping privileges

On Linux, when dropping privileges, interaction with the network 
configuration, such as tearing down routes or ovpn-dco interfaces
will fail when --user/--group are used.

This patch set sets the CAP_NET_ADMIN capability, which grants the 
needed privileges during the lifetime of the OpenVPN process when 
dropping root privileges.

Signed-off-by: Timo Rothenpieler <timo@...2669...>
Reviewed-By: David Sommerseth <davids@...515...>
----------------------------------------------


I have otherwise tested this patch on a Rocky Linux 8 distribution.
Client test cases I ran when testing this was:

   * from the command line, with and without DCO
   * via systemd, with and without DCO

With these 4 test cases, each of them were run with combinations of

   * no --user/--group
   * only --user
   * only --group
   * both --user and --group

I've also run a few tests using an --up script which modified 
/etc/resolv.conf, which also worked as expected with capabilities enabled.

There were no unexpected behavior with this final patch set, with one 
special exception which is outside the scope of this patch - SELinux.

SELinux on Fedora and RHEL (which Rocky Linux inherits) denies the 
OpenVPN process when run via systemd to use the SET_PCAP capability.  In 
addition, the SELinux reference policy also denies all interactions with 
the Generic Netlink interfaces used by ovpn-dco.  I will follow up this 
with the upstream SELinux reference policy maintainers.

Package maintainers needing SELinux can in the mean time, until an 
updated SELinux policy is available, provide an additional SELinux 
module which grants the needed privileges to openvpn_t labelled processes.


-- 
kind regards,

David Sommerseth
OpenVPN Inc


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

Hi,

On Thu, Mar 31, 2022 at 03:20:59PM +0200, David Sommerseth wrote:
> I've also run a few tests using an --up script which modified 
> /etc/resolv.conf, which also worked as expected with capabilities enabled.

This is actually an interesting corner case.  As far as I understand,
--up runs before setuid, so that should always succeed - but if you do
that, cleaning up resolv.conf in --down won't succeed.

(But this is a totally independent problem of "network things without
root" that this patch addresses)

[..]
> SELinux on Fedora and RHEL (which Rocky Linux inherits) denies the 
> OpenVPN process when run via systemd to use the SET_PCAP capability.  In 
> addition, the SELinux reference policy also denies all interactions with 
> the Generic Netlink interfaces used by ovpn-dco.  I will follow up this 
> with the upstream SELinux reference policy maintainers.

This is a good find.  Thanks :-)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 1043 bytes --]

On 31/03/2022 15:26, Gert Doering wrote:
> Hi,
> 
> On Thu, Mar 31, 2022 at 03:20:59PM +0200, David Sommerseth wrote:
>> I've also run a few tests using an --up script which modified
>> /etc/resolv.conf, which also worked as expected with capabilities enabled.
> 
> This is actually an interesting corner case.  As far as I understand,
> --up runs before setuid, so that should always succeed - but if you do
> that, cleaning up resolv.conf in --down won't succeed.

That is actually correct, and to be honest I didn't think about the 
order of when running as client.

We could "fix" --down now, but I will not recommend it at all.  We could 
add the CAP_DAC_OVERRIDE capability.  But that's a massive sledge 
hammer, giving read/write access to any file on the system. Only 
security modules like SELinux, AppArmor and such can block access with 
this capability enabled.  So this is definitely not the right capability 
to have in the main OpenVPN process now.


-- 
kind regards,

David Sommerseth
OpenVPN Inc


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

Hi,

On Thu, Mar 31, 2022 at 04:38:06PM +0200, David Sommerseth wrote:
> We could "fix" --down now, but I will not recommend it at all.  We could 
> add the CAP_DAC_OVERRIDE capability.  But that's a massive sledge 
> hammer, giving read/write access to any file on the system. Only 
> security modules like SELinux, AppArmor and such can block access with 
> this capability enabled.  So this is definitely not the right capability 
> to have in the main OpenVPN process now.

I agree.  

This is not what I was suggesting (not at all), just pointing out that 
the combination of --up, --user and --down is not with its own set 
of surprises ;-)

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Antonio Quartulli]

Hi,

On 30/03/2022 22:55, Timo Rothenpieler wrote:
> ---
> Using libcap-ng now

A commit message would be good, but I see that David has already 
proposed one.

> 
> 
>   configure.ac                              | 19 +++++
>   distro/systemd/openvpn-client@...2221... |  2 +-
>   distro/systemd/openvpn-server@...2221... |  2 +-
>   src/openvpn/init.c                        | 25 ++++++-
>   src/openvpn/platform.c                    | 91 +++++++++++++++++++++++
>   src/openvpn/platform.h                    |  5 ++
>   6 files changed, 140 insertions(+), 4 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 7199483a..168360d4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -794,6 +794,25 @@ dnl
>   	esac
>   fi
>   
> +dnl
> +dnl Depend on libcap-ng on Linux
> +dnl
> +case "$host" in
> +	*-*-linux*)
> +		PKG_CHECK_MODULES([LIBCAPNG],
> +				  [libcap-ng],
> +				  [have_libcapng="yes"],

do we really need have_libcapng? it seems it is not used further in 
configure.ac

> +				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
> +		)
> +		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
> +
> +		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
> +		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
> +		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
> +	;;
> +esac
> +
> +
>   if test "${with_crypto_library}" = "openssl"; then
>   	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
>   	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
> diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
> index cbcef653..159fb4dc 100644
> --- a/distro/systemd/openvpn-client@...2221...
> +++ b/distro/systemd/openvpn-client@...2221...
> @@ -11,7 +11,7 @@ Type=notify
>   PrivateTmp=true
>   WorkingDirectory=/etc/openvpn/client
>   ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
>   LimitNPROC=10
>   DeviceAllow=/dev/null rw
>   DeviceAllow=/dev/net/tun rw
> diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
> index d1cc72cb..6e8e7d94 100644
> --- a/distro/systemd/openvpn-server@...2221...
> +++ b/distro/systemd/openvpn-server@...2221...
> @@ -11,7 +11,7 @@ Type=notify
>   PrivateTmp=true
>   WorkingDirectory=/etc/openvpn/server
>   ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
>   LimitNPROC=10
>   DeviceAllow=/dev/null rw
>   DeviceAllow=/dev/net/tun rw
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 8818ba6f..705eb92e 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.
> + */

Since this function is returning a tri-state value, may it be better to 
document what the returned values mean in the comment above?

> +static int
> +get_need_keep_caps(struct context *c)

I know this function is kinda a getter, but two verbs in the function 
name sounds weird. How about just "need_keep_caps"?

> +{
> +    if (dco_enabled(&c->options))
> +    {
> +        return 1;
> +    }
> +
> +#ifdef ENABLE_SITNL
> +    return -1;
> +#else
> +    return 0;
> +#endif
> +}

Since the check above is really platform specific, wouldn't it make 
sense to move its definition to platform.c?
At the end of the day this function is just invoked once to get a value 
and immediately pass it to platform_user_group_set().

> +
>   /*
>    * Actually do UID/GID downgrade, chroot and SELinux context switching, if requested.
>    */
> @@ -1167,8 +1186,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
>           {
>               if (no_delay)
>               {
> -                platform_group_set(&c0->platform_state_group);
> -                platform_user_set(&c0->platform_state_user);

By removing the invocations above, these two functions are now defined 
and used only in platform.c. Therefore, they can be made static and 
their declarations can be removed from platform.h.

> +                int keep_caps = get_need_keep_caps(c);
> +                platform_user_group_set(&c0->platform_state_user,
> +                                        &c0->platform_state_group,
> +                                        keep_caps);
>               }
>               else if (c->first_time)
>               {
> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
> index 450f28ba..4fce5a83 100644
> --- a/src/openvpn/platform.c
> +++ b/src/openvpn/platform.c
> @@ -43,6 +43,11 @@
>   #include <direct.h>
>   #endif
>   
> +#ifdef HAVE_LIBCAPNG
> +#include <cap-ng.h>
> +#include <sys/prctl.h>
> +#endif
> +
>   /* Redefine the top level directory of the filesystem
>    * to restrict access to files for security */
>   void
> @@ -155,6 +160,92 @@ platform_group_set(const struct platform_state_group *state)
>   #endif
>   }
>   

Should we add a comment here describing what this function is doing? I 
know it is not complex, but maybe we should say that we will try to set 
the uid/gid using libcap-ng because we will also try to retain the 
CAP_NET_ADMIN capa? If libcap-ng is not available, we will simply try to 
switch user/group, unless not allowed (i.e. SITNL is enabled)

> +void platform_user_group_set(const struct platform_state_user *user_state,
> +                             const struct platform_state_group *group_state,
> +                             int keep_caps)
> +{
> +    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;

Shrug - I really don't like this "OpenVPN way" of handling code flow - 
with a value passed to a print() function...but hey, this is how it's 
currently done in most of the code.

> +#ifdef HAVE_LIBCAPNG
> +    int new_gid = -1, new_uid = -1;
> +    int res;
> +
> +    if (keep_caps == 0)
> +    {
> +        goto fallback;
> +    }
> +
> +    /*
> +     * new_uid/new_gid defaults to -1, which will not make
> +     * libcap-ng change the UID/GID unless configured
> +     */
> +    if (group_state->groupname && group_state->gr)
> +    {
> +        new_gid = group_state->gr->gr_gid;
> +    }
> +    if (user_state->username && user_state->pw)
> +    {
> +        new_uid = user_state->pw->pw_uid;
> +    }
> +
> +    /* Prepare capabilities before dropping UID/GID */
> +    capng_clear(CAPNG_SELECT_BOTH);
> +    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
> +    if (res < 0)
> +    {
> +        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
> +        goto fallback;
> +    }
> +
> +    /* Change to new UID/GID.
> +     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
> +     */
> +    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
> +    if (res == -4 || res == -6)

Argh, libcap-ng defines its own error codes...can we at least extend the 
comment above to explain what these magic -4 and -6 are? (this way we 
are sure to catch what the devel

> +    {
> +        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
> +            user_state->username, group_state->groupname, res);

If I understood correctly, in these two cases (-4 and -6) we failed to 
set either the uid or the gid, but the capas were retained?

If so, this means that the --user or --group options failed to be 
applied. Is it right to continue the execution? If I am not wrong 
OpenVPN currently aborts if the user/group couldn't be changed. 
Shouldn't we keep this behaviuor?

> +    }
> +    else if (res < 0)
> +    {
> +        if (res == -3)
> +        {
> +            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
> +        }
> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",

'man cap_change_id' does not mention setting errno at all.
What do we expect to see with M_ERRNO?

> +            user_state->username, group_state->groupname, res);
> +        goto fallback;
> +    }
> +
> +    if (new_uid >= 0)
> +    {
> +         msg(M_INFO, "UID set to %s", user_state->username);
> +    }
> +    if (new_gid >= 0)
> +    {
> +         msg(M_INFO, "GID set to %s", group_state->groupname);
> +    }
> +
> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
> +
> +    return;
> +fallback:
> +    /* capng_change_id() can leave this flag clobbered on failure */
> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)

What does this flag mean and why do we need to reset it to 0?
And what happens if the flag is not reset? (It seems we don't care much 
and just continue the execution)

> +    {
> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
> +    }
> +#endif  /* HAVE_LIBCAPNG */
> +    if (keep_caps)

for my poor eyes..add an empty line after the endif :-)

> +    {
> +        msg(err_flags, "Unable to retain capabilities");
> +    }
> +
> +    platform_group_set(group_state);
> +    platform_user_set(user_state);
> +}
> +
> +
> +

I think only one empty line is enough :D

>   /* Change process priority */
>   void
>   platform_nice(int niceval)
> diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
> index a3eec298..b163f093 100644
> --- a/src/openvpn/platform.h
> +++ b/src/openvpn/platform.h
> @@ -85,6 +85,11 @@ bool platform_group_get(const char *groupname, struct platform_state_group *stat
>   
>   void platform_group_set(const struct platform_state_group *state);
>   
> +void platform_user_group_set(const struct platform_state_user *user_state,
> +                             const struct platform_state_group *group_state,
> +                             int keep_caps);
> +
> +
>   /*
>    * Extract UID or GID
>    */


Regards,

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 06.04.2022 11:52, Antonio Quartulli wrote:
> Hi,
> 
> On 30/03/2022 22:55, Timo Rothenpieler wrote:
>> ---
>> Using libcap-ng now
> 
> A commit message would be good, but I see that David has already 
> proposed one.

The latest rebased version of this patch already has that message.
Just seemed silly to re-send it just because of that:
https://github.com/BtbN/openvpn/tree/dco


>>
>>
>>   configure.ac                              | 19 +++++
>>   distro/systemd/openvpn-client@...2221... |  2 +-
>>   distro/systemd/openvpn-server@...2221... |  2 +-
>>   src/openvpn/init.c                        | 25 ++++++-
>>   src/openvpn/platform.c                    | 91 +++++++++++++++++++++++
>>   src/openvpn/platform.h                    |  5 ++
>>   6 files changed, 140 insertions(+), 4 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 7199483a..168360d4 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -794,6 +794,25 @@ dnl
>>       esac
>>   fi
>> +dnl
>> +dnl Depend on libcap-ng on Linux
>> +dnl
>> +case "$host" in
>> +    *-*-linux*)
>> +        PKG_CHECK_MODULES([LIBCAPNG],
>> +                  [libcap-ng],
>> +                  [have_libcapng="yes"],
> 
> do we really need have_libcapng? it seems it is not used further in 
> configure.ac

I have little to no experience with autotools, and I think this is 
straight up copied from the openvpn3 setup.
Can I just leave the line empty? Or put empty [] there?

>> +                  [AC_MSG_ERROR([libcap-ng package not found. Is the 
>> development package and pkg-config installed?])]
>> +        )
>> +        AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not 
>> found!])])
>> +
>> +        CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
>> +        LIBS="${LIBS} ${LIBCAPNG_LIBS}"
>> +        AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
>> +    ;;
>> +esac
>> +
>> +
>>   if test "${with_crypto_library}" = "openssl"; then
>>       AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
>>       AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
>> diff --git a/distro/systemd/openvpn-client@...2221... 
>> b/distro/systemd/openvpn-client@...2221...
>> index cbcef653..159fb4dc 100644
>> --- a/distro/systemd/openvpn-client@...2221...
>> +++ b/distro/systemd/openvpn-client@...2221...
>> @@ -11,7 +11,7 @@ Type=notify
>>   PrivateTmp=true
>>   WorkingDirectory=/etc/openvpn/client
>>   ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config 
>> %i.conf
>> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW 
>> CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
>> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW 
>> CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
>>   LimitNPROC=10
>>   DeviceAllow=/dev/null rw
>>   DeviceAllow=/dev/net/tun rw
>> diff --git a/distro/systemd/openvpn-server@...2221... 
>> b/distro/systemd/openvpn-server@...2221...
>> index d1cc72cb..6e8e7d94 100644
>> --- a/distro/systemd/openvpn-server@...2221...
>> +++ b/distro/systemd/openvpn-server@...2221...
>> @@ -11,7 +11,7 @@ Type=notify
>>   PrivateTmp=true
>>   WorkingDirectory=/etc/openvpn/server
>>   ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log 
>> --status-version 2 --suppress-timestamps --config %i.conf
>> -CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
>> CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE 
>> CAP_AUDIT_WRITE
>> +CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE 
>> CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT 
>> CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
>>   LimitNPROC=10
>>   DeviceAllow=/dev/null rw
>>   DeviceAllow=/dev/net/tun rw
>> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
>> index 8818ba6f..705eb92e 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.
>> + */
> 
> Since this function is returning a tri-state value, may it be better to 
> document what the returned values mean in the comment above?
> 
>> +static int
>> +get_need_keep_caps(struct context *c)
> 
> I know this function is kinda a getter, but two verbs in the function 
> name sounds weird. How about just "need_keep_caps"?
> 
>> +{
>> +    if (dco_enabled(&c->options))
>> +    {
>> +        return 1;
>> +    }
>> +
>> +#ifdef ENABLE_SITNL
>> +    return -1;
>> +#else
>> +    return 0;
>> +#endif
>> +}
> 
> Since the check above is really platform specific, wouldn't it make 
> sense to move its definition to platform.c?
> At the end of the day this function is just invoked once to get a value 
> and immediately pass it to platform_user_group_set().

The reason it ended up here is that it needs access to the context, to 
find out if it's using the dco or not.
Should the context just be passed to the platform function instead? I 
see no other one of them doing that.

>> +
>>   /*
>>    * Actually do UID/GID downgrade, chroot and SELinux context 
>> switching, if requested.
>>    */
>> @@ -1167,8 +1186,10 @@ do_uid_gid_chroot(struct context *c, bool 
>> no_delay)
>>           {
>>               if (no_delay)
>>               {
>> -                platform_group_set(&c0->platform_state_group);
>> -                platform_user_set(&c0->platform_state_user);
> 
> By removing the invocations above, these two functions are now defined 
> and used only in platform.c. Therefore, they can be made static and 
> their declarations can be removed from platform.h.

That was intentional, since it seemed out of scope of this patch.
I can easily add it though if preferred.

>> +                int keep_caps = get_need_keep_caps(c);
>> +                platform_user_group_set(&c0->platform_state_user,
>> +                                        &c0->platform_state_group,
>> +                                        keep_caps);
>>               }
>>               else if (c->first_time)
>>               {
>> diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
>> index 450f28ba..4fce5a83 100644
>> --- a/src/openvpn/platform.c
>> +++ b/src/openvpn/platform.c
>> @@ -43,6 +43,11 @@
>>   #include <direct.h>
>>   #endif
>> +#ifdef HAVE_LIBCAPNG
>> +#include <cap-ng.h>
>> +#include <sys/prctl.h>
>> +#endif
>> +
>>   /* Redefine the top level directory of the filesystem
>>    * to restrict access to files for security */
>>   void
>> @@ -155,6 +160,92 @@ platform_group_set(const struct 
>> platform_state_group *state)
>>   #endif
>>   }
> 
> Should we add a comment here describing what this function is doing? I 
> know it is not complex, but maybe we should say that we will try to set 
> the uid/gid using libcap-ng because we will also try to retain the 
> CAP_NET_ADMIN capa? If libcap-ng is not available, we will simply try to 
> switch user/group, unless not allowed (i.e. SITNL is enabled)
> 
>> +void platform_user_group_set(const struct platform_state_user 
>> *user_state,
>> +                             const struct platform_state_group 
>> *group_state,
>> +                             int keep_caps)
>> +{
>> +    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
> 
> Shrug - I really don't like this "OpenVPN way" of handling code flow - 
> with a value passed to a print() function...but hey, this is how it's 
> currently done in most of the code.
> 
>> +#ifdef HAVE_LIBCAPNG
>> +    int new_gid = -1, new_uid = -1;
>> +    int res;
>> +
>> +    if (keep_caps == 0)
>> +    {
>> +        goto fallback;
>> +    }
>> +
>> +    /*
>> +     * new_uid/new_gid defaults to -1, which will not make
>> +     * libcap-ng change the UID/GID unless configured
>> +     */
>> +    if (group_state->groupname && group_state->gr)
>> +    {
>> +        new_gid = group_state->gr->gr_gid;
>> +    }
>> +    if (user_state->username && user_state->pw)
>> +    {
>> +        new_uid = user_state->pw->pw_uid;
>> +    }
>> +
>> +    /* Prepare capabilities before dropping UID/GID */
>> +    capng_clear(CAPNG_SELECT_BOTH);
>> +    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, 
>> CAP_NET_ADMIN);
>> +    if (res < 0)
>> +    {
>> +        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
>> +        goto fallback;
>> +    }
>> +
>> +    /* Change to new UID/GID.
>> +     * capng_change_id() internally calls capng_apply() to apply 
>> prepared capabilities.
>> +     */
>> +    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | 
>> CAPNG_CLEAR_BOUNDING);
>> +    if (res == -4 || res == -6)
> 
> Argh, libcap-ng defines its own error codes...can we at least extend the 
> comment above to explain what these magic -4 and -6 are? (this way we 
> are sure to catch what the devel
> 
>> +    {
>> +        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
>> +            user_state->username, group_state->groupname, res);
> 
> If I understood correctly, in these two cases (-4 and -6) we failed to 
> set either the uid or the gid, but the capas were retained?
> 
> If so, this means that the --user or --group options failed to be 
> applied. Is it right to continue the execution? If I am not wrong 
> OpenVPN currently aborts if the user/group couldn't be changed. 
> Shouldn't we keep this behaviuor?

That's the whole point of this check, it aborts (via M_ERR) if 
setuid/setgid failed, since there is no fallback to that and it'd fail 
again anyway if openvpn tried again.

A comment explaining the -4 and -6 return codes is definitely a good idea.

>> +    }
>> +    else if (res < 0)
>> +    {
>> +        if (res == -3)
>> +        {
>> +            msg(M_NONFATAL, "Following error likely due to missing 
>> capability CAP_SETPCAP.");
>> +        }
>> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed 
>> retaining capabilities: %d",
> 
> 'man cap_change_id' does not mention setting errno at all.
> What do we expect to see with M_ERRNO?

Every function it internally calls sets errno, so in case of failure 
errno will reflect what went wrong. Like, for example EPERM will be the 
most common cause of failure.

>> +            user_state->username, group_state->groupname, res);
>> +        goto fallback;
>> +    }
>> +
>> +    if (new_uid >= 0)
>> +    {
>> +         msg(M_INFO, "UID set to %s", user_state->username);
>> +    }
>> +    if (new_gid >= 0)
>> +    {
>> +         msg(M_INFO, "GID set to %s", group_state->groupname);
>> +    }
>> +
>> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
>> +
>> +    return;
>> +fallback:
>> +    /* capng_change_id() can leave this flag clobbered on failure */
>> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
> 
> What does this flag mean and why do we need to reset it to 0?
> And what happens if the flag is not reset? (It seems we don't care much 
> and just continue the execution)

See https://github.com/stevegrubb/libcap-ng/issues/33

It's basically working around that issue, where capng_change_id() 
failing can leave the KEEPCAPS flag set, which would lead to our 
fallback setuid to retain rootlike capabilities, so we have to ensure 
it's unset before attempting to setuid.

It's fixed in upstream libcap-ng now, but it will take potentially 
months until that makes it into a release, and then even longer until it 
hits distros, so implementing a workaround seemed in order.
It has no ill effects and will just be a no-op with the patch in place.

>> +    {
>> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
>> +    }
>> +#endif  /* HAVE_LIBCAPNG */
>> +    if (keep_caps)
> 
> for my poor eyes..add an empty line after the endif :-)
> 
>> +    {
>> +        msg(err_flags, "Unable to retain capabilities");
>> +    }
>> +
>> +    platform_group_set(group_state);
>> +    platform_user_set(user_state);
>> +}
>> +
>> +
>> +
> 
> I think only one empty line is enough :D
> 
>>   /* Change process priority */
>>   void
>>   platform_nice(int niceval)
>> diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
>> index a3eec298..b163f093 100644
>> --- a/src/openvpn/platform.h
>> +++ b/src/openvpn/platform.h
>> @@ -85,6 +85,11 @@ bool platform_group_get(const char *groupname, 
>> struct platform_state_group *stat
>>   void platform_group_set(const struct platform_state_group *state);
>> +void platform_user_group_set(const struct platform_state_user 
>> *user_state,
>> +                             const struct platform_state_group 
>> *group_state,
>> +                             int keep_caps);
>> +
>> +
>>   /*
>>    * Extract UID or GID
>>    */
> 
> 
> Regards,
> 

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

On 06/04/2022 14:44, Timo Rothenpieler wrote:
>>>
>>
>> 'man cap_change_id' does not mention setting errno at all.
>> What do we expect to see with M_ERRNO?
> 
> Every function it internally calls sets errno, so in case of failure 
> errno will reflect what went wrong. Like, for example EPERM will be the 
> most common cause of failure.

capng_change_id() does several other lower level calls, like prctl(), 
setgroups(), setresgid() and setresuid().  They all set errno if an 
error occurs.  The return code of capng_change_id() just reflects in 
which phase of dropping privileges/uid/gid it failed.

For more details of the capng_change_id(), the implementation itself 
isn't that hard to read (but it does a several steps to harden the 
privilege drop): 
<https://github.com/stevegrubb/libcap-ng/blob/03b8572843b36bf071776a311c61f8d1dcfc4d53/src/cap-ng.c#L960>


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH v2] Retain CAP_NET_ADMIN when dropping privileges

[David Sommerseth]

On 06/04/2022 14:44, Timo Rothenpieler wrote:
>>>
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -794,6 +794,25 @@ dnl
>>>       esac
>>>   fi
>>> +dnl
>>> +dnl Depend on libcap-ng on Linux
>>> +dnl
>>> +case "$host" in
>>> +    *-*-linux*)
>>> +        PKG_CHECK_MODULES([LIBCAPNG],
>>> +                  [libcap-ng],
>>> +                  [have_libcapng="yes"],
>>
>> do we really need have_libcapng? it seems it is not used further in 
>> configure.ac
> 
> I have little to no experience with autotools, and I think this is 
> straight up copied from the openvpn3 setup.

That's probably right.

> Can I just leave the line empty? Or put empty [] there?

Yes, [] should suffice.  That said, it makes no big difference if it is 
there or not.  If needed to check ("test") if libcap-ng is available or 
not, this checks needs to be added again though.

That said, we do not have a consistent way of using PKG_CHECK_MODULES() 
in general.  We have at least 4 different ways in use today.

Probably something to clean-up some day later.


-- 
kind regards,

David Sommerseth
OpenVPN Inc

[Openvpn-devel] [PATCH v3] platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On Linux, when dropping privileges, interaction with
the network configuration, such as tearing down routes
or ovpn-dco interfaces will fail when --user/--group are
used.

This patch sets the CAP_NET_ADMIN capability, which grants
the needed privileges during the lifetime of the OpenVPN
process when dropping root privileges.

Signed-off-by: Timo Rothenpieler <timo@...2669...>
Reviewed-By: David Sommerseth <davids@...515...>
---
 configure.ac                              |  19 ++++
 distro/systemd/openvpn-client@...2221... |   2 +-
 distro/systemd/openvpn-server@...2221... |   2 +-
 src/openvpn/init.c                        |  30 ++++++-
 src/openvpn/platform.c                    | 105 +++++++++++++++++++++-
 src/openvpn/platform.h                    |   7 +-
 6 files changed, 156 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 85921ddb..d2eb3426 100644
--- a/configure.ac
+++ b/configure.ac
@@ -794,6 +794,25 @@ dnl
 	esac
 fi
 
+dnl
+dnl Depend on libcap-ng on Linux
+dnl
+case "$host" in
+	*-*-linux*)
+		PKG_CHECK_MODULES([LIBCAPNG],
+				  [libcap-ng],
+				  [],
+				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
+		)
+		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
+
+		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
+		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
+		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
+	;;
+esac
+
+
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
index cbcef653..159fb4dc 100644
--- a/distro/systemd/openvpn-client@...2221...
+++ b/distro/systemd/openvpn-client@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/client
 ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
index d1cc72cb..6e8e7d94 100644
--- a/distro/systemd/openvpn-server@...2221...
+++ b/distro/systemd/openvpn-server@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/server
 ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 8818ba6f..0a1b00ca 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1138,6 +1138,30 @@ 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.
+ *
+ * Returns the tri-state expected by platform_user_group_set.
+ * -1: try to keep caps, but continue if impossible
+ *  0: don't keep caps
+ *  1: keep caps, fail hard if impossible
+ */
+static int
+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 +1191,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
         {
             if (no_delay)
             {
-                platform_group_set(&c0->platform_state_group);
-                platform_user_set(&c0->platform_state_user);
+                int keep_caps = need_keep_caps(c);
+                platform_user_group_set(&c0->platform_state_user,
+                                        &c0->platform_state_group,
+                                        keep_caps);
             }
             else if (c->first_time)
             {
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 450f28ba..1e321369 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -43,6 +43,11 @@
 #include <direct.h>
 #endif
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#include <sys/prctl.h>
+#endif
+
 /* Redefine the top level directory of the filesystem
  * to restrict access to files for security */
 void
@@ -91,7 +96,7 @@ platform_user_get(const char *username, struct platform_state_user *state)
     return ret;
 }
 
-void
+static void
 platform_user_set(const struct platform_state_user *state)
 {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
@@ -130,7 +135,7 @@ platform_group_get(const char *groupname, struct platform_state_group *state)
     return ret;
 }
 
-void
+static void
 platform_group_set(const struct platform_state_group *state)
 {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
@@ -155,6 +160,102 @@ platform_group_set(const struct platform_state_group *state)
 #endif
 }
 
+/* Set user and group, retaining neccesary capabilities required by the platform.
+ *
+ * The keep_caps argument has 3 possible states:
+ *  >0: Retain capabilities, and fail hard on failure to do so.
+ * ==0: Don't attempt to retain any capabilities, just sitch user/group.
+ *  <0: Try to retain capabilities, but continue on failure.
+ */
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps)
+{
+    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
+#ifdef HAVE_LIBCAPNG
+    int new_gid = -1, new_uid = -1;
+    int res;
+
+    if (keep_caps == 0)
+    {
+        goto fallback;
+    }
+
+    /*
+     * new_uid/new_gid defaults to -1, which will not make
+     * libcap-ng change the UID/GID unless configured
+     */
+    if (group_state->groupname && group_state->gr)
+    {
+        new_gid = group_state->gr->gr_gid;
+    }
+    if (user_state->username && user_state->pw)
+    {
+        new_uid = user_state->pw->pw_uid;
+    }
+
+    /* Prepare capabilities before dropping UID/GID */
+    capng_clear(CAPNG_SELECT_BOTH);
+    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
+    if (res < 0)
+    {
+        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
+        goto fallback;
+    }
+
+    /* Change to new UID/GID.
+     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
+     */
+    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
+    if (res == -4 || res == -6)
+    {
+        /* -4 and -6 mean failure of setuid/gid respectively.
+           There is no point for us to continue if those failed. */
+        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
+            user_state->username, group_state->groupname, res);
+    }
+    else if (res < 0)
+    {
+        if (res == -3)
+        {
+            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
+        }
+        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
+            user_state->username, group_state->groupname, res);
+        goto fallback;
+    }
+
+    if (new_uid >= 0)
+    {
+         msg(M_INFO, "UID set to %s", user_state->username);
+    }
+    if (new_gid >= 0)
+    {
+         msg(M_INFO, "GID set to %s", group_state->groupname);
+    }
+
+    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
+
+    return;
+fallback:
+    /* capng_change_id() can leave this flag clobbered on failure
+     * This is working around a bug in libcap-ng, which can leave the flag set
+     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
+    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
+    {
+        msg(M_ERR, "Clearing KEEPCAPS flag failed");
+    }
+#endif  /* HAVE_LIBCAPNG */
+
+    if (keep_caps)
+    {
+        msg(err_flags, "Unable to retain capabilities");
+    }
+
+    platform_group_set(group_state);
+    platform_user_set(user_state);
+}
+
 /* Change process priority */
 void
 platform_nice(int niceval)
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a3eec298..19187d3a 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -79,11 +79,12 @@ 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);
-
 bool platform_group_get(const char *groupname, struct platform_state_group *state);
 
-void platform_group_set(const struct platform_state_group *state);
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps);
+
 
 /*
  * Extract UID or GID
-- 
2.25.1

Re: [Openvpn-devel] [PATCH v3] platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

Hi,

On Thu, Apr 07, 2022 at 08:40:24PM +0200, Timo Rothenpieler wrote:
> +    else if (res < 0)
> +    {
> +        if (res == -3)
> +        {
> +            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
> +        }
> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
> +            user_state->username, group_state->groupname, res);
> +        goto fallback;
> +    }

Wouldn't that overwrite errno for the "res == -3" case, given that
msg() will do stdio stuff?  Maybe reorder and print the "error likely due..."
message with a preceding "NOTE:" after the capng_change_id() message?

(That would be more typical for our logs - the "NOTE: this could be
because..." tends to come after the error message)

> +    if (new_uid >= 0)
> +    {
> +         msg(M_INFO, "UID set to %s", user_state->username);
> +    }
> +    if (new_gid >= 0)
> +    {
> +         msg(M_INFO, "GID set to %s", group_state->groupname);
> +    }
> +
> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
> +
> +    return;
> +fallback:

My inner whitespace dragon does would prefer to have the blank line
between "return" and "fallback:" (and no blank linke after the M_INFO).

> +    /* capng_change_id() can leave this flag clobbered on failure
> +     * This is working around a bug in libcap-ng, which can leave the flag set
> +     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
> +    {
> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
> +    }
> +#endif  /* HAVE_LIBCAPNG */

This one does not really look like it should be in "fallback:" - because
that way it always gets called, even if we jump there right at function
entry, if keep_caps == 0.

> +
> +    if (keep_caps)
> +    {
> +        msg(err_flags, "Unable to retain capabilities");
> +    }
> +
> +    platform_group_set(group_state);
> +    platform_user_set(user_state);
> +}
> +

Maybe "fallback:" should be right before platform_group_set()?


(Sorry for being late to the "complain about your code" party...)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH v3] platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 08/04/2022 11:35, Gert Doering wrote:
> Hi,
> 
> On Thu, Apr 07, 2022 at 08:40:24PM +0200, Timo Rothenpieler wrote:
>> +    else if (res < 0)
>> +    {
>> +        if (res == -3)
>> +        {
>> +            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
>> +        }
>> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
>> +            user_state->username, group_state->groupname, res);
>> +        goto fallback;
>> +    }
> 
> Wouldn't that overwrite errno for the "res == -3" case, given that
> msg() will do stdio stuff?  Maybe reorder and print the "error likely due..."
> message with a preceding "NOTE:" after the capng_change_id() message?
> 
> (That would be more typical for our logs - the "NOTE: this could be
> because..." tends to come after the error message)

Good point.
I like that idea better as well, will implement.

>> +    if (new_uid >= 0)
>> +    {
>> +         msg(M_INFO, "UID set to %s", user_state->username);
>> +    }
>> +    if (new_gid >= 0)
>> +    {
>> +         msg(M_INFO, "GID set to %s", group_state->groupname);
>> +    }
>> +
>> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
>> +
>> +    return;
>> +fallback:
> 
> My inner whitespace dragon does would prefer to have the blank line
> between "return" and "fallback:" (and no blank linke after the M_INFO).

No strong preference there on my side, so sure.

>> +    /* capng_change_id() can leave this flag clobbered on failure
>> +     * This is working around a bug in libcap-ng, which can leave the flag set
>> +     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
>> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
>> +    {
>> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
>> +    }
>> +#endif  /* HAVE_LIBCAPNG */
> 
> This one does not really look like it should be in "fallback:" - because
> that way it always gets called, even if we jump there right at function
> entry, if keep_caps == 0.

No, it's intentional. It ensures that it's printed even if we don't 
HAVE_LIBCAPNG but someone called the function requesting to keep 
capabilities, which is not possible then.

>> +
>> +    if (keep_caps)
>> +    {
>> +        msg(err_flags, "Unable to retain capabilities");
>> +    }
>> +
>> +    platform_group_set(group_state);
>> +    platform_user_set(user_state);
>> +}
>> +
> 
> Maybe "fallback:" should be right before platform_group_set()?
> 
> 
> (Sorry for being late to the "complain about your code" party...)
> 
> gert

Re: [Openvpn-devel] [PATCH v3] platform: Retain CAP_NET_ADMIN when dropping privileges

[Antonio Quartulli]

Hi,

On 08/04/2022 13:18, Timo Rothenpieler wrote:
>> This one does not really look like it should be in "fallback:" - because
>> that way it always gets called, even if we jump there right at function
>> entry, if keep_caps == 0.
> 
> No, it's intentional. It ensures that it's printed even if we don't 
> HAVE_LIBCAPNG but someone called the function requesting to keep 
> capabilities, which is not possible then.

Along Gert's thoughts: maybe there should be 2 labels, one that is just 
"out" and one that is "fallback".

Simply because jumping to "fallback" when it was not requested to retain 
capabilities (keep_caps == 0) may not sound right (are we really falling 
back in this case?)

Cheers,

> 
>>> +
>>> +    if (keep_caps)
>>> +    {
>>> +        msg(err_flags, "Unable to retain capabilities");
>>> +    }
>>> +
>>> +    platform_group_set(group_state);
>>> +    platform_user_set(user_state);
>>> +}
>>> +
>>
>> Maybe "fallback:" should be right before platform_group_set()?
>>
>>
>> (Sorry for being late to the "complain about your code" party...)
>>
>> gert
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
> 

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH] platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On Linux, when dropping privileges, interaction with
the network configuration, such as tearing down routes
or ovpn-dco interfaces will fail when --user/--group are
used.

This patch sets the CAP_NET_ADMIN capability, which grants
the needed privileges during the lifetime of the OpenVPN
process when dropping root privileges.

Signed-off-by: Timo Rothenpieler <timo@...2669...>
Reviewed-By: David Sommerseth <davids@...515...>
---
 configure.ac                              |  19 ++++
 distro/systemd/openvpn-client@...2221... |   2 +-
 distro/systemd/openvpn-server@...2221... |   2 +-
 src/openvpn/init.c                        |  30 +++++-
 src/openvpn/platform.c                    | 107 +++++++++++++++++++++-
 src/openvpn/platform.h                    |   7 +-
 6 files changed, 158 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 85921ddb..d2eb3426 100644
--- a/configure.ac
+++ b/configure.ac
@@ -794,6 +794,25 @@ dnl
 	esac
 fi
 
+dnl
+dnl Depend on libcap-ng on Linux
+dnl
+case "$host" in
+	*-*-linux*)
+		PKG_CHECK_MODULES([LIBCAPNG],
+				  [libcap-ng],
+				  [],
+				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
+		)
+		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
+
+		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
+		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
+		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
+	;;
+esac
+
+
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
index cbcef653..159fb4dc 100644
--- a/distro/systemd/openvpn-client@...2221...
+++ b/distro/systemd/openvpn-client@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/client
 ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
index d1cc72cb..6e8e7d94 100644
--- a/distro/systemd/openvpn-server@...2221...
+++ b/distro/systemd/openvpn-server@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/server
 ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 857e52ef..559f7cda 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1138,6 +1138,30 @@ 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.
+ *
+ * Returns the tri-state expected by platform_user_group_set.
+ * -1: try to keep caps, but continue if impossible
+ *  0: don't keep caps
+ *  1: keep caps, fail hard if impossible
+ */
+static int
+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 +1191,10 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
         {
             if (no_delay)
             {
-                platform_group_set(&c0->platform_state_group);
-                platform_user_set(&c0->platform_state_user);
+                int keep_caps = need_keep_caps(c);
+                platform_user_group_set(&c0->platform_state_user,
+                                        &c0->platform_state_group,
+                                        keep_caps);
             }
             else if (c->first_time)
             {
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 450f28ba..5a81e8d0 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -43,6 +43,11 @@
 #include <direct.h>
 #endif
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#include <sys/prctl.h>
+#endif
+
 /* Redefine the top level directory of the filesystem
  * to restrict access to files for security */
 void
@@ -91,7 +96,7 @@ platform_user_get(const char *username, struct platform_state_user *state)
     return ret;
 }
 
-void
+static void
 platform_user_set(const struct platform_state_user *state)
 {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
@@ -130,7 +135,7 @@ platform_group_get(const char *groupname, struct platform_state_group *state)
     return ret;
 }
 
-void
+static void
 platform_group_set(const struct platform_state_group *state)
 {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
@@ -155,6 +160,104 @@ platform_group_set(const struct platform_state_group *state)
 #endif
 }
 
+/* Set user and group, retaining neccesary capabilities required by the platform.
+ *
+ * The keep_caps argument has 3 possible states:
+ *  >0: Retain capabilities, and fail hard on failure to do so.
+ * ==0: Don't attempt to retain any capabilities, just sitch user/group.
+ *  <0: Try to retain capabilities, but continue on failure.
+ */
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps)
+{
+    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
+#ifdef HAVE_LIBCAPNG
+    int new_gid = -1, new_uid = -1;
+    int res;
+
+    if (keep_caps == 0)
+    {
+        goto fallback;
+    }
+
+    /*
+     * new_uid/new_gid defaults to -1, which will not make
+     * libcap-ng change the UID/GID unless configured
+     */
+    if (group_state->groupname && group_state->gr)
+    {
+        new_gid = group_state->gr->gr_gid;
+    }
+    if (user_state->username && user_state->pw)
+    {
+        new_uid = user_state->pw->pw_uid;
+    }
+
+    /* Prepare capabilities before dropping UID/GID */
+    capng_clear(CAPNG_SELECT_BOTH);
+    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
+    if (res < 0)
+    {
+        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
+        goto fallback;
+    }
+
+    /* Change to new UID/GID.
+     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
+     */
+    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
+    if (res == -4 || res == -6)
+    {
+        /* -4 and -6 mean failure of setuid/gid respectively.
+           There is no point for us to continue if those failed. */
+        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
+            user_state->username, group_state->groupname, res);
+    }
+    else if (res == -3)
+    {
+        msg(M_NONFATAL | M_ERRNO, "capng_change_id() failed applying capabilities");
+        msg(err_flags, "NOTE: previous error likely due to missing capability CAP_SETPCAP.");
+        goto fallback;
+    }
+    else if (res < 0)
+    {
+        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
+            user_state->username, group_state->groupname, res);
+        goto fallback;
+    }
+
+    if (new_uid >= 0)
+    {
+         msg(M_INFO, "UID set to %s", user_state->username);
+    }
+    if (new_gid >= 0)
+    {
+         msg(M_INFO, "GID set to %s", group_state->groupname);
+    }
+
+    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
+    return;
+
+fallback:
+    /* capng_change_id() can leave this flag clobbered on failure
+     * This is working around a bug in libcap-ng, which can leave the flag set
+     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
+    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
+    {
+        msg(M_ERR, "Clearing KEEPCAPS flag failed");
+    }
+#endif  /* HAVE_LIBCAPNG */
+
+    if (keep_caps)
+    {
+        msg(err_flags, "Unable to retain capabilities");
+    }
+
+    platform_group_set(group_state);
+    platform_user_set(user_state);
+}
+
 /* Change process priority */
 void
 platform_nice(int niceval)
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a3eec298..19187d3a 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -79,11 +79,12 @@ 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);
-
 bool platform_group_get(const char *groupname, struct platform_state_group *state);
 
-void platform_group_set(const struct platform_state_group *state);
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             int keep_caps);
+
 
 /*
  * Extract UID or GID
-- 
2.25.1

[Openvpn-devel] [PATCH v5] platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On Linux, when dropping privileges, interaction with
the network configuration, such as tearing down routes
or ovpn-dco interfaces will fail when --user/--group are
used.

This patch sets the CAP_NET_ADMIN capability, which grants
the needed privileges during the lifetime of the OpenVPN
process when dropping root privileges.

Signed-off-by: Timo Rothenpieler <timo@...2669...>
Reviewed-By: David Sommerseth <davids@...515...>
---
 configure.ac                              |  19 +++
 distro/systemd/openvpn-client@...2221... |   2 +-
 distro/systemd/openvpn-server@...2221... |   2 +-
 src/openvpn/init.c                        |   5 +-
 src/openvpn/platform.c                    | 146 +++++++++++++++++++++-
 src/openvpn/platform.h                    |  10 +-
 6 files changed, 175 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 85921ddb..d2eb3426 100644
--- a/configure.ac
+++ b/configure.ac
@@ -794,6 +794,25 @@ dnl
 	esac
 fi
 
+dnl
+dnl Depend on libcap-ng on Linux
+dnl
+case "$host" in
+	*-*-linux*)
+		PKG_CHECK_MODULES([LIBCAPNG],
+				  [libcap-ng],
+				  [],
+				  [AC_MSG_ERROR([libcap-ng package not found. Is the development package and pkg-config installed?])]
+		)
+		AC_CHECK_HEADER([sys/prctl.h],,[AC_MSG_ERROR([sys/prctl.h not found!])])
+
+		CFLAGS="${CFLAGS} ${LIBCAPNG_CFALGS}"
+		LIBS="${LIBS} ${LIBCAPNG_LIBS}"
+		AC_DEFINE(HAVE_LIBCAPNG, 1, [Enable libcap-ng support])
+	;;
+esac
+
+
 if test "${with_crypto_library}" = "openssl"; then
 	AC_ARG_VAR([OPENSSL_CFLAGS], [C compiler flags for OpenSSL])
 	AC_ARG_VAR([OPENSSL_LIBS], [linker flags for OpenSSL])
diff --git a/distro/systemd/openvpn-client@...2221... b/distro/systemd/openvpn-client@...2221...
index cbcef653..159fb4dc 100644
--- a/distro/systemd/openvpn-client@...2221...
+++ b/distro/systemd/openvpn-client@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/client
 ExecStart=@sbindir@/openvpn --suppress-timestamps --nobind --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/distro/systemd/openvpn-server@...2221... b/distro/systemd/openvpn-server@...2221...
index d1cc72cb..6e8e7d94 100644
--- a/distro/systemd/openvpn-server@...2221...
+++ b/distro/systemd/openvpn-server@...2221...
@@ -11,7 +11,7 @@ Type=notify
 PrivateTmp=true
 WorkingDirectory=/etc/openvpn/server
 ExecStart=@sbindir@/openvpn --status %t/openvpn-server/status-%i.log --status-version 2 --suppress-timestamps --config %i.conf
-CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
+CapabilityBoundingSet=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SETPCAP CAP_SYS_CHROOT CAP_DAC_OVERRIDE CAP_AUDIT_WRITE
 LimitNPROC=10
 DeviceAllow=/dev/null rw
 DeviceAllow=/dev/net/tun rw
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index a6c93038..39b8f77a 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1163,8 +1163,9 @@ do_uid_gid_chroot(struct context *c, bool no_delay)
         {
             if (no_delay)
             {
-                platform_group_set(&c0->platform_state_group);
-                platform_user_set(&c0->platform_state_user);
+                platform_user_group_set(&c0->platform_state_user,
+                                        &c0->platform_state_group,
+                                        c);
             }
             else if (c->first_time)
             {
diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index 61afee83..56dca6e6 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -29,6 +29,9 @@
 
 #include "syshead.h"
 
+#include "openvpn.h"
+#include "options.h"
+
 #include "buffer.h"
 #include "crypto.h"
 #include "error.h"
@@ -43,6 +46,11 @@
 #include <direct.h>
 #endif
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#include <sys/prctl.h>
+#endif
+
 /* Redefine the top level directory of the filesystem
  * to restrict access to files for security */
 void
@@ -91,7 +99,7 @@ platform_user_get(const char *username, struct platform_state_user *state)
     return ret;
 }
 
-void
+static void
 platform_user_set(const struct platform_state_user *state)
 {
 #if defined(HAVE_GETPWNAM) && defined(HAVE_SETUID)
@@ -130,7 +138,7 @@ platform_group_get(const char *groupname, struct platform_state_group *state)
     return ret;
 }
 
-void
+static void
 platform_group_set(const struct platform_state_group *state)
 {
 #if defined(HAVE_GETGRNAM) && defined(HAVE_SETGID)
@@ -155,6 +163,140 @@ platform_group_set(const struct platform_state_group *state)
 #endif
 }
 
+/*
+ * 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.
+ *
+ * Returns the tri-state expected by platform_user_group_set.
+ * -1: try to keep caps, but continue if impossible
+ *  0: don't keep caps
+ *  1: keep caps, fail hard if impossible
+ */
+static int
+need_keep_caps(struct context *c)
+{
+    if (!c)
+    {
+        return -1;
+    }
+
+    if (dco_enabled(&c->options))
+    {
+#ifdef TARGET_LINUX
+        /* DCO on Linux does not work at all without CAP_NET_ADMIN */
+        return 1;
+#else
+        /* Windows/BSD/... has no equivalent capability mechanism */
+        return -1;
+#endif
+    }
+
+#ifdef ENABLE_SITNL
+    return -1;
+#else
+    return 0;
+#endif
+}
+
+/* Set user and group, retaining neccesary capabilities required by the platform.
+ *
+ * The keep_caps argument has 3 possible states:
+ *  >0: Retain capabilities, and fail hard on failure to do so.
+ * ==0: Don't attempt to retain any capabilities, just sitch user/group.
+ *  <0: Try to retain capabilities, but continue on failure.
+ */
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             struct context *c)
+{
+    int keep_caps = need_keep_caps(c);
+    unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
+#ifdef HAVE_LIBCAPNG
+    int new_gid = -1, new_uid = -1;
+    int res;
+
+    if (keep_caps == 0)
+    {
+        goto fallback;
+    }
+
+    /*
+     * new_uid/new_gid defaults to -1, which will not make
+     * libcap-ng change the UID/GID unless configured
+     */
+    if (group_state->groupname && group_state->gr)
+    {
+        new_gid = group_state->gr->gr_gid;
+    }
+    if (user_state->username && user_state->pw)
+    {
+        new_uid = user_state->pw->pw_uid;
+    }
+
+    /* Prepare capabilities before dropping UID/GID */
+    capng_clear(CAPNG_SELECT_BOTH);
+    res = capng_update(CAPNG_ADD, CAPNG_EFFECTIVE | CAPNG_PERMITTED, CAP_NET_ADMIN);
+    if (res < 0)
+    {
+        msg(err_flags, "capng_update(CAP_NET_ADMIN) failed: %d", res);
+        goto fallback;
+    }
+
+    /* Change to new UID/GID.
+     * capng_change_id() internally calls capng_apply() to apply prepared capabilities.
+     */
+    res = capng_change_id(new_uid, new_gid, CAPNG_DROP_SUPP_GRP | CAPNG_CLEAR_BOUNDING);
+    if (res == -4 || res == -6)
+    {
+        /* -4 and -6 mean failure of setuid/gid respectively.
+           There is no point for us to continue if those failed. */
+        msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
+            user_state->username, group_state->groupname, res);
+    }
+    else if (res == -3)
+    {
+        msg(M_NONFATAL | M_ERRNO, "capng_change_id() failed applying capabilities");
+        msg(err_flags, "NOTE: previous error likely due to missing capability CAP_SETPCAP.");
+        goto fallback;
+    }
+    else if (res < 0)
+    {
+        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
+            user_state->username, group_state->groupname, res);
+        goto fallback;
+    }
+
+    if (new_uid >= 0)
+    {
+         msg(M_INFO, "UID set to %s", user_state->username);
+    }
+    if (new_gid >= 0)
+    {
+         msg(M_INFO, "GID set to %s", group_state->groupname);
+    }
+
+    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
+    return;
+
+fallback:
+    /* capng_change_id() can leave this flag clobbered on failure
+     * This is working around a bug in libcap-ng, which can leave the flag set
+     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
+    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
+    {
+        msg(M_ERR, "Clearing KEEPCAPS flag failed");
+    }
+#endif  /* HAVE_LIBCAPNG */
+
+    if (keep_caps)
+    {
+        msg(err_flags, "Unable to retain capabilities");
+    }
+
+    platform_group_set(group_state);
+    platform_user_set(user_state);
+}
+
 /* Change process priority */
 void
 platform_nice(int niceval)
diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h
index a3eec298..1ffd81e3 100644
--- a/src/openvpn/platform.h
+++ b/src/openvpn/platform.h
@@ -55,6 +55,9 @@
 #include "basic.h"
 #include "buffer.h"
 
+/* forward declared to avoid large amounts of extra includes */
+struct context;
+
 /* Get/Set UID of process */
 
 struct platform_state_user {
@@ -79,11 +82,12 @@ 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);
-
 bool platform_group_get(const char *groupname, struct platform_state_group *state);
 
-void platform_group_set(const struct platform_state_group *state);
+void platform_user_group_set(const struct platform_state_user *user_state,
+                             const struct platform_state_group *group_state,
+                             struct context *c);
+
 
 /*
  * Extract UID or GID
-- 
2.25.1

Re: [Openvpn-devel] [PATCH v5] platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 14/05/2022 12:37, Timo Rothenpieler wrote:
> On Linux, when dropping privileges, interaction with
> the network configuration, such as tearing down routes
> or ovpn-dco interfaces will fail when --user/--group are
> used.
> 
> This patch sets the CAP_NET_ADMIN capability, which grants
> the needed privileges during the lifetime of the OpenVPN
> process when dropping root privileges.
> 
> Signed-off-by: Timo Rothenpieler <timo@...2669...>
> Reviewed-By: David Sommerseth <davids@...515...>

With Linux DCO now fully merged, this patch is ready to land.
It still applies cleanly for all I can tell.

Re: [Openvpn-devel] [PATCH v5] platform: Retain CAP_NET_ADMIN when dropping privileges

[Frank Lichtenheld]

On Sat, May 14, 2022 at 12:37:17PM +0200, Timo Rothenpieler wrote:
> On Linux, when dropping privileges, interaction with
> the network configuration, such as tearing down routes
> or ovpn-dco interfaces will fail when --user/--group are
> used.
> 
> This patch sets the CAP_NET_ADMIN capability, which grants
> the needed privileges during the lifetime of the OpenVPN
> process when dropping root privileges.
> 
> Signed-off-by: Timo Rothenpieler <timo@...2669...>
> Reviewed-By: David Sommerseth <davids@...515...>
> ---
>  configure.ac                              |  19 +++
>  distro/systemd/openvpn-client@...2221... |   2 +-
>  distro/systemd/openvpn-server@...2221... |   2 +-
>  src/openvpn/init.c                        |   5 +-
>  src/openvpn/platform.c                    | 146 +++++++++++++++++++++-
>  src/openvpn/platform.h                    |  10 +-
>  6 files changed, 175 insertions(+), 9 deletions(-)


I ran several t_client test runs with --user nobody on a DCO-enabled system.

Without the patch:
 - errors on teardown in all tests (sitnl)
 - test 11 fails (which actually uses DCO, since no comp)

With the patch:
 - errors on teardown gone
 - test 11 passes

With the patch and --disable-dco --enable-iproute2:
 - no cap retained
 - errors on teardown (ip)

Looks to me like it does what it is supposed to do.

Acked-By: Frank Lichtenheld <frank@...2641...>

That said, maybe we should add some hint about this
behavior to the actual documentation? Maybe to
--user documentation? Or at least Changes?

Regards,
-- 
  Frank Lichtenheld

[Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

I have not tested this myself, but if I had, the test setup would have
been very similar to what Frank did (so, big thanks) - run a DCO 
environment with "owner nobody", and see if things still work.

I will add this to my DCO server test environment - run one of the
iroute-using instances with "nobody", so it is continuously tested.

I did have a stare-at-code a few weeks ago, and we did discuss this
a few months ago, and the approach chosen seems to make sense.

Uncrustify complained about two lines with tabs -> fixed.

Your patch has been applied to the master branch.

commit 2e359a088226ab1e5ee41fbab27d38d8a8d192ac
Author: Timo Rothenpieler
Date:   Sat May 14 12:37:17 2022 +0200

     platform: Retain CAP_NET_ADMIN when dropping privileges

     Signed-off-by: Timo Rothenpieler <timo@...2669...>
     Acked-by: Frank Lichtenheld <frank@...2641...>
     Message-Id: <20220514103717.235-1-timo@...2669...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24360.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1.1: Type: text/plain, Size: 1019 bytes --]

Hi,

On Thu, Aug 11, 2022 at 12:03:45PM +0200, Gert Doering wrote:
> Uncrustify complained about two lines with tabs -> fixed.

Turns out that it's actually 4 lines, and while I did "git apply $patch",
I forgot the "git commit --amend platform.c", so the whitespace errors
landed in the commit, and the *fix* was still sitting in my tree.

Since this is just whitespace, I'll merge the patch as follows, 
into 

commit 649874df9edb52a9d85bf9db690b6150fdb6dcc9 (HEAD -> master)
Author: Gert Doering <gert@...1296...>
Date:   Thu Aug 11 13:26:58 2022 +0200

    Apply uncrustify changes that forgotten in the last patch.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #1.2: 0001-Apply-uncrustify-changes-that-forgotten-in-the-last-.patch --]
[-- Type: text/x-diff, Size: 2431 bytes --]

From 649874df9edb52a9d85bf9db690b6150fdb6dcc9 Mon Sep 17 00:00:00 2001
From: Gert Doering <gert@...1296...>
Date: Thu, 11 Aug 2022 13:26:58 +0200
Subject: [PATCH] Apply uncrustify changes that forgotten in the last patch.

commit 2e359a088226ab1e5 has a few whitespace errors that uncrustify
complained on merge, but due to git handling mistakes, these were not
properly included in the actual commit.  Fix.

Signed-off-by: Gert Doering <gert@...1296...>
---
 src/openvpn/platform.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c
index e6778b3f..327a2d23 100644
--- a/src/openvpn/platform.c
+++ b/src/openvpn/platform.c
@@ -205,9 +205,10 @@ need_keep_caps(struct context *c)
  * ==0: Don't attempt to retain any capabilities, just sitch user/group.
  *  <0: Try to retain capabilities, but continue on failure.
  */
-void platform_user_group_set(const struct platform_state_user *user_state,
-                             const struct platform_state_group *group_state,
-                             struct context *c)
+void
+platform_user_group_set(const struct platform_state_user *user_state,
+                        const struct platform_state_group *group_state,
+                        struct context *c)
 {
     int keep_caps = need_keep_caps(c);
     unsigned int err_flags = (keep_caps > 0) ? M_FATAL : M_NONFATAL;
@@ -249,7 +250,7 @@ void platform_user_group_set(const struct platform_state_user *user_state,
     if (res == -4 || res == -6)
     {
         /* -4 and -6 mean failure of setuid/gid respectively.
-           There is no point for us to continue if those failed. */
+         * There is no point for us to continue if those failed. */
         msg(M_ERR, "capng_change_id('%s','%s') failed: %d",
             user_state->username, group_state->groupname, res);
     }
@@ -268,11 +269,11 @@ void platform_user_group_set(const struct platform_state_user *user_state,
 
     if (new_uid >= 0)
     {
-         msg(M_INFO, "UID set to %s", user_state->username);
+        msg(M_INFO, "UID set to %s", user_state->username);
     }
     if (new_gid >= 0)
     {
-         msg(M_INFO, "GID set to %s", group_state->groupname);
+        msg(M_INFO, "GID set to %s", group_state->groupname);
     }
 
     msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
-- 
2.37.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

HI,

On Thu, Aug 11, 2022 at 12:03:45PM +0200, Gert Doering wrote:
> I have not tested this myself, but if I had, the test setup would have
> been very similar to what Frank did (so, big thanks) - run a DCO 
> environment with "owner nobody", and see if things still work.
> 
> I will add this to my DCO server test environment - run one of the
> iroute-using instances with "nobody", so it is continuously tested.
[..]
> commit 2e359a088226ab1e5ee41fbab27d38d8a8d192ac
> Author: Timo Rothenpieler
> Date:   Sat May 14 12:37:17 2022 +0200
> 
>      platform: Retain CAP_NET_ADMIN when dropping privileges

Unfortunately, it seems that our approach to "if SITNL is used, we hard
require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
ways that people use openvpn.

Namely, network-manager...

  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017379

... runs openvpn with --ifconfig-noexec / --route-noexec, and insists
on doing all that itself.  I do not like NM's way of trying to control
everything (up to the point that it defaults to redirecting a default
route to OpenVPN even if config and server do not want that), but this
is what Linux people seem to be stuck with, so we need to handle it.


So I think we need to amend this patch twofold

 - if --ifconfig-noexec && --route-noexec are set, do not mandate
   success on CAP_NET_ADMIN (users might want it for using it in --up
   scripts, but if it fails, *openvpn* is not missing functionality)

   --> this should take care of the NM case

 - also, we might want to think long and hard about mandating it if
   --client (that is, --pull) is in use.  We postpone dropping of privileges
   until after the initial ifconfig/route setup has been done, and on
   program end, closing tun/dco interface will (I hope) make the interface
   go away without needing privileges, and the system will remove the 
   routes together with it.

   --redirect-gateway will break, --redirect-gateway def1 will not.

   Overlapping vpn routes with vpn gateway (= install a host route) will
   also be unable to clean up at program end.  But this is no worse than
   2.5.x with --user nobody.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 15/08/2022 11:54, Gert Doering wrote:
> HI,
> 
> On Thu, Aug 11, 2022 at 12:03:45PM +0200, Gert Doering wrote:
>> I have not tested this myself, but if I had, the test setup would have
>> been very similar to what Frank did (so, big thanks) - run a DCO
>> environment with "owner nobody", and see if things still work.
>>
>> I will add this to my DCO server test environment - run one of the
>> iroute-using instances with "nobody", so it is continuously tested.
> [..]
>> commit 2e359a088226ab1e5ee41fbab27d38d8a8d192ac
>> Author: Timo Rothenpieler
>> Date:   Sat May 14 12:37:17 2022 +0200
>>
>>       platform: Retain CAP_NET_ADMIN when dropping privileges
> 
> Unfortunately, it seems that our approach to "if SITNL is used, we hard
> require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
> ways that people use openvpn.

That's not how the patch operates.
It only hard-requires the capability retention is dco_enabled() returns 
true.
In all other cases, it will try to retain capabilities, but continue 
with a warning if it fails.

Making the dco_enabled() case a "try but continue" would be a matter of 
changing a 1 to a -1. But given that DCO can't really work then, I'm not 
sure if that's desirable.

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]

Hi,

On Mon, Aug 15, 2022 at 12:14:23PM +0200, Timo Rothenpieler wrote:
> > Unfortunately, it seems that our approach to "if SITNL is used, we hard
> > require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
> > ways that people use openvpn.
> 
> That's not how the patch operates.
> It only hard-requires the capability retention is dco_enabled() returns 
> true.
> In all other cases, it will try to retain capabilities, but continue 
> with a warning if it fails.

Yes, but we do have DCO here, setting CAP_NET_ADMIN fails, and we abort,
instead of having a working client connect.

> Making the dco_enabled() case a "try but continue" would be a matter of 
> changing a 1 to a -1. But given that DCO can't really work then, I'm not 
> sure if that's desirable.

*DCO* can work fine, from the looks of that bug report (because all
the DCO open happens before capability drop on a --client config).

"Calling ifconfig and installing routes" cannot, but this is exactly what
we are not doing if --ifconfig-noexec + --route-noexec are set.

Please look closely at the log in the bug report, and keep in mind
that NM will do all the "SITNL" stuff for the user in that scenaro,
not OpenVPN itself.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Timo Rothenpieler]

On 15/08/2022 12:29, Gert Doering wrote:
> Hi,
> 
> On Mon, Aug 15, 2022 at 12:14:23PM +0200, Timo Rothenpieler wrote:
>>> Unfortunately, it seems that our approach to "if SITNL is used, we hard
>>> require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
>>> ways that people use openvpn.
>>
>> That's not how the patch operates.
>> It only hard-requires the capability retention is dco_enabled() returns
>> true.
>> In all other cases, it will try to retain capabilities, but continue
>> with a warning if it fails.
> 
> Yes, but we do have DCO here, setting CAP_NET_ADMIN fails, and we abort,
> instead of having a working client connect.
> 
>> Making the dco_enabled() case a "try but continue" would be a matter of
>> changing a 1 to a -1. But given that DCO can't really work then, I'm not
>> sure if that's desirable.
> 
> *DCO* can work fine, from the looks of that bug report (because all
> the DCO open happens before capability drop on a --client config).
> 
> "Calling ifconfig and installing routes" cannot, but this is exactly what
> we are not doing if --ifconfig-noexec + --route-noexec are set.
> 
> Please look closely at the log in the bug report, and keep in mind
> that NM will do all the "SITNL" stuff for the user in that scenaro,
> not OpenVPN itself.
> 
> gert

There's basically two options then:

Remove the dco_enabled() check entirely, and purely check for SITNL, but 
always only warn if it fails and carry on, hoping it works fine.

Add checks for ifconfig-noexec + route-noexec being set, and either only 
warn in that case, or don't even try to retain capabilities, since 
they're not needed either way. I'd prefer the later, since fewer 
capabilities is generally better.

The later of the two options seems nicer, will have a look at 
implementing them. Shouldn't be that hard, given the config is already 
there.
I don't have a NM setup to test that on though.

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

Hi,

On Mon, Aug 15, 2022 at 12:40:55PM +0200, Timo Rothenpieler wrote:
> Add checks for ifconfig-noexec + route-noexec being set, and either only 
> warn in that case, 

... this is what I suggested two mails upthread :-)

> or don't even try to retain capabilities, since 
> they're not needed either way. I'd prefer the later, since fewer 
> capabilities is generally better.

I could see arguments for "we want to do the ifconfig/route setup in
an --up script" - for example to do VRF/NetNS stuff that OpenVPN can not
do itself.  So for these scenarios having the capability around would
be useful (= thus, try-and-warn)...

Different scenario, same options.  We don't always know what users want.

> implementing them. Shouldn't be that hard, given the config is already 
> there.
> I don't have a NM setup to test that on though.

Sending to the debian bug report referenced upthread and asking them
to test might be an option here.  Or ask David.

I do not use NM either.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Steffan Karger]

Hi,

On Mon, 15 Aug 2022 at 12:50, Gert Doering <gert@...1296...> wrote:
> On Mon, Aug 15, 2022 at 12:40:55PM +0200, Timo Rothenpieler wrote:
> > or don't even try to retain capabilities, since
> > they're not needed either way. I'd prefer the later, since fewer
> > capabilities is generally better.
>
> I could see arguments for "we want to do the ifconfig/route setup in
> an --up script" - for example to do VRF/NetNS stuff that OpenVPN can not
> do itself.  So for these scenarios having the capability around would
> be useful (= thus, try-and-warn)...
>
> Different scenario, same options.  We don't always know what users want.

Let me just second the "fewer capabilities is generally better"
argument. CAP_NET_ADMIN is a broad set of effective capabilties and
has been a popular path for privilege escalation vulnerabilities in
the past. See for example these two recent CVEs:

CVE-2022-2586

    A use-after-free in the Netfilter subsystem may result in local
    privilege escalation for a user with the CAP_NET_ADMIN capability in
    any user or network namespace.

CVE-2022-2588

    Zhenpeng Lin discovered a use-after-free flaw in the cls_route
    filter implementation which may result in local privilege escalation
    for a user with the CAP_NET_ADMIN capability in any user or network
    namespace.

So I'm really not in favour of retaining CAP_NET_ADMIN "just in case".
I would even like to be able to not retain it at all.

-Steffan

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

Hi,

On Tue, Aug 16, 2022 at 11:16:50AM +0200, Steffan Karger wrote:
> So I'm really not in favour of retaining CAP_NET_ADMIN "just in case".
> I would even like to be able to not retain it at all.

I hear what you say, and I support that line of thought.

Alas, with current Linux-DCO, we need this for fiddling all the DCO bits
(keys / key renewal! / peer-id / install/update peers), as this is done
via netlink, and netlink requires CAP_NET_ADMIN.

To get around that we'd need to find a communication channel to 
(Linux-)DCO that does not require privileges after initial setup 
("hey, DCO, give me a socket that does not need privileges") and 
rewrite quite a bit...  I'm sure that Antonio will be thrilled by
that new challenge... :-)

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

Hi,

On Mon, Aug 15, 2022 at 11:54:21AM +0200, Gert Doering wrote:
> [..]
> > commit 2e359a088226ab1e5ee41fbab27d38d8a8d192ac
> > Author: Timo Rothenpieler
> > Date:   Sat May 14 12:37:17 2022 +0200
> > 
> >      platform: Retain CAP_NET_ADMIN when dropping privileges
> 
> Unfortunately, it seems that our approach to "if SITNL is used, we hard
> require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
> ways that people use openvpn.
> 
> Namely, network-manager...
> 
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017379
> 
> ... runs openvpn with --ifconfig-noexec / --route-noexec, and insists
> on doing all that itself.  I do not like NM's way of trying to control
> everything (up to the point that it defaults to redirecting a default
> route to OpenVPN even if config and server do not want that), but this
> is what Linux people seem to be stuck with, so we need to handle it.

We've discussed this quite a bit in #openvpn-devel, and it's more complicated
than "just ifconfig and route".

Namely, there is more operations OpenVPN needs to do if running with
the DCO kernel module, which all needs CAP_NET_ADMIN

  - renegotiate on TLS session expiry (control channel communication is
    done through kernel module, not directly on socket)
  - install / swap keys
  - reconfigure the kernel peer on reconnect (peer-id / remote IP)

So, running in an environment that prevents use of CAP_NET_ADMIN (how
does it do that, in the first place?) *and* forces a non-root user
means "DCO will break".  Not immediately, but openvpn will abort on
each of these envents - NM might hide that by just restarting it, but
it's still broken.


So the discussion seems to run towards two options

  - disable DCO if CAP_NET_ADMIN can not be retained
  - fix this in NM

Preferably, fix this in NM...

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH applied] Re: platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Hi,

On Mon, Aug 15, 2022 at 11:54:21AM +0200, Gert Doering wrote:
> [..]
> > commit 2e359a088226ab1e5ee41fbab27d38d8a8d192ac
> > Author: Timo Rothenpieler
> > Date:   Sat May 14 12:37:17 2022 +0200
> > 
> >      platform: Retain CAP_NET_ADMIN when dropping privileges
> 
> Unfortunately, it seems that our approach to "if SITNL is used, we hard
> require that setting CAP_NET_ADMIN succeeds" is too strong for the twisted
> ways that people use openvpn.
> 
> Namely, network-manager...
> 
>   https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017379

For the sake of the list archives: *this* particular problem has been
solved by 

commit da31c1654c8534658157cfe9c9de5750ee752608
Author: Timo Rothenpieler <timo@...2669...>
Date:   Wed Aug 17 15:18:17 2022 +0200

    dco: disable DCO if --user specified but unable to retain capabilities


so if we detect "the caller wants us to go to --user $notroot but we do
not have the necessary capabilities to retain CAP_NET_ADMIN, disable DCO".

This is basically the only thing we can do - if we have no CAP_NET_ADMIN,
DCO will be unable to function today.


Next steps are

 - talk to the NM maintainers to get them to call OpenVPN with something
   like "CAP_NET_ADMIN and uid != 0" (and no --user config) - so we can
   just do our thing, without root privs.  David :-)

 - figure out if we can do Linux DCO without CAP_NET_ADMIN, at least 
   "after startup" (open with privs, get a ticket, continue without privs,
   something mumble mumble).  Antonio :-)

Thanks for all the enlightenment that happened here.

gert

-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]