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

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

[corentin.labbe]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

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




[-- Attachment #2: auditd_drop_root_privilege.patch --]
[-- Type: text/x-patch, Size: 1349 bytes --]

--- src/auditd.c.orig	2009-10-05 14:18:52.000000000 +0200
+++ src/auditd.c	2009-10-05 14:55:36.000000000 +0200
@@ -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,

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Serge E. Hallyn]

Quoting corentin.labbe (corentin.labbe@geomatys.fr):
> 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
> 
> 
> 

> --- src/auditd.c.orig	2009-10-05 14:18:52.000000000 +0200
> +++ src/auditd.c	2009-10-05 14:55:36.000000000 +0200
> @@ -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) {

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();

>  		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;
> +		}

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 :)

> +	
>  	audit_msg(LOG_NOTICE,
>  	    "Init complete, auditd %s listening for events (startup state %s)",
>  		VERSION,

> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

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

[Steve Grubb]

On Tuesday 20 October 2009 10:07:30 am corentin.labbe wrote:
> This is a patch that add a -u parameter to auditd.

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

> 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