Quoting corentin.labbe ([email protected]):
> 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
> [email protected]
> https://www.redhat.com/mailman/listinfo/linux-audit

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Reply via email to