Proof of concept patch, add dropping privileges to a non root user

PATCH corentin.labbe 3 participants 3 messages Oct 20, 2009 – Oct 21, 2009

Share & tools

corentin.labbe

author

2009-10-20 14:03 UTC #0 lore raw

Hello

This is a patch that add a -u parameter to auditd.
This parameter permit to auditd to drop to an unprivilegied UID after initialization.

Any comment will be appreciated.

Cordially

auditd.c renamed from auditd.c.orig +22 -2

@@ -471,9 +471,10 @@
 	struct ev_signal sigusr2_watcher;
 	struct ev_signal sigchld_watcher;
 	int rc;
+	int auditd_uid=0;
 
 	/* Get params && set mode */
-	while ((c = getopt(argc, argv, "flns:")) != -1) {
+	while ((c = getopt(argc, argv, "flns:u:")) != -1) {
 		switch (c) {
 		case 'f':
 			opt_foreground = 1;

@@ -481,6 +482,17 @@
 		case 'l':
 			opt_allow_links=1;
 			break;
+		case 'u':
+			auditd_uid = atoi(optarg);
+			if (auditd_uid > 65535) {
+				fprintf(stderr, "Invalid UID '%s' > 65535\n", optarg);
+				usage();
+			}
+			if (auditd_uid < 0) {
+				fprintf(stderr, "Invalid UID '%s' < 0\n", optarg);
+				usage();
+			}
+			break;
 		case 'n':
 			do_fork = 0;
 			break;

@@ -522,7 +534,7 @@
 
 #ifndef DEBUG
 	/* Make sure we are root */
-	if (getuid() != 0) {
+	if (getuid() != 0 && auditd_uid == 0) {
 		fprintf(stderr, "You must be root to run this program.\n");
 		return 4;
 	}

@@ -690,6 +702,14 @@
 		shutdown_dispatcher();
 		return 1;
 	}
+	
+	if (auditd_uid > 0)
+		if (setuid(auditd_uid) == -1) {
+			fprintf(stderr, "setuid error() %d.\n", errno);
+			shutdown_dispatcher();
+			return 1;
+		}
+	
 	audit_msg(LOG_NOTICE,
 	    "Init complete, auditd %s listening for events (startup state %s)",
 		VERSION,

Steve Grubb

2009-10-20 16:34 UTC #1 lore raw

Quoted 1 line · corentin.labbe

This is a patch that add a -u parameter to auditd.

That would perhaps change a bunch of things in auditd file permissions.

Quoted 2 lines

This parameter permit to auditd to drop to an unprivilegied UID after
initialization.

Have you checked to see if these things still work:

* service auditd rotate, and do you get a DAEMON_ROTATE record filled in?

* service auditd reload, and do you get a DAEMON_RECONFIG record filled in?

* service auditd stop, and do you get a DAEMON_END record filled in?

* If you increase the priority in auditd.conf and run service auditd reload, does it work?

*Does space_left_action still work for email, single, and halt options?

* Can you still change tcp_listen_port to another privileged port and service auditd reload?

* What about the kerberos options?

Just curious if these scenarios were checked. :)

-Steve

Serge E. Hallyn

2009-10-21 00:39 UTC #2 lore raw

In reply to corentin.labbe

Quoted 45 lines · corentin.labbe

Hello

This is a patch that add a -u parameter to auditd.
This parameter permit to auditd to drop to an unprivilegied UID after initialization.

Any comment will be appreciated.

Cordially

auditd.c renamed from auditd.c.orig +13 -1

@@ -471,9 +471,10 @@
 	struct ev_signal sigusr2_watcher;
 	struct ev_signal sigchld_watcher;
 	int rc;
+	int auditd_uid=0;
 
 	/* Get params && set mode */
-	while ((c = getopt(argc, argv, "flns:")) != -1) {
+	while ((c = getopt(argc, argv, "flns:u:")) != -1) {
 		switch (c) {
 		case 'f':
 			opt_foreground = 1;

@@ -481,6 +482,17 @@
 		case 'l':
 			opt_allow_links=1;
 			break;
+		case 'u':
+			auditd_uid = atoi(optarg);
+			if (auditd_uid > 65535) {
+				fprintf(stderr, "Invalid UID '%s' > 65535\n", optarg);
+				usage();
+			}
+			if (auditd_uid < 0) {
+				fprintf(stderr, "Invalid UID '%s' < 0\n", optarg);
+				usage();
+			}
+			break;
 		case 'n':
 			do_fork = 0;
 			break;

auditd.c line 537

Quoted 5 lines

auditd.c renamed from auditd.c.orig +1 -1

@@ -522,7 +534,7 @@
 
 #ifndef DEBUG
 	/* Make sure we are root */
-	if (getuid() != 0) {
+	if (getuid() != 0 && auditd_uid == 0) {

I don't have the original source in front of me, but I think what you'd really want to do here is check that

	if (geteuid() != 0) {
		...
	}

or better yet do a detailed check for the capabilities you need, which I suppose are something like

	if (!got_caps(CAP_AUDIT_CONTROL | CAP_AUDIT_WRITE))
		complain();

	if (getuid() != auditd_uid && !got_caps(CAP_SETUID))
		complain();

Quoted 4 lines

auditd.c renamed from auditd.c.orig +0 -0

 		fprintf(stderr, "You must be root to run this program.\n");
 		return 4;
 	}

auditd.c line 711

Quoted 10 lines

auditd.c renamed from auditd.c.orig +7 -0

@@ -690,6 +702,14 @@
 		shutdown_dispatcher();
 		return 1;
 	}
+	
+	if (auditd_uid > 0)
+		if (setuid(auditd_uid) == -1) {
+			fprintf(stderr, "setuid error() %d.\n", errno);
+			shutdown_dispatcher();
+			return 1;
+		}

I think it's always worthwhile to follow this by a

	getresuid(&r, &e, &s);
	if (r != auditd_uid || e != auditd_uid || s != auditd_uid)
		bail();

I don't really know that an attacker could set things up so that uid and suid wouldn't get set (i.e. !CAP_SETUID, and uid==auditd_uid, but it's conceivable - i.e. finds a way to drop CAP_SETUID from the bounding set through another vulnerability, then runs a setuid root auditd using 'auditd -u id -u'. That's not quite it, as saveduid would have to be 0, and i can't recall offhand whether execve() of a setuid-root binary sets saved_uid to 0 or not. But hopefully this rant is scary enough to convince you that it's worth just making sure :)

Quoted 9 lines

auditd.c renamed from auditd.c.orig +1 -1
+	
 	audit_msg(LOG_NOTICE,
 	    "Init complete, auditd %s listening for events (startup state %s)",
 		VERSION,
 
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit