[PATCH 1/3] make teamd's run directory configurable

[Pawel Wieczorkiewicz]

Signed-off-by: Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;
Signed-off-by: Sebastian Krahmer <krahmer(a)suse.com&gt;
---
 configure.ac        | 8 ++++++++
 teamd/teamd.c       | 8 ++++----
 teamd/teamd.h       | 2 ++
 teamd/teamd_usock.c | 4 ----
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/configure.ac b/configure.ac
index 1115e95..302a11a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -101,6 +101,14 @@ AC_ARG_ENABLE([zmq],
 	fi
 fi
 
+AC_ARG_WITH([run-dir],
+	[AS_HELP_STRING([--with-run-dir@<:@=DIR@:>@], [Teamd run time directory
@<:@default=${localstatedir}@:>@])],
+	[case "$withval" in
+		yes|no|"") ;;
+		*) AC_DEFINE_UNQUOTED(TEAMD_RUN_DIR, "${withval}/", [TEAMD RUN DIR.]) ;;
+	esac]
+)
+
 AC_CHECK_PROGS([DOXYGEN], [doxygen])
 if test -z "$DOXYGEN";
 	then AC_MSG_WARN([Doxygen not found - continuing without Doxygen support])
diff --git a/teamd/teamd.c b/teamd/teamd.c
index 764e4f6..cdf1fc3 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -1621,10 +1621,6 @@ static int teamd_set_default_pid_file(struct teamd_context *ctx)
 	if (ctx->pid_file)
 		return 0;
 
-	err = teamd_make_rundir();
-	if (err)
-		return err;
-
 	err = asprintf(&ctx->pid_file, TEAMD_RUN_DIR"%s.pid",
ctx->team_devname);
 	if (err == -1) {
 		teamd_log_err("Failed allocate memory for PID file string.");
@@ -1676,6 +1672,10 @@ int main(int argc, char **argv)
 	int err;
 	struct teamd_context *ctx;
 
+	err = teamd_make_rundir();
+	if (err)
+		return ret;
+
 	err = teamd_context_init(&ctx);
 	if (err) {
 		fprintf(stderr, "Failed to init daemon context\n");
diff --git a/teamd/teamd.h b/teamd/teamd.h
index 845ca18..d8ab6c8 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -67,7 +67,9 @@ static inline void TEAMD_BUG_ON(bool condition)
 		TEAMD_BUG();
 }
 
+#ifndef TEAMD_RUN_DIR
 #define TEAMD_RUN_DIR "/var/run/teamd/"
+#endif
 
 static inline int teamd_make_rundir(void)
 {
diff --git a/teamd/teamd_usock.c b/teamd/teamd_usock.c
index 967f68c..c421c7a 100644
--- a/teamd/teamd_usock.c
+++ b/teamd/teamd_usock.c
@@ -298,10 +298,6 @@ static int teamd_usock_sock_open(struct teamd_context *ctx)
 	int sock;
 	int err;
 
-	err = teamd_make_rundir();
-	if (err)
-		return err;
-
 	sock = socket(AF_UNIX, SOCK_SEQPACKET, 0);
 	if (sock == -1) {
 		teamd_log_err("usock: Failed to create socket.");
-- 
2.1.4

[PATCH 2/3] drop privileges to usr/grp specified at build time

[Pawel Wieczorkiewicz]

From: Sebastian Krahmer <krahmer(a)suse.com&gt;

Signed-off-by: Sebastian Krahmer <krahmer(a)suse.com&gt;
Signed-off-by: Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;
---
 configure.ac  |  25 ++++++++++++++
 teamd/teamd.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 124 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 302a11a..f459f4e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,6 +77,31 @@ AS_IF([test "x$enable_debug" = "xyes"], [
 	AC_DEFINE(ENABLE_DEBUG, [1], [Debug messages.])
 ])
 
+user=root
+AC_ARG_WITH([user],
+	[AS_HELP_STRING([--with-user@<:@=username@:>@], [Set default daemon user
@<:@default=root@:>@])],
+	[case "$withval" in
+		yes|no|"") ;;
+		*) user="$withval" ;;
+	esac]
+)
+AC_DEFINE_UNQUOTED(TEAMD_USER, "${user}", [TEAMD USER.])
+
+group=root
+AC_ARG_WITH([group],
+	[AS_HELP_STRING([--with-group@<:@=groupname@:>@], [Specify the system group
@<:@default=root@:>@])],
+	[case "$withval" in
+		yes|no|"") ;;
+		*) group="$withval" ;;
+	esac]
+)
+AC_DEFINE_UNQUOTED(TEAMD_GROUP, "${group}", [TEAMD GROUP.])
+
+AM_CONDITIONAL([LIBCAP], [test "x$user" != "xroot" -o
"x$group" != "xroot"])
+AM_COND_IF([LIBCAP],
+	[AC_CHECK_LIB([cap], [cap_init],,
+		AC_MSG_ERROR([*** Non-root daemon support requested but libcap-devel library not
found]))])
+
 have_dbus=no
 AC_ARG_ENABLE([dbus],
 	AS_HELP_STRING([--disable-dbus], [disable D-Bus API @<:@default=enabled@:>@]))
diff --git a/teamd/teamd.c b/teamd/teamd.c
index cdf1fc3..8b68abb 100644
--- a/teamd/teamd.c
+++ b/teamd/teamd.c
@@ -1447,10 +1447,8 @@ static int teamd_start(struct teamd_context *ctx, enum
teamd_exit_code *p_ret)
 	pid_t pid;
 	int err = 0;
 
-	if (getuid() != 0) {
-		teamd_log_err("This program is intended to be run as root.");
-		return -EPERM;
-	}
+	if (getuid() == 0)
+		teamd_log_warn("This program is not intended to be run as root.");
 
 	if (daemon_reset_sigs(-1) < 0) {
 		teamd_log_err("Failed to reset all signal handlers.");
@@ -1666,6 +1664,99 @@ static void teamd_context_fini(struct teamd_context *ctx)
 	free(ctx);
 }
 
+
+#ifdef HAVE_LIBCAP
+#include <sys/prctl.h>
+#include <sys/capability.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include <grp.h>
+
+#ifndef TEAMD_USER
+#define TEAMD_USER "root"
+#endif
+#ifndef TEAMD_GROUP
+#define TEAMD_GROUP "root"
+#endif
+
+int drop_privileges()
+{
+	struct passwd *pw = NULL;
+	struct group *grpent = NULL;
+
+	if ((pw = getpwnam(TEAMD_USER)) == NULL) {
+		fprintf(stderr, "Error reading user %s entry (%m)\n", TEAMD_USER);
+		goto error;
+	}
+
+	if (pw->pw_uid == 0)
+		return 0;
+
+	if ((grpent = getgrnam(TEAMD_GROUP)) == NULL) {
+		fprintf(stderr, "Error reading group %s entry (%m)\n", TEAMD_GROUP);
+		goto error;
+	}
+
+	if (pw->pw_gid != grpent->gr_gid) {
+		fprintf(stderr, "%s GID (%u) does not match %s GID (%u)\n",
+			TEAMD_USER, pw->pw_gid, TEAMD_GROUP, grpent->gr_gid);
+		goto error;
+	}
+
+	if (chown(TEAMD_RUN_DIR, pw->pw_uid, pw->pw_gid) < 0) {
+		fprintf(stderr, "Unable to change ownership of %s to %s/%s (%m)\n",
+			TEAMD_RUN_DIR, TEAMD_USER, TEAMD_GROUP);
+		goto error;
+	}
+
+	if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) < 0)
+		goto error;
+
+	if (setgid(pw->pw_gid) < 0) {
+		fprintf(stderr, "Unable to set process GID to %u (%m)\n", pw->pw_gid);
+		goto error;
+	}
+
+	if (initgroups(TEAMD_USER, pw->pw_gid) < 0) {
+		fprintf(stderr, "Unable to initialize the group access list for %s user with GID
%u (%m)\n",
+			TEAMD_USER, pw->pw_gid);
+		goto error;
+	}
+	if (setuid(pw->pw_uid) < 0) {
+		fprintf(stderr, "Unable to set UID to %u (%m)\n", pw->pw_uid);
+		goto error;
+	}
+
+	cap_t my_caps;
+	cap_value_t cv[] = {CAP_NET_ADMIN, CAP_NET_BIND_SERVICE};
+
+	if ((my_caps = cap_init()) == NULL)
+		goto error;
+	if (cap_set_flag(my_caps, CAP_EFFECTIVE, 2, cv, CAP_SET) < 0)
+		goto error;
+	if (cap_set_flag(my_caps, CAP_PERMITTED, 2, cv, CAP_SET) < 0)
+		goto error;
+	if (cap_set_proc(my_caps) < 0)
+		goto error;
+	cap_free(my_caps);
+
+	return 0;
+error:
+	fprintf(stderr, "Failed to drop privileges\n");
+	return -1;
+}
+
+
+#else
+
+int drop_privileges()
+{
+	return 0;
+}
+
+#endif
+
+
 int main(int argc, char **argv)
 {
 	enum teamd_exit_code ret = TEAMD_EXIT_FAILURE;
@@ -1676,6 +1767,10 @@ int main(int argc, char **argv)
 	if (err)
 		return ret;
 
+	err = drop_privileges();
+	if (err)
+		return ret;
+
 	err = teamd_context_init(&ctx);
 	if (err) {
 		fprintf(stderr, "Failed to init daemon context\n");
-- 
2.1.4

[PATCH 3/3] regenerate dbus policy file from template when user changed

[Pawel Wieczorkiewicz]

Signed-off-by: Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;
Signed-off-by: Sebastian Krahmer <krahmer(a)suse.com&gt;
---
 configure.ac        |  8 ++++++--
 teamd/teamd.conf.in | 17 +++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)
 create mode 100644 teamd/teamd.conf.in

diff --git a/configure.ac b/configure.ac
index f459f4e..ddfccde 100644
--- a/configure.ac
+++ b/configure.ac
@@ -78,11 +78,15 @@ AS_IF([test "x$enable_debug" = "xyes"], [
 ])
 
 user=root
+dbus_policy_template="teamd/teamd.conf.in"
+dbus_policy_file="teamd/dbus/teamd.conf"
 AC_ARG_WITH([user],
 	[AS_HELP_STRING([--with-user@<:@=username@:>@], [Set default daemon user
@<:@default=root@:>@])],
 	[case "$withval" in
-		yes|no|"") ;;
-		*) user="$withval" ;;
+		root|yes|no|"") ;;
+		*) user="$withval"
+		   sed -e "s|[@]teamd_user[@]|$user|g" \
+			< $dbus_policy_template > $dbus_policy_file ;;
 	esac]
 )
 AC_DEFINE_UNQUOTED(TEAMD_USER, "${user}", [TEAMD USER.])
diff --git a/teamd/teamd.conf.in b/teamd/teamd.conf.in
new file mode 100644
index 0000000..d1cb2a1
--- /dev/null
+++ b/teamd/teamd.conf.in
@@ -0,0 +1,17 @@
+<?xml version="1.0"?> <!--*-nxml-*-->
+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration
1.0//EN"
+	"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
+<busconfig>
+	<policy user="root">
+		<allow own_prefix="org.libteam.teamd"/>
+		<allow send_interface="org.libteam.teamd"/>
+	</policy>
+	<policy user="@teamd_user@">
+		<allow own_prefix="org.libteam.teamd"/>
+		<allow send_interface="org.libteam.teamd"/>
+	</policy>
+	<policy context="default">
+		<deny own_prefix="org.libteam.teamd"/>
+		<deny send_interface="org.libteam.teamd"/>
+	</policy>
+</busconfig>
-- 
2.1.4

[PATCH 3/3] regenerate dbus policy file from template when user changed

[Jiri Pirko]

Mon, Sep 14, 2015 at 02:49:08PM CEST, pwieczorkiewicz(a)suse.de wrote:
> Signed-off-by: Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;
> Signed-off-by: Sebastian Krahmer <krahmer(a)suse.com&gt;
> ---
>  configure.ac        |  8 ++++++--
>  teamd/teamd.conf.in | 17 +++++++++++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>  create mode 100644 teamd/teamd.conf.in
>
> diff --git a/configure.ac b/configure.ac
> index f459f4e..ddfccde 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -78,11 +78,15 @@ AS_IF([test "x$enable_debug" = "xyes"], [
>  ])
>  
>  user=root
> +dbus_policy_template="teamd/teamd.conf.in"
> +dbus_policy_file="teamd/dbus/teamd.conf"
>  AC_ARG_WITH([user],
>  	[AS_HELP_STRING([--with-user@<:@=username@:>@], [Set default daemon user
> @<:@default=root@:>@])],
>  	[case "$withval" in
> -		yes|no|"") ;;
> -		*) user="$withval" ;;
> +		root|yes|no|"") ;;
> +		*) user="$withval"
> +		   sed -e "s|[@]teamd_user[@]|$user|g" \
> +			< $dbus_policy_template > $dbus_policy_file ;;
>  	esac]
>  )
>  AC_DEFINE_UNQUOTED(TEAMD_USER, "${user}", [TEAMD USER.])
> diff --git a/teamd/teamd.conf.in b/teamd/teamd.conf.in
> new file mode 100644
> index 0000000..d1cb2a1
> --- /dev/null
> +++ b/teamd/teamd.conf.in
> @@ -0,0 +1,17 @@
> +<?xml version="1.0"?> <!--*-nxml-*-->
> +<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration
> 1.0//EN"
> +	"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
> +<busconfig>
> +	<policy user="root">
> +		<allow own_prefix="org.libteam.teamd"/>
> +		<allow send_interface="org.libteam.teamd"/>
> +	</policy>
> +	<policy user="@teamd_user@">
> +		<allow own_prefix="org.libteam.teamd"/>
> +		<allow send_interface="org.libteam.teamd"/>
> +	</policy> 

Ok, so by default, section for user root will be duplicated? That looks
a bit odd.

[PATCH 3/3] regenerate dbus policy file from template when user changed

[Pawel Wieczorkiewicz]

On Mon, 14 Sep 2015 15:13:55 +0200
Jiri Pirko <jiri(a)resnulli.us&gt; wrote:

<snip>
>  > user=root
>  >+dbus_policy_template="teamd/teamd.conf.in"
>  >+dbus_policy_file="teamd/dbus/teamd.conf"
>  > AC_ARG_WITH([user],
>  > 	[AS_HELP_STRING([--with-user@<:@=username@:>@], [Set
>  > default daemon user @<:@default=root@:>@])], [case "$withval" in
>  >-		yes|no|"") ;;
>  >-		*) user="$withval" ;;
>  >+		root|yes|no|"") ;;
>  >+		*) user="$withval"
>  >+		   sed -e "s|[@]teamd_user[@]|$user|g" \
>  >+			< $dbus_policy_template > 
<snip>
>  >--- /dev/null
>  >+++ b/teamd/teamd.conf.in
>  >@@ -0,0 +1,17 @@
>  >+<?xml version="1.0"?> <!--*-nxml-*-->
>  >+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus
>  >Configuration 1.0//EN"
>  >+	"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
>  >+<busconfig>
>  >+	<policy user="root">
>  >+		<allow own_prefix="org.libteam.teamd"/>
>  >+		<allow send_interface="org.libteam.teamd"/>
>  >+	</policy>
>  >+	<policy user="@teamd_user@">
>  >+		<allow own_prefix="org.libteam.teamd"/>
>  >+		<allow send_interface="org.libteam.teamd"/>
>  >+	</policy>
>  
>  
>  Ok, so by default, section for user root will be duplicated? That
>  looks a bit odd. 
No, by default when non-root user is not specified, nothing happens and
regular teamd/dbus/teamd.conf file remains.
When --with-user=$user is specified and it does not equal 'root' the
template is modified and replaces teamd/dbus/teamd.conf.

-- 
Best Regards,
Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;, Linux System Developer

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu,
Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 / 90409
Nürnberg / Germany / Phone: [phone] - 613

[Jiri Pirko]

Mon, Sep 14, 2015 at 02:49:06PM CEST, pwieczorkiewicz(a)suse.de wrote:
> Signed-off-by: Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;
> Signed-off-by: Sebastian Krahmer <krahmer(a)suse.com&gt;
> ---
>  configure.ac        | 8 ++++++++
>  teamd/teamd.c       | 8 ++++----
>  teamd/teamd.h       | 2 ++
>  teamd/teamd_usock.c | 4 ----
>  4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 1115e95..302a11a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -101,6 +101,14 @@ AC_ARG_ENABLE([zmq],
>  	fi
>  fi
>  
> +AC_ARG_WITH([run-dir],
> +	[AS_HELP_STRING([--with-run-dir@<:@=DIR@:>@], [Teamd run time directory
> @<:@default=${localstatedir}@:>@])],
> +	[case "$withval" in
> +		yes|no|"") ;;
> +		*) AC_DEFINE_UNQUOTED(TEAMD_RUN_DIR, "${withval}/", [TEAMD RUN DIR.]) ;;
> +	esac]
> +)
> +
>  AC_CHECK_PROGS([DOXYGEN], [doxygen])
>  if test -z "$DOXYGEN";
>  	then AC_MSG_WARN([Doxygen not found - continuing without Doxygen support])
> diff --git a/teamd/teamd.c b/teamd/teamd.c
> index 764e4f6..cdf1fc3 100644
> --- a/teamd/teamd.c
> +++ b/teamd/teamd.c
> @@ -1621,10 +1621,6 @@ static int teamd_set_default_pid_file(struct teamd_context *ctx)
>  	if (ctx->pid_file)
>  		return 0;
>  
> -	err = teamd_make_rundir();
> -	if (err)
> -		return err;
> -
>  	err = asprintf(&ctx->pid_file, TEAMD_RUN_DIR"%s.pid",
> ctx->team_devname);
>  	if (err == -1) {
>  		teamd_log_err("Failed allocate memory for PID file string.");
> @@ -1676,6 +1672,10 @@ int main(int argc, char **argv)
>  	int err;
>  	struct teamd_context *ctx;
>  
> +	err = teamd_make_rundir();
> +	if (err)
> +		return ret;
> + 

What's up with the teamd_make_rundir call move? Any comments to that at
least? I think is should be rather in a separate patch.

[Pawel Wieczorkiewicz]

On Mon, 14 Sep 2015 15:07:32 +0200
Jiri Pirko <jiri(a)resnulli.us&gt; wrote:

>  >--- a/teamd/teamd.c
>  >+++ b/teamd/teamd.c
>  >@@ -1621,10 +1621,6 @@ static int teamd_set_default_pid_file(struct
>  >teamd_context *ctx)
>  > 	if (ctx->pid_file)
>  > 		return 0;
>  > 
>  >-	err = teamd_make_rundir();
>  >-	if (err)
>  >-		return err;
>  >-
>  > 	err = asprintf(&ctx->pid_file, TEAMD_RUN_DIR"%s.pid",
>  > ctx->team_devname); if (err == -1) {
>  > 		teamd_log_err("Failed allocate memory for PID file
>  > string.");
>  >@@ -1676,6 +1672,10 @@ int main(int argc, char **argv)
>  > 	int err;
>  > 	struct teamd_context *ctx;
>  > 
>  >+	err = teamd_make_rundir();
>  >+	if (err)
>  >+		return ret;
>  >+
>  
>  
>  What's up with the teamd_make_rundir call move? Any comments to that
>  at least? I think is should be rather in a separate patch. 
The move was needed in order to assign proper owner to that dir before
dropping privileges. Since /var/run or /run is root owned dir such
chown operation is not possible afterward.
There were basically two calls to teamd_make_rundir() following each
other quite shortly. As far as I checked that calls could not happen
independently, thus I concluded that it would be more consistent if
there was a single call at the process start beginning since we might
need to adjust dir properties anyway.

-- 
Best Regards,
Pawel Wieczorkiewicz <pwieczorkiewicz(a)suse.de&gt;, Linux System Developer

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu,
Graham Norton, HRB 21284 (AG Nürnberg) Maxfeldstraße 5 / 90409
Nürnberg / Germany / Phone: [phone] - 613

[Jiri Pirko]

Mon, Sep 14, 2015 at 05:59:19PM CEST, pwieczorkiewicz(a)suse.de wrote:
> On Mon, 14 Sep 2015 15:07:32 +0200
> Jiri Pirko <jiri(a)resnulli.us&gt; wrote:
>
> > >--- a/teamd/teamd.c
> > >+++ b/teamd/teamd.c
> > >@@ -1621,10 +1621,6 @@ static int teamd_set_default_pid_file(struct
> > >teamd_context *ctx)
> > > 	if (ctx->pid_file)
> > > 		return 0;
> > > 
> > >-	err = teamd_make_rundir();
> > >-	if (err)
> > >-		return err;
> > >-
> > > 	err = asprintf(&ctx->pid_file, TEAMD_RUN_DIR"%s.pid",
> > > ctx->team_devname); if (err == -1) {
> > > 		teamd_log_err("Failed allocate memory for PID file
> > > string.");
> > >@@ -1676,6 +1672,10 @@ int main(int argc, char **argv)
> > > 	int err;
> > > 	struct teamd_context *ctx;
> > > 
> > >+	err = teamd_make_rundir();
> > >+	if (err)
> > >+		return ret;
> > >+
> > 
> > 
> > What's up with the teamd_make_rundir call move? Any comments to that
> > at least? I think is should be rather in a separate patch.
>
> The move was needed in order to assign proper owner to that dir before
> dropping privileges. Since /var/run or /run is root owned dir such
> chown operation is not possible afterward. 
Okay. Then the squash with this patch does not seem correct to me.


> There were basically two calls to teamd_make_rundir() following each
> other quite shortly. As far as I checked that calls could not happen
> independently, thus I concluded that it would be more consistent if
> there was a single call at the process start beginning since we might
> need to adjust dir properties anyway. 
True. Could you please reduce that into 1 call? That would be great.

Thanks!