| Message ID | 20180423092813.24844-1-list@eworm.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel,1/1] systemd: run openvpn with dedicated user | expand |
Hello, I haven't followed the netlink conversion in detail, so please tell me if the following was already discussed and I've just missed it. On Mon, Apr 23, 2018 at 11:28:13AM +0200, Christian Hesse wrote: > if ENABLE_SYSTEMD > +if ENABLE_IPROUTE > +SYSTEMD_USER=root > +SYSTEMD_CAPS_OPTION=CapabilityBoundingSet > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE > +else > +SYSTEMD_USER=openvpn > +SYSTEMD_CAPS_OPTION=AmbientCapabilities > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SYS_CHROOT CAP_DAC_OVERRIDE Are those capabilities dropped after initialization? If they are not this sounds like a serious issue as the process is basically running as root even if it's using another user (CAP_NET_ADMIN and CAP_DAC_OVERRIDE). Or am I missing something here? Regarding the netlink change in general: From what I understand it means that openvpn will always run with CAP_NET_ADMIN capabilities. Is this correct? If so, this sounds like it requires much more privileges than before for the normal operation (unless I misunderstand the current setup - to my knowledge it only requires a normal user after setup and no further capabilities or privileges once setup/connected). Regards Simon
Simon Ruderich <simon@ruderich.org> on Tue, 2018/04/24 10:38: > I haven't followed the netlink conversion in detail, so please > tell me if the following was already discussed and I've just > missed it. No, it has not been discussed and needs a review. > On Mon, Apr 23, 2018 at 11:28:13AM +0200, Christian Hesse wrote: > > if ENABLE_SYSTEMD > > +if ENABLE_IPROUTE > > +SYSTEMD_USER=root > > +SYSTEMD_CAPS_OPTION=CapabilityBoundingSet > > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE > > CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE +else > > +SYSTEMD_USER=openvpn > > +SYSTEMD_CAPS_OPTION=AmbientCapabilities > > +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE > > CAP_NET_RAW CAP_SYS_CHROOT CAP_DAC_OVERRIDE > > Are those capabilities dropped after initialization? If they are > not this sounds like a serious issue as the process is basically > running as root even if it's using another user (CAP_NET_ADMIN > and CAP_DAC_OVERRIDE). Or am I missing something here? > > Regarding the netlink change in general: From what I understand > it means that openvpn will always run with CAP_NET_ADMIN > capabilities. Is this correct? If so, this sounds like it > requires much more privileges than before for the normal > operation (unless I misunderstand the current setup - to my > knowledge it only requires a normal user after setup and no > further capabilities or privileges once setup/connected). The above snippet holds code for both, netlink and iproute2 versions. The iproute2 version (that is what is used currently) uses systemd option "CapabilityBoundingSet" to limit the capabilities to the given set. If configured openvpn will drop privileges after setup. With netlink and my patch on top we go the other way: The process runs (and is started) with user "openvpn". To grant required privileges we use systemd option "AmbientCapabilities" and give capabilities to the process. The process keeps these capabilities, but that's a benefit: The process survives a reconnect that requires configuration changes and shuts down cleanly (takes down routes and addresses). I do not agree that the process is running with root privileges. It has some extra capabilities, but it can not kill processes, fork away and change cgroups, etc. IMHO that is what we want to achieve. For this patch I took the current set of capabilities and stripped CAP_SETGID and CAP_SETUID for the netlink version. Whether or not the other capabilities are required should be discussed independently. Wondering why we have CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious indeed.
On Tue, Apr 24, 2018 at 12:03:37PM +0200, Christian Hesse wrote: > The above snippet holds code for both, netlink and iproute2 versions. > > The iproute2 version (that is what is used currently) uses systemd option > "CapabilityBoundingSet" to limit the capabilities to the given set. If > configured openvpn will drop privileges after setup. > > With netlink and my patch on top we go the other way: The process runs (and > is started) with user "openvpn". To grant required privileges we use > systemd option "AmbientCapabilities" and give capabilities to the process. > The process keeps these capabilities, but that's a benefit: The process > survives a reconnect that requires configuration changes and shuts down > cleanly (takes down routes and addresses). Hello Christian, Thanks for the confirmation, that's what I assumed. > I do not agree that the process is running with root privileges. It has some > extra capabilities, but it can not kill processes, fork away and change > cgroups, etc. > IMHO that is what we want to achieve. I disagree. A process with CAP_DAC_OVERRIDE can read/write _every_ file on the system (man 7 capabilities)! This equals root privileges. Even CAP_NET_ADMIN is very powerful as it allows modifying the firewall which might give access to sensitive services which are normally not exposed. > For this patch I took the current set of capabilities and stripped CAP_SETGID > and CAP_SETUID for the netlink version. Whether or not the other capabilities > are required should be discussed independently. Wondering why we have > CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious > indeed. Even with CAP_DAC_OVERRIDE stripped this change keeps openvpn running with (much) more privileges than before. Is this desirable? Regards Simon
Hi, On 24/04/18 21:08, Simon Ruderich wrote: >> I do not agree that the process is running with root privileges. It has some >> extra capabilities, but it can not kill processes, fork away and change >> cgroups, etc. >> IMHO that is what we want to achieve. > > I disagree. A process with CAP_DAC_OVERRIDE can read/write > _every_ file on the system (man 7 capabilities)! This equals root > privileges. Even CAP_NET_ADMIN is very powerful as it allows > modifying the firewall which might give access to sensitive > services which are normally not exposed. > >> For this patch I took the current set of capabilities and stripped CAP_SETGID >> and CAP_SETUID for the netlink version. Whether or not the other capabilities >> are required should be discussed independently. Wondering why we have >> CAP_DAC_OVERRIDE in our capability capability set... That looks suspicious >> indeed. > > Even with CAP_DAC_OVERRIDE stripped this change keeps openvpn > running with (much) more privileges than before. Is this > desirable? I think it depends on your perspective. What Christian says is that with this patch: 1) you can start openvpn as non-root directly (impossible right now) 2) you have full support for tunnel reconfiguration even when running as non-root (people willing to achieve this now must start and keep openvpn running as root) I consider both points above steps forward towards a better security model for OpenVPN. OTOH I understand that there are people that don't care about having a working tunnel reconfiguration and are fine with starting openvpn as root (and then dropping privileges). For these people, adding the above capabilities results in giving the openvpn process more power than before. Maybe users willing to adopt this stricter behaviour should have a knob somewhere that will enable the usual run-as-root-and-then-drop-priv-with-no-caps? Generally speaking I believe that openvpn, as a VPN and partly routing daemon, should be allowed to run with CAP_NET_ADMIN set as it enables more features (tunnel reconfiguration to start with). Maybe clients should run with the caps by default while servers should be launched with the original behaviour? Not sure, honestly. Cheers,
Antonio Quartulli <a@unstable.cc> on Tue, 2018/04/24 23:08: > OTOH I understand that there are people that don't care about having a > working tunnel reconfiguration and are fine with starting openvpn as > root (and then dropping privileges). > > For these people, adding the above capabilities results in giving the > openvpn process more power than before. > > Maybe users willing to adopt this stricter behaviour should have a knob > somewhere that will enable the usual > run-as-root-and-then-drop-priv-with-no-caps? NAK. :-p I think the solution for this dilemma is pretty easy: I should strip the part from my patch that disables user switching when started from systemd. We can start as user "openvpn" any way - as long as the process has capabilities CAP_SETGID and CAP_SETUID it still can switch user context and drop privileges. So users have two options: keep the process running as user "openvpn" with capabilities for more flexibility or switch to unprivileged user for more security. No need to have root involved. Sounds good?
Hi, On Tue, Apr 24, 2018 at 11:08:22PM +0800, Antonio Quartulli wrote: > Generally speaking I believe that openvpn, as a VPN and partly routing > daemon, should be allowed to run with CAP_NET_ADMIN set as it enables > more features (tunnel reconfiguration to start with). If we go there, we might consider dropping all non-default capabilities before running external programs (--up, --client-connect, ...) This patch is a bit "gallopping ahead", though - it needs a somewhat more solid agreement on our privilege model before modifying systemd unit files... gert
Hi,
On Tue, Apr 24, 2018 at 10:16:36PM +0200, Christian Hesse wrote:
> No need to have root involved. Sounds good?
This is not our traditional approach of "give people rope to hang themselves
if they want so". So I'll NAK any patch that *requires* use of systemd,
capabilities and non-root users on Linux.
As an option, welcome.
(I'm not finding a really good reason right now why I would need to run
openvpn as root which couldn't be done with CAP_NET_ADMIN, so the
recommendation should certainly be "use non-root" - but then, we're not
enforcing --setuid today either)
gert
Hi, On Mon, Apr 23, 2018 at 11:28:13AM +0200, Christian Hesse wrote: > @@ -1151,6 +1151,14 @@ do_uid_gid_chroot(struct context *c, bool no_delay) > /* set user and/or group if we want to setuid/setgid */ > if (c0->uid_gid_specified) > { > +#ifdef ENABLE_SYSTEMD > + if (sd_notify(0, "READY=0") > 0 && getuid() != 0) > + { > + msg(M_INFO, "NOTE: Running from systemd with non-root uid, skipping downgrade"); > + return; > + } > +#endif > + > if (no_delay) This is not a good approach. If you run with a limited capability model, then just do not set "setuid" in your config files, and document clearly that this is incompatible. But do not litter general-purpose code with #ifdef SYSTEMD bits that will randomly ignore user-specified options. gert
Hi, On Tue, Apr 24, 2018 at 4:16 PM, Christian Hesse <list@eworm.de> wrote: > Antonio Quartulli <a@unstable.cc> on Tue, 2018/04/24 23:08: >> OTOH I understand that there are people that don't care about having a >> working tunnel reconfiguration and are fine with starting openvpn as >> root (and then dropping privileges). >> >> For these people, adding the above capabilities results in giving the >> openvpn process more power than before. >> >> Maybe users willing to adopt this stricter behaviour should have a knob >> somewhere that will enable the usual >> run-as-root-and-then-drop-priv-with-no-caps? > > NAK. :-p > > I think the solution for this dilemma is pretty easy: I should strip the part > from my patch that disables user switching when started from systemd. We can > start as user "openvpn" any way - as long as the process has capabilities > CAP_SETGID and CAP_SETUID it still can switch user context and drop > privileges I'm not yet up to speed with capabilities in recent versions of linux, but some of the proposed ideas look unsafe to me. In particular: Be careful setting CAP_SETUID on the file without code changes. Once that is set, starting as root and dropping privileges is not the same as starting as non-root and dropping privileges. In the former case all capabilities will get cleared (which is good) but not so in the latter case. The process can elevate back to root in the second case which is bad. We'll need to add some code to clear all caps before the setuid() call to do this safely. Also, if the server is using pam for user-auth, the forked copy needs to run as root which is currently handled by forking before the setuid call. Need to consider how that will work with capabilities without handing out cap_setuid to all. As Gert said, looks like we are jumping the gun here. First thoroughly test and evaluate the nuances of the use of capabilities in context before deciding on how to make use of it. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/.gitignore b/.gitignore index 25009d81..00abdd5a 100644 --- a/.gitignore +++ b/.gitignore @@ -55,6 +55,7 @@ doc/openvpn.8.html /doc/doxygen/openvpn.doxyfile distro/rpm/openvpn.spec distro/systemd/*.service +distro/systemd/tmpfiles-openvpn.conf sample/sample-keys/sample-ca/ vendor/.build vendor/dist diff --git a/configure.ac b/configure.ac index 251cb9a2..ef8f5864 100644 --- a/configure.ac +++ b/configure.ac @@ -367,6 +367,7 @@ AC_ARG_VAR([GIT], [path to git utility]) AC_ARG_VAR([SYSTEMD_ASK_PASSWORD], [path to systemd-ask-password utility]) AC_ARG_VAR([SYSTEMD_UNIT_DIR], [Path of systemd unit directory @<:@default=LIBDIR/systemd/system@:>@]) AC_ARG_VAR([TMPFILES_DIR], [Path of tmpfiles directory @<:@default=LIBDIR/tmpfiles.d@:>@]) +AC_ARG_VAR([SYSUSERS_DIR], [Path of sysusers directory @<:@default=LIBDIR/sysusers.d@:>@]) AC_PATH_PROGS([IFCONFIG], [ifconfig],, [$PATH:/usr/local/sbin:/usr/sbin:/sbin]) AC_PATH_PROGS([ROUTE], [route],, [$PATH:/usr/local/sbin:/usr/sbin:/sbin]) AC_PATH_PROGS([IPROUTE], [ip],, [$PATH:/usr/local/sbin:/usr/sbin:/sbin]) @@ -1200,6 +1201,12 @@ if test "$enable_systemd" = "yes" ; then else tmpfilesdir="\${libdir}/tmpfiles.d" fi + + if test -n "${SYSUSERS_DIR}"; then + sysusersdir="${SYSUSERS_DIR}" + else + sysusersdir="\${libdir}/sysusers.d" + fi fi @@ -1375,6 +1382,7 @@ AM_CONDITIONAL([GIT_CHECKOUT], [test "${GIT_CHECKOUT}" = "yes"]) AM_CONDITIONAL([ENABLE_PLUGIN_AUTH_PAM], [test "${enable_plugin_auth_pam}" = "yes"]) AM_CONDITIONAL([ENABLE_PLUGIN_DOWN_ROOT], [test "${enable_plugin_down_root}" = "yes"]) AM_CONDITIONAL([HAVE_LD_WRAP_SUPPORT], [test "${have_ld_wrap_support}" = "yes"]) +AM_CONDITIONAL([ENABLE_IPROUTE], [test "${enable_iproute2}" = "yes"]) sampledir="\$(docdir)/sample" AC_SUBST([plugindir]) @@ -1382,6 +1390,7 @@ AC_SUBST([sampledir]) AC_SUBST([systemdunitdir]) AC_SUBST([tmpfilesdir]) +AC_SUBST([sysusersdir]) VENDOR_SRC_ROOT="\$(abs_top_srcdir)/vendor/" VENDOR_DIST_ROOT="\$(abs_top_builddir)/vendor/dist" diff --git a/distro/systemd/Makefile.am b/distro/systemd/Makefile.am index 69e12699..1b7ce5f9 100644 --- a/distro/systemd/Makefile.am +++ b/distro/systemd/Makefile.am @@ -10,14 +10,35 @@ %.service: %.service.in Makefile $(AM_V_GEN)sed -e 's|\@sbindir\@|$(sbindir)|' \ + -e 's|\@SYSTEMD_USER\@|$(SYSTEMD_USER)|' \ + -e 's|\@SYSTEMD_CAPS_OPTION\@|$(SYSTEMD_CAPS_OPTION)|' \ + -e 's|\@SYSTEMD_CAPS_VALUES\@|$(SYSTEMD_CAPS_VALUES)|' \ + $< > $@.tmp && mv $@.tmp $@ + +%.conf: %.conf.in Makefile + $(AM_V_GEN)sed -e 's|\@SYSTEMD_USER\@|$(SYSTEMD_USER)|g' \ $< > $@.tmp && mv $@.tmp $@ EXTRA_DIST = \ - tmpfiles-openvpn.conf \ + sysusers-openvpn.conf \ + tmpfiles-openvpn.conf.in \ openvpn-client@.service.in \ openvpn-server@.service.in if ENABLE_SYSTEMD +if ENABLE_IPROUTE +SYSTEMD_USER=root +SYSTEMD_CAPS_OPTION=CapabilityBoundingSet +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID CAP_SYS_CHROOT CAP_DAC_OVERRIDE +else +SYSTEMD_USER=openvpn +SYSTEMD_CAPS_OPTION=AmbientCapabilities +SYSTEMD_CAPS_VALUES=CAP_IPC_LOCK CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SYS_CHROOT CAP_DAC_OVERRIDE + +sysusers_DATA = \ + sysusers-openvpn.conf +endif + systemdunit_DATA = \ openvpn-client@.service \ openvpn-server@.service @@ -28,6 +49,7 @@ dist_doc_DATA = \ install-data-hook: mv $(DESTDIR)$(tmpfilesdir)/tmpfiles-openvpn.conf $(DESTDIR)$(tmpfilesdir)/openvpn.conf + mv $(DESTDIR)$(sysusersdir)/sysusers-openvpn.conf $(DESTDIR)$(sysusersdir)/openvpn.conf || true endif MAINTAINERCLEANFILES = \ diff --git a/distro/systemd/openvpn-client@.service.in b/distro/systemd/openvpn-client@.service.in index cbcef653..96cbf68e 100644 --- a/distro/systemd/openvpn-client@.service.in +++ b/distro/systemd/openvpn-client@.service.in @@ -9,9 +9,11 @@ Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO [Service] Type=notify PrivateTmp=true +User=@SYSTEMD_USER@ +Group=@SYSTEMD_USER@ 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 +@SYSTEMD_CAPS_OPTION@=@SYSTEMD_CAPS_VALUES@ LimitNPROC=10 DeviceAllow=/dev/null rw DeviceAllow=/dev/net/tun rw diff --git a/distro/systemd/openvpn-server@.service.in b/distro/systemd/openvpn-server@.service.in index a8366a04..3f00642e 100644 --- a/distro/systemd/openvpn-server@.service.in +++ b/distro/systemd/openvpn-server@.service.in @@ -9,9 +9,11 @@ Documentation=https://community.openvpn.net/openvpn/wiki/HOWTO [Service] Type=notify PrivateTmp=true +User=@SYSTEMD_USER@ +Group=@SYSTEMD_USER@ 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 +@SYSTEMD_CAPS_OPTION@=@SYSTEMD_CAPS_VALUES@ LimitNPROC=10 DeviceAllow=/dev/null rw DeviceAllow=/dev/net/tun rw diff --git a/distro/systemd/sysusers-openvpn.conf b/distro/systemd/sysusers-openvpn.conf new file mode 100644 index 00000000..d200852b --- /dev/null +++ b/distro/systemd/sysusers-openvpn.conf @@ -0,0 +1 @@ +u openvpn - "OpenVPN user" / diff --git a/distro/systemd/tmpfiles-openvpn.conf b/distro/systemd/tmpfiles-openvpn.conf deleted file mode 100644 index bb79671e..00000000 --- a/distro/systemd/tmpfiles-openvpn.conf +++ /dev/null @@ -1,2 +0,0 @@ -d /run/openvpn-client 0710 root root - -d /run/openvpn-server 0710 root root - diff --git a/distro/systemd/tmpfiles-openvpn.conf.in b/distro/systemd/tmpfiles-openvpn.conf.in new file mode 100644 index 00000000..f58d2967 --- /dev/null +++ b/distro/systemd/tmpfiles-openvpn.conf.in @@ -0,0 +1,4 @@ +d /run/openvpn-client 0750 @SYSTEMD_USER@ @SYSTEMD_USER@ - +d /run/openvpn-server 0750 @SYSTEMD_USER@ @SYSTEMD_USER@ - +d /etc/openvpn/client 0750 @SYSTEMD_USER@ @SYSTEMD_USER@ - +d /etc/openvpn/server 0750 @SYSTEMD_USER@ @SYSTEMD_USER@ - diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 36c1a4c4..0fc60d62 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1151,6 +1151,14 @@ do_uid_gid_chroot(struct context *c, bool no_delay) /* set user and/or group if we want to setuid/setgid */ if (c0->uid_gid_specified) { +#ifdef ENABLE_SYSTEMD + if (sd_notify(0, "READY=0") > 0 && getuid() != 0) + { + msg(M_INFO, "NOTE: Running from systemd with non-root uid, skipping downgrade"); + return; + } +#endif + if (no_delay) { platform_group_set(&c0->platform_state_group);