Skip to contentSkip to conversation

setuid/capabilities fixups

PATCH New Yuriy Kaminskiy 1 participant 1 message Jan 9, 2013

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Yuriy Kaminskiy

author
#0 lore raw

PATCH 1/1 setuid/capabilities fixups

Run ping, look at /proc/pidof ping/status -> oops (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.

Quoted 1 line

From bc01c19df465bec369ded83bf48b069c6ad462d2 Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
arping.c +9 -0
index 35408c1..bdf81e9 100644
@@ -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;
 }
clockdiff.c +8 -0
index 7c1ea1b..f12da2d 100644
@@ -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;
ping_common.c +9 -0
index 7f82851..12c87a4 100644
@@ -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;
 }
traceroute6.c +8 -0
index 0538d4b..a14ddb6 100644
@@ -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;
Signature · 1 line
1.7.6.3
Quoted 1 line

From da09261219322ec5116f016a858f11f83902deb9 Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
ping.c +2 -0
index c0366cd..7ac1217 100644
@@ -587,6 +587,8 @@ main(int argc, char **argv)
 
 	setup(icmp_sock);
 
+	drop_capabilities();
+
 	main_loop(icmp_sock, packet, packlen);
 }
Signature · 1 line
1.7.6.3
Quoted 1 line

From c4b6600fe72e169c64e47f85a973484053f23861 Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
arping.c +1 -1
index bdf81e9..a35fafd 100644
@@ -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;
 	}
Signature · 1 line
1.7.6.3
Quoted 1 line

From d8f54230f344c92f2120230dc24ac9c5d6672da9 Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
arping.c +4 -0
index a35fafd..0033f33 100644
@@ -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);
ping_common.c +4 -0
index 12c87a4..39b2c74 100644
@@ -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);
Signature · 1 line
1.7.6.3
Quoted 1 line

From 00f63e1afc9d07e9793574304ef34a5bb54629b7 Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
arping.c +15 -15
index 0033f33..3c02abf 100644
@@ -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();
clockdiff.c +5 -3
index f12da2d..540366d 100644
@@ -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);
 	}
 }
ping_common.c +15 -15
index 39b2c74..08c8582 100644
@@ -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
Signature · 1 line
1.7.6.3
Quoted 1 line

From 32ad925cba9aca3513561c7655c9f61642a341ca Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
ninfod/ninfod.c +1 -1
index d1b99d9..0f54da3 100644
@@ -561,7 +561,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);
 	}
Signature · 1 line
1.7.6.3
Quoted 1 line

From ab1b55fd572c1e28f762ef4feff911053c5eb1fd Mon Sep 17 00:00:00 2001

From: "Yuriy M. Kaminskiy" <[email protected]>
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(-)
ninfod/ninfod.c +18 -40

108 lines not on this page · raw message

Signature · 1 line
1.7.6.3