Subject: [resent x2][iputils][patch 01-07] setuid/capabilities fixups

From: Yuriy Kaminskiy <[email-masked]>
Date: Thu, 21 Mar 2013 15:38:07 +0400

Yuriy Kaminskiy wrote:
> Yuriy Kaminskiy wrote:
>> Run ping, look at /proc/`pidof ping`/status -> capabilities are not
>> [permanently] dropped, some of uids are not dropped, etc. Fix assorted issues
>> with setuid and capabilities drop. Limited testing only, please review/check
>> carefully.

ping.
>>From fd8a9b6fa39a7fa926b105e477fc9a133b6ac862 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 01:43:01 +0400
Subject: [PATCH 1/7] arping, ping, ping6, traceroute2, clockdiff: drop fsuid
 at start

Prevent ignoring netfilter -m owner checks. With this patch ping
correctly blocked by iptables rules, e.g.:
$ sudo iptables -I OUTPUT -m owner --uid-owner $UID -j DROP
$ ping www.google.com
---
 arping.c      |    9 +++++++++
 clockdiff.c   |    8 ++++++++
 ping_common.c |    9 +++++++++
 traceroute6.c |    8 ++++++++
 4 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/arping.c b/arping.c
index 35408c1..bdf81e9 100644
--- a/arping.c
+++ b/arping.c
@@ -27,6 +27,9 @@
 #include <sys/prctl.h>
 #include <sys/capability.h>
 #endif
+#ifdef __linux__
+#include <sys/fsuid.h>
+#endif
 
 #include <netdb.h>
 #include <unistd.h>
@@ -229,6 +232,12 @@ int modify_capability_raw(int on)
 		perror("arping: setuid");
 		return -1;
 	}
+#ifdef __linux__
+	if (on) {
+	    /* FIXME: error handling? setfsuid() have weird return code */
+	    setfsuid(getuid());
+	}
+#endif
 #endif
 	return 0;
 }
diff --git a/clockdiff.c b/clockdiff.c
index 7c1ea1b..f12da2d 100644
--- a/clockdiff.c
+++ b/clockdiff.c
@@ -3,6 +3,9 @@
 #include <sys/param.h>
 #include <stdio.h>
 #include <unistd.h>
+#ifdef __linux__
+#include <sys/fsuid.h>
+#endif
 #include <stdlib.h>
 #include <math.h>
 #include <string.h>
@@ -561,6 +564,11 @@ main(int argc, char *argv[])
 		usage();
 	}
 
+#ifdef __linux__
+	// FIXME: error handling? setfsuid() have weird return code
+	setfsuid(getuid());
+#endif
+
 	sock_raw = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
 	s_errno = errno;
 
diff --git a/ping_common.c b/ping_common.c
index 8d6b145..1d0012a 100644
--- a/ping_common.c
+++ b/ping_common.c
@@ -2,6 +2,9 @@
 #include <ctype.h>
 #include <sched.h>
 #include <math.h>
+#ifdef __linux__
+#include <sys/fsuid.h>
+#endif
 
 int options;
 
@@ -175,6 +178,12 @@ int modify_capability(int on)
 		perror("seteuid");
 		return -1;
 	}
+#ifdef __linux__
+	if (on) {
+		/* FIXME: error handling? setfsuid() have weird return code */
+		setfsuid(uid);
+	}
+#endif
 
 	return 0;
 }
diff --git a/traceroute6.c b/traceroute6.c
index 0538d4b..a14ddb6 100644
--- a/traceroute6.c
+++ b/traceroute6.c
@@ -266,6 +266,9 @@ char copyright[] =
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#ifdef __linux__
+#include <sys/fsuid.h>
+#endif
 
 #include "SNAPSHOT.h"
 
@@ -343,6 +346,11 @@ int main(int argc, char *argv[])
 	int ch, i, on, probe, seq, tos, ttl;
 	int socket_errno;
 
+#ifdef __linux__
+	// FIXME: error handling? setfsuid() have weird return code
+	setfsuid(getuid());
+#endif
+
 	icmp_sock = socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6);
 	socket_errno = errno;
 
-- 
1.7.6.3

>>From 550366b0d5ba7498c6f102a0f7da08f6119028c7 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 01:44:49 +0400
Subject: [PATCH 2/7] ping: permanently drop capabilities before entering main
 loop

---
 ping.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/ping.c b/ping.c
index c0366cd..7ac1217 100644
--- a/ping.c
+++ b/ping.c
@@ -587,6 +587,8 @@ main(int argc, char **argv)
 
 	setup(icmp_sock);
 
+	drop_capabilities();
+
 	main_loop(icmp_sock, packet, packlen);
 }
 
-- 
1.7.6.3

>>From 0f1a605b1b7838749f2dd315c588b939380ccbd0 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 01:46:44 +0400
Subject: [PATCH 3/7] arping: use seteuid for temporal uid changes

---
 arping.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arping.c b/arping.c
index bdf81e9..a35fafd 100644
--- a/arping.c
+++ b/arping.c
@@ -228,7 +228,7 @@ int modify_capability_raw(int on)
 
 	cap_free(cap_p);
 #else
-	if (setuid(on ? euid : getuid())) {
+	if (seteuid(on ? euid : getuid())) {
 		perror("arping: setuid");
 		return -1;
 	}
-- 
1.7.6.3

>>From b0025d82ce5592f8e8da51cf4b94ceea51073cbe Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 01:53:39 +0400
Subject: [PATCH 4/7] arping, ping_common: reset euid before permanent drop

setuid drop saved uid only if euid is 0
---
 arping.c      |    4 ++++
 ping_common.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arping.c b/arping.c
index a35fafd..0033f33 100644
--- a/arping.c
+++ b/arping.c
@@ -269,6 +269,10 @@ void drop_capabilities(void)
 
 	cap_free(cap_p);
 #else
+	if (seteuid(euid)) {
+		perror("arping: setuid");
+		return -1;
+	}
 	if (setuid(getuid()) < 0) {
 		perror("arping: setuid");
 		exit(-1);
diff --git a/ping_common.c b/ping_common.c
index 1d0012a..4a184ee 100644
--- a/ping_common.c
+++ b/ping_common.c
@@ -199,6 +199,10 @@ void drop_capabilities(void)
 	}
 	cap_free(cap);
 #else
+	if (seteuid(euid)) {
+		perror("seteuid");
+		exit(-1);
+	}
 	if (setuid(getuid())) {
 		perror("ping: setuid");
 		exit(-1);
-- 
1.7.6.3

>>From dcb8828342f2591ee29f1be2fda2f225aee5efa9 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 01:56:07 +0400
Subject: [PATCH 5/7] ping, ping6, arping, clockdiff: Fix CAP_SETUID <->
 setuid() interaction

setuid() only drops saved uid if process have CAP_SETUID.
Drop capabilities only after setuid().
---
 arping.c      |   30 +++++++++++++++---------------
 clockdiff.c   |    8 +++++---
 ping_common.c |   30 +++++++++++++++---------------
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/arping.c b/arping.c
index 0033f33..3c02abf 100644
--- a/arping.c
+++ b/arping.c
@@ -161,6 +161,21 @@ void limit_capabilities(void)
 #ifdef CAPABILITIES
 	cap_t cap_p;
 
+	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
+		perror("arping: prctl");
+		exit(-1);
+	}
+
+	if (setuid(getuid()) < 0) {
+		perror("arping: setuid");
+		exit(-1);
+	}
+
+	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
+		perror("arping: prctl");
+		exit(-1);
+	}
+
 	cap_p = cap_get_proc();
 	if (!cap_p) {
 		perror("arping: cap_get_proc");
@@ -184,21 +199,6 @@ void limit_capabilities(void)
 		}
 	}
 
-	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
-		perror("arping: prctl");
-		exit(-1);
-	}
-
-	if (setuid(getuid()) < 0) {
-		perror("arping: setuid");
-		exit(-1);
-	}
-
-	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
-		perror("arping: prctl");
-		exit(-1);
-	}
-
 	cap_free(cap_p);
 #else
 	euid = geteuid();
diff --git a/clockdiff.c b/clockdiff.c
index f12da2d..540366d 100644
--- a/clockdiff.c
+++ b/clockdiff.c
@@ -536,6 +536,11 @@ usage() {
 }
 
 void drop_rights(void) {
+	if (setuid(getuid())) {
+		perror("clockdiff: setuid");
+		exit(-1);
+	}
+	{
 #ifdef CAPABILITIES
 	cap_t caps = cap_init();
 	if (cap_set_proc(caps)) {
@@ -544,9 +549,6 @@ void drop_rights(void) {
 	}
 	cap_free(caps);
 #endif
-	if (setuid(getuid())) {
-		perror("clockdiff: setuid");
-		exit(-1);
 	}
 }
 
diff --git a/ping_common.c b/ping_common.c
index 4a184ee..b2344a7 100644
--- a/ping_common.c
+++ b/ping_common.c
@@ -80,6 +80,21 @@ void limit_capabilities(void)
 	cap_t cap_p;
 	cap_flag_value_t cap_ok;
 
+	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
+		perror("ping: prctl");
+		exit(-1);
+	}
+
+	if (setuid(getuid()) < 0) {
+		perror("setuid");
+		exit(-1);
+	}
+
+	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
+		perror("ping: prctl");
+		exit(-1);
+	}
+
 	cap_cur_p = cap_get_proc();
 	if (!cap_cur_p) {
 		perror("ping: cap_get_proc");
@@ -109,21 +124,6 @@ void limit_capabilities(void)
 		exit(-1);
 	}
 
-	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
-		perror("ping: prctl");
-		exit(-1);
-	}
-
-	if (setuid(getuid()) < 0) {
-		perror("setuid");
-		exit(-1);
-	}
-
-	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
-		perror("ping: prctl");
-		exit(-1);
-	}
-
 	cap_free(cap_p);
 	cap_free(cap_cur_p);
 #endif
-- 
1.7.6.3

>>From fdabf4869f868fe8ea0994910cc4944ebfd0b098 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 02:57:15 +0400
Subject: [PATCH 6/7] ninfod: use -u without capabilities too

---
 ninfod/ninfod.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ninfod/ninfod.c b/ninfod/ninfod.c
index 5ff6993..f1db977 100644
--- a/ninfod/ninfod.c
+++ b/ninfod/ninfod.c
@@ -589,7 +589,7 @@ static void drop_capabilities(void)
 
 	cap_free(cap_p);
 #else
-	if (setuid(getuid()) < 0) {
+	if (setuid(opt_u ? opt_u : getuid()) < 0) {
 		DEBUG(LOG_ERR, "setuid: %s\n", strerror(errno));
 		exit(-1);
 	}
-- 
1.7.6.3

>>From 4298d3af86b881ef7abb61a36f83a610fc8bb772 Mon Sep 17 00:00:00 2001
From: "Yuriy M. Kaminskiy" <[email-masked]>
Date: Wed, 2 Jan 2013 03:08:30 +0400
Subject: [PATCH 7/7] ninfod: fix capabilities setting

1) -u option failed to change real uid too (likely leaving it as root);
2) it failed to drop saved uid;
---
 ninfod/ninfod.c |   58 +++++++++++++++++-------------------------------------
 1 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/ninfod/ninfod.c b/ninfod/ninfod.c
index f1db977..7f6a2fa 100644
--- a/ninfod/ninfod.c
+++ b/ninfod/ninfod.c
@@ -497,16 +497,28 @@ static void do_daemonize(void)
 /* --------- */
 #ifdef HAVE_LIBCAP
 static const cap_value_t cap_net_raw = CAP_NET_RAW;
-static const cap_value_t cap_setuid =  CAP_SETUID; 
-static cap_flag_value_t cap_ok;
-#else
-static uid_t euid;
 #endif
 
 static void limit_capabilities(void)
 {
 #ifdef HAVE_LIBCAP
 	cap_t cap_p, cap_cur_p;
+	cap_flag_value_t cap_ok;
+
+	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
+		DEBUG(LOG_ERR, "prctl: %s\n", strerror(errno));
+		exit(-1);
+	}
+
+	if (setuid(opt_u ? opt_u : getuid()) < 0) {
+		DEBUG(LOG_ERR, "setuid: %s\n", strerror(errno));
+		exit(-1);
+	}
+
+	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
+		DEBUG(LOG_ERR, "prctl: %s\n", strerror(errno));
+		exit(-1);
+	}
 
 	cap_p = cap_init();
 	if (!cap_p) {
@@ -520,32 +532,20 @@ static void limit_capabilities(void)
 		exit(-1);
         }
 
-	/* net_raw + setuid / net_raw */
 	cap_get_flag(cap_cur_p, CAP_NET_RAW, CAP_PERMITTED, &cap_ok);
 	if (cap_ok != CAP_CLEAR) {
 		cap_set_flag(cap_p, CAP_PERMITTED, 1, &cap_net_raw, CAP_SET);
 		cap_set_flag(cap_p, CAP_EFFECTIVE, 1, &cap_net_raw, CAP_SET);
 	}
 
-	cap_get_flag(cap_cur_p, CAP_SETUID, CAP_PERMITTED, &cap_ok);
-	if (cap_ok != CAP_CLEAR)
-		cap_set_flag(cap_p, CAP_PERMITTED, 1, &cap_setuid, CAP_SET);
-
 	if (cap_set_proc(cap_p) < 0) {
 		DEBUG(LOG_ERR, "cap_set_proc: %s\n", strerror(errno));
 		if (errno != EPERM)
 			exit(-1);
 	}
 
-	if (prctl(PR_SET_KEEPCAPS, 1) < 0) {
-		DEBUG(LOG_ERR, "prctl: %s\n", strerror(errno));
-		exit(-1);
-	}
-
 	cap_free(cap_cur_p);
 	cap_free(cap_p);
-#else
-	euid = geteuid();
 #endif
 }
 
@@ -560,28 +560,6 @@ static void drop_capabilities(void)
 		exit(-1);
 	}
 
-	/* setuid / setuid */
-	if (cap_ok != CAP_CLEAR) {
-		cap_set_flag(cap_p, CAP_PERMITTED, 1, &cap_setuid, CAP_SET);
-		cap_set_flag(cap_p, CAP_EFFECTIVE, 1, &cap_setuid, CAP_SET);
-
-		if (cap_set_proc(cap_p) < 0) {
-			DEBUG(LOG_ERR, "cap_set_proc: %s\n", strerror(errno));
-			exit(-1);
-		}
-	}
-
-	if (seteuid(opt_u ? opt_u : getuid()) < 0) {
-		DEBUG(LOG_ERR, "setuid: %s\n", strerror(errno));
-		exit(-1);
-	}
-
-	if (prctl(PR_SET_KEEPCAPS, 0) < 0) {
-		DEBUG(LOG_ERR, "prctl: %s\n", strerror(errno));
-		exit(-1);
-	}
-
-	cap_clear(cap_p);
 	if (cap_set_proc(cap_p) < 0) {
 		DEBUG(LOG_ERR, "cap_set_proc: %s\n", strerror(errno));
 		exit(-1);
@@ -667,14 +645,14 @@ int main (int argc, char **argv)
 	appname = argv[0];
 	set_logfile();
 
+	parse_args(argc, argv);
+
 	limit_capabilities();
 
 	sock = open_sock();
 	if (sock < 0)
 		sock_errno = errno;
 
-	parse_args(argc, argv);
-
 	drop_capabilities();
 
 	if (opt_h || opt_v)
-- 
1.7.6.3