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

["Serge E. Hallyn" <serue@us.ibm.com>]

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