[apparmor] [PATCH] various aa-notify fixes

[John Johansen]

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

On 08/17/2011 07:59 AM, Jamie Strandboge wrote:
> On Wed, 2011-08-17 at 01:21 +0200, Christian Boltz wrote:
>>> > > 0003-check-dirname-with-auditd.patch:
>> > I'd like to reject that patch.
>> > 
>> > This might surprise you because it fixes the bug I reported (tested 
>> > successfully). The reason for the recect is that it introduces lots of 
>> > duplicated code to raise and drop privileges. That's a guarantee for a 
>> > future maintenance hell [1].
>> > 
>> > Please move this code into a "sub raise_privileges" and another "sub 
>> > drop_privileges", and I'll happily say that the patch is OK ;-)
>> > (If you want to keep the name of the calling sub in the debug message, 
>> > add it as parameter when calling raise_privileges/drop_privileges.)
> Attached 0003-check-dirname-with-auditd_v2.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). Add raise_privileges() and drop_privileges() helper functions and adjust
> get_logfile_size() and get_logfile_inode() to raise then drop privileges if the
> logfile parent directory is not executable. Also adjust reopen_logfile() to use
> these helpers.
>   
> When error checking, use '$> == ...' instead of '$> = ... or die...' since perl
> always dies when raising privs in this manner even though the euid did change
> (and $!, $@, $^E, and $? are all the same). Not sure why this is happening but
> the '==' check should be sufficient.
> 
> 
>>> > >   Interestingly, this issue was masked on Ubuntu because of the
>>> > > improper
>>> > >   dropping of supplemental groups fixed in 0001, above.
>> > 
>> > Bad Ubuntu, they have set /var/log/audit too permissive ;-)
> Well, they aren't that bad at root:root 750...
> 
> -- Jamie Strandboge | http://www.canonical.com
> 
> 
> 0003-check-dirname-with-auditd_v2.patch
> 
> 
> ------------------------------------------------------------
> revno: 1788
> committer: Jamie Strandboge <jamie at canonical.com>
> branch nick: apparmor-trunk-aa-notify
> timestamp: Wed 2011-08-17 09:48:12 -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). Add raise_privileges() and drop_privileges() helper functions and adjust
>   get_logfile_size() and get_logfile_inode() to raise then drop privileges if the
>   logfile parent directory is not executable. Also adjust reopen_logfile() to use
>   these helpers.
>   
>   When error checking, use '$> == ...' instead of '$> = ... or die...' since perl
>   always dies when raising privs in this manner even though the euid did change
>   (and $!, $@, $^E, and $? are all the same). Not sure why this is happening but
>   the '==' check should be sufficient.
> diff:
> === modified file 'utils/aa-notify'
> --- utils/aa-notify	2011-08-17 13:35:52 +0000
> +++ utils/aa-notify	2011-08-17 14:48:12 +0000
> @@ -1,7 +1,7 @@
>  #!/usr/bin/perl
>  # ------------------------------------------------------------------
>  #
> -#    Copyright (C) 2009-2010 Canonical Ltd.
> +#    Copyright (C) 2009-2011 Canonical Ltd.
>  #
>  #    This program is free software; you can redistribute it and/or
>  #    modify it under the terms of version 2 of the GNU General Public
> @@ -561,42 +561,79 @@
>      print $s;
>  }
>  
> +sub raise_privileges {
> +    my $old_euid = -1;
> +
> +    if ($> != $<) {
> +        _debug("raising privileges to '$orig_euid'");
> +        $old_euid = $>;
> +        $> = $orig_euid;
> +        $> == $orig_euid or die "Could not raise privileges\n";
> +    }
> +
> +    return $old_euid;
> +}
> +
> +sub drop_privileges {
> +    my $old_euid = $_[0];
> +
> +    # Just exit if we didn't raise privileges
> +    $old_euid == -1 and return;
> +
> +    _debug("dropping privileges to '$old_euid'");
> +    $> = $old_euid;
> +    $> == $old_euid or die "Could not drop privileges\n";
> +}
> +
>  sub reopen_logfile {
>      # reopen the logfile, temporarily switching back to starting euid for
>      # file permissions.
>      close(LOGFILE);
>  
> -    my $old_euid = $>;
> -    my $change_euid = 0;
> -    if ($> != $<) {
> -        _debug("raising privileges to '$orig_euid' in reopen_logfile()");
> -        $change_euid = 1;
> -        $> = $orig_euid;
> -        $> == $orig_euid or die "Could not raise privileges\n";
> -    }
> +    my $old_euid = raise_privileges();
>  
>      $logfile_inode = get_logfile_inode($logfile);
>      $logfile_size = get_logfile_size($logfile);
>      open (LOGFILE, "<$logfile") or die "Could not open '$logfile'\n";
>  
> -    if ($change_euid) {
> -        _debug("dropping privileges to '$old_euid' in reopen_logfile()");
> -        $> = $old_euid;
> -        $> == $old_euid or die "Could not drop privileges\n";
> -    }
> +    drop_privileges($old_euid);
>  }
>  
>  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 = -1;
> +    if (! -x $dir) {
> +        $old_euid = raise_privileges();
> +    }
> +
>      defined(($size = (stat($fn))[7])) or (sleep(10) and defined(($size = (stat($fn))[7])) or die "'$fn' disappeared. Aborting\n");
> +
> +    drop_privileges($old_euid);
> +
>      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 = -1;
> +    if (! -x $dir) {
> +        $old_euid = raise_privileges();
> +    }
> +
>      defined(($inode = (stat($fn))[1])) or (sleep(10) and defined(($inode = (stat($fn))[1])) or die "'$fn' disappeared. Aborting\n");
> +
> +    drop_privileges($old_euid);
> +
>      return $inode;
>  }
> 
> 
It looks good to me (for what that is worth with my less than stellar perl skills)

Acked-by: John Johansen <john.johansen at canonical.com>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5MCaEACgkQxAVxIsEKI+b/JQCeM3WfVrwcPSJ86WKa6GOg5FCG
cZ4AnROrV5ruE/kqw7IGZL4eWkd3uVbZ
=R/o6
-----END PGP SIGNATURE-----