[apparmor] [PATCH] various aa-notify fixes

Tue Aug 16 23:29:58 UTC 2011

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/16/2011 03:13 PM, Jamie Strandboge wrote:
> Hi,
> 
> Christian Boltz reported several problems (via IRC) with aa-notify when
> used on OpenSUSE. Attaching all patches in this email as they are all
> quite straitforward.
> 
> 0001-drop-supplemental-groups.patch:
>   utils/aa-notify:
>   - drop supplemental group privileges too. While POSIX::setgid() works
> nice in
>     that it will set both the real uid and euid, it doesn't do anything
> with the
>     supplemental groups (sigh). Instead, assign to $( and $) in a manner
> that
>     clears the supplemental groups.
> 
> 
> 0002-update-aa-notify-manpage-for-user-and-p.patch:
>   utils/aa-notify.pod: update to clarify '-u' argument when using '-p'.
> 
> 
> 0003-check-dirname-with-auditd.patch:
>   utils/aa-notify:
>   
>   aa-notify would abort if it could not stat the logfile, as can happen
> when
>   using auditd and the directory perms for the logfile do not allow
> access (x).
>   Adjust get_logfile_size() and get_logfile_inode() to raise then drop
>   privileges if the logfile parent directory is not executable.
> 
>   Interestingly, this issue was masked on Ubuntu because of the
> improper 
>   dropping of supplemental groups fixed in 0001, above.
> 
> 
> -- Jamie Strandboge | http://www.canonical.com
> 
> 
> 0001-drop-supplemental-groups.patch
> 
> 
> ------------------------------------------------------------
> revno: 1783
> committer: Jamie Strandboge <jamie at canonical.com>
> branch nick: apparmor-trunk
> timestamp: Tue 2011-08-16 16:57:02 -0500
> message:
>   utils/aa-notify:
>   - drop supplemental group privileges too. While POSIX::setgid() works nice in
>     that it will set both the real uid and euid, it doesn't do anything with the
>     supplemental groups (sigh). Instead, assign to $( and $) in a manner that
>     clears the supplemental groups.
> diff:
> === modified file 'utils/aa-notify'
> --- utils/aa-notify	2010-11-04 00:03:52 +0000
> +++ utils/aa-notify	2011-08-16 21:57:02 +0000
> @@ -122,7 +122,8 @@
>  if ($< == 0) {
>      $login = "root";
>      if (defined($ENV{SUDO_UID}) and defined($ENV{SUDO_GID})) {
> -        POSIX::setgid($ENV{SUDO_GID}) or _error("Could not change gid");
> +        $) = "$ENV{SUDO_GID} $ENV{SUDO_GID}" or _error("Could not change egid");
> +        $( = $ENV{SUDO_GID} or _error("Could not change gid");
>          $> = $ENV{SUDO_UID} or _error("Could not change euid");
>          defined($ENV{SUDO_USER}) and $login = $ENV{SUDO_USER};
>      } else {
> @@ -131,7 +132,9 @@
>              $drop_to = $opt_u;
>          }
>          # nobody/nogroup
> -        POSIX::setgid(scalar(getgrnam($nobody_group))) or _error("Could not change gid to '$nobody_group'");
> +        my $nam = scalar(getgrnam($nobody_group));
> +        $) = "$nam $nam" or _error("Could not change egid");
> +        $( = $nam or _error("Could not change gid");
>          $> = scalar(getpwnam($drop_to)) or _error("Could not change euid to '$drop_to'");
>      }
>  } else {
> 
Thankyou for reminding me why I HATE perl

you need to check $! after the assignment of $( and $) for errors

> 
> 0002-update-aa-notify-manpage-for-user-and-p.patch
> 
> 
> ------------------------------------------------------------
> revno: 1784
> committer: Jamie Strandboge <jamie at canonical.com>
> branch nick: apparmor-trunk
> timestamp: Tue 2011-08-16 16:18:22 -0500
> message:
>   utils/aa-notify.pod: update to clarify '-u' argument when using '-p'.
> diff:
> === modified file 'utils/aa-notify.pod'
> --- utils/aa-notify.pod	2010-12-20 20:29:10 +0000
> +++ utils/aa-notify.pod	2011-08-16 21:18:22 +0000
> @@ -58,8 +58,9 @@
>  
>  =item -u USER, --user=USER
>  
> -user to drop privileges to when running privileged. This has no effect when
> -running under sudo.
> +user to drop privileges to when running privileged. When used with the -p
> +option, this should be set to the user that will receive desktop notifications.
> +This has no effect when running under sudo.
>  
>  =item -w NUM, --wait=NUM
> 
ACK

> 
> 0003-check-dirname-with-auditd.patch
> 
> 
> ------------------------------------------------------------
> revno: 1785
> committer: Jamie Strandboge <jamie at canonical.com>
> branch nick: apparmor-trunk
> timestamp: Tue 2011-08-16 16:46:41 -0500
> message:
>   utils/aa-notify:
>   
>   aa-notify would abort if it could not stat the logfile, as can happen when
>   using auditd and the directory perms for the logfile do not allow access (x).
>   Adjust get_logfile_size() and get_logfile_inode() to raise then drop
>   privileges if the logfile parent directory is not executable.
> diff:
> === modified file 'utils/aa-notify'
> --- utils/aa-notify	2011-08-16 21:10:27 +0000
> +++ utils/aa-notify	2011-08-16 21:46:41 +0000
> @@ -589,14 +589,54 @@
>  sub get_logfile_size {
>      my $fn = $_[0];
>      my $size;
> +    my $dir = File::Basename::dirname($fn);
> +
> +    # If we can't access the file, then raise privs. This can happen when
> +    # using auditd and /var/log/audit/ is 700.
> +    my $old_euid = $>;
> +    my $change_euid = 0;
> +    if (! -x $dir and $> != $<) {
> +        _debug("raising privileges to '$orig_euid' in get_logfile_size()");
> +        $change_euid = 1;
> +        $> = $orig_euid;
> +        $> == $orig_euid or die "Could not raise privileges\n";
> +    }
> +
>      defined(($size = (stat($fn))[7])) or (sleep(10) and defined(($size = (stat($fn))[7])) or die "'$fn' disappeared. Aborting\n");
> +
> +    if ($change_euid) {
> +        _debug("dropping privileges to '$old_euid' in get_logfile_size()");
> +        $> = $old_euid;
> +        $> == $old_euid or die "Could not drop privileges\n";
> +    }
> +
>      return $size;
>  }
>  
>  sub get_logfile_inode {
>      my $fn = $_[0];
>      my $inode;
> +    my $dir = File::Basename::dirname($fn);
> +
> +    # If we can't access the file, then raise privs. This can happen when
> +    # using auditd and /var/log/audit/ is 700.
> +    my $old_euid = $>;
> +    my $change_euid = 0;
> +    if (! -x $dir and $> != $<) {
> +        _debug("raising privileges to '$orig_euid' in get_logfile_inode()");
> +        $change_euid = 1;
> +        $> = $orig_euid;
> +        $> == $orig_euid or die "Could not raise privileges\n";
> +    }
> +
>      defined(($inode = (stat($fn))[1])) or (sleep(10) and defined(($inode = (stat($fn))[1])) or die "'$fn' disappeared. Aborting\n");
> +
> +    if ($change_euid) {
> +        _debug("dropping privileges to '$old_euid' in get_logfile_inode()");
> +        $> = $old_euid;
> +        $> == $old_euid or die "Could not drop privileges\n";
> +    }
> +
>      return $inode;
>  }

As with Christian I would like to see the duplicate code moved into fns.  I think testing $> == $orig_euid etc is probably good enough but the for some reason I would love to see a check against $! for errors as well
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5K/WMACgkQxAVxIsEKI+bsRgCfci4RwTltMdssNedpQkv514Ml
vXgAnjglkckAWoLxpmVTq1yTWxGIYRq5
=E3N9
-----END PGP SIGNATURE-----