diff mbox series

[Openvpn-devel] dco: disable DCO if --user specified but unable to retain capabilities

Message ID 20220817131817.467-1-timo@rothenpieler.org
State Accepted
Headers show
Series [Openvpn-devel] dco: disable DCO if --user specified but unable to retain capabilities | expand

Commit Message

Timo Rothenpieler Aug. 17, 2022, 3:18 a.m. UTC 
---
 src/openvpn/dco.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Gert Doering Aug. 17, 2022, 4:18 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks.  Bernhard has already tested this on his Debian/NM testing 
environment and confirms it fixes the issues seen in 

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

I've added a bit of explanatory text to the commit message (quite a 
bit of blurb, actually, but the issue was confusing us enough that
this might help future readers).

Your patch has been applied to the master branch.

commit da31c1654c8534658157cfe9c9de5750ee752608
Author: Timo Rothenpieler
Date:   Wed Aug 17 15:18:17 2022 +0200

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

     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220817131817.467-1-timo@rothenpieler.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24952.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Aug. 17, 2022, 9:07 a.m. UTC | #2 
Hi,

On Wed, Aug 17, 2022 at 04:18:39PM +0200, Gert Doering wrote:
> Acked-by: Gert Doering <gert@greenie.muc.de>
> 
> Thanks.  Bernhard has already tested this on his Debian/NM testing 
> environment and confirms it fixes the issues seen in 
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1017379
> 
> I've added a bit of explanatory text to the commit message (quite a 
> bit of blurb, actually, but the issue was confusing us enough that
> this might help future readers).
> 
> Your patch has been applied to the master branch.
> 
> commit da31c1654c8534658157cfe9c9de5750ee752608
> Author: Timo Rothenpieler
> Date:   Wed Aug 17 15:18:17 2022 +0200
> 
>      dco: disable DCO if --user specified but unable to retain capabilities

I really hate to say this... but... we seem to be calling this function
for incoming MULTI connects:

I have an UDP p2mp server with "--user nobody + DCO", which starts up 
fine (because it has root and can set CAP_NET_ADMIN), but on incoming
client connects, it goes there *again*, and then fails:

Aug 17 21:00:18 ubuntu2004 tun-udp-p2mp[2167001]: freebsd-14-amd64/194.97.140.5:23821 --user specified but lacking CAP_SETPCAP. Cannot retain CAP_NET_ADMIN. Disabling data channel offload
Aug 17 21:00:18 ubuntu2004 tun-udp-p2mp[2167001]: freebsd-14-amd64/194.97.140.5:23821 MULTI: client has been rejected due to incompatible DCO options
Aug 17 21:00:19 ubuntu2004 tun-udp-p2mp[2167001]: freebsd-14-amd64/194.97.140.5:23821 PUSH: Received control message: 'PUSH_REQUEST'
Aug 17 21:00:19 ubuntu2004 tun-udp-p2mp[2167001]: freebsd-14-amd64/194.97.140.5:23821 Delayed exit in 5 seconds
Aug 17 21:00:19 ubuntu2004 tun-udp-p2mp[2167001]: freebsd-14-amd64/194.97.140.5:23821 SENT CONTROL [freebsd-14-amd64]: 'AUTH_FAILED' (status=1)


This is hairy mess of checks, double checks, and too many checks...
(a quick fix would be to add "if (getuid() == 0 && o->user && ...)" to
ensure this is only called if we haven't already changed user id, but
it still feels wrong)

Better suggestions where to move "this is something we only care at
startup, not at per-instance config" checks?

gert
diff mbox series

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index caa4ce32..b7db23f4 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -44,6 +44,10 @@ 
 #include "ssl_ncp.h"
 #include "tun.h"
 
+#ifdef HAVE_LIBCAPNG
+#include <cap-ng.h>
+#endif
+
 static int
 dco_install_key(struct tls_multi *multi, struct key_state *ks,
                 const uint8_t *encrypt_key, const uint8_t *encrypt_iv,
@@ -247,6 +251,28 @@  dco_check_option_conflict_platform(int msglevel, const struct options *o)
         }
     }
 #endif /* if defined(TARGET_LINUX) */
+
+#if defined(HAVE_LIBCAPNG)
+    /* DCO can't operate without CAP_NET_ADMIN. To retain it when switching user
+     * we need CAP_SETPCAP. CAP_NET_ADMIN also needs to be part of the permitted set
+     * of capabilities in order to retain it.
+     */
+    if (o->username)
+    {
+        if (!capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP))
+        {
+            msg(msglevel, "--user specified but lacking CAP_SETPCAP. "
+                "Cannot retain CAP_NET_ADMIN. Disabling data channel offload");
+            return false;
+        }
+        if (!capng_have_capability(CAPNG_PERMITTED, CAP_NET_ADMIN))
+        {
+            msg(msglevel, "--user specified but not permitted to retain CAP_NET_ADMIN. "
+                "Disabling data channel offload");
+            return false;
+        }
+    }
+#endif /* if defined(HAVE_LIBCAPNG) */
     return true;
 }