[Apparmor-dev] [rfr] apparmor_status: report more detailed information

[Steve Beattie]

On Wed, May 30, 2007 at 03:01:55PM -0400, Mathias Gug wrote:
> On Tue, May 29, 2007 at 11:48:06AM -0700, jesse michael wrote:
> > > +	  $profiles{$1} = $2 if m/(\S+)\s+\((\w+)\)$/;
> > 
> > One thing that should be noted is that profile names can currently have
> > whitespace in them.  So if you profile "/usr/bin/my silly program", that
> > \S+ in the m// won't match things correctly.
> >
> I've updated the regex to m/^([^\(]+)\s+\((\w+)\)$/, which should take
> care of the white space problem.

For the purposes of apparmor_status, that regex is probably okay; it
won't correctly grab all of the whitespace if it's the tail of the name,
e.g. "/usr/bin/even sillier program    ".

> --
> Mathias
> 

> Index: utils/apparmor_status
> ===================================================================
> --- utils/apparmor_status	(révision 704)
> +++ utils/apparmor_status	(copie de travail)
> @@ -13,6 +13,7 @@
>  
>  use strict;
>  use Getopt::Long;
> +use Cwd 'abs_path';
>  
>  my $confdir = "/etc/apparmor";
>  my $sd_mountpoint;
> @@ -73,44 +74,43 @@
>    return $sd_mountpoint;
>  }
>  
> -sub count_profiles {
> +sub get_profiles {
>    my $mountpoint = shift;
> -  my $profiles = 0;
> -  my $enforced = 0;
> -  my $complain = 0;
> +  my %profiles = ();
>  
>    if (open(PROFILES, "$mountpoint/profiles")) {
>      while(<PROFILES>) {
> -      $profiles++;
> -      $enforced++ if m/\(enforce\)$/;
> -      $complain++ if m/\(complain\)$/;
> +	  $profiles{$1} = $2 if m/^([^\(]+)\s+\((\w+)\)$/;
>      }
>      close(PROFILES);
>    }
> -  return ($profiles, $enforced, $complain);
> +  return (%profiles);
>  }
>  
> -sub count_processes {
> -  my $processes = 0;
> -  my $confined = 0;
> -  my $enforced = 0;
> -  my $complain = 0;
> +sub get_processes {
> +  my %profiles = @_;
> +  my %processes = ();
>    if (opendir(PROC, "/proc")) {
>      my $file;
>      while (defined($file = readdir(PROC))) {
> -      if ($file =~ m/^\d+/ && open(CURRENT, "/proc/$file/attr/current")) {
> -	while (<CURRENT>) {
> -	  $processes++;
> -	  $confined++ if not m/^uncon(stra|f)ined$/;
> -	  $enforced++ if m/\(enforce\)$/;
> -	  $complain++ if m/\(complain\)$/;
> -	}
> -	close(CURRENT);
> +      if ($file =~ m/^\d+/ && grep(abs_path("/proc/$file/exe") eq $_ , keys(%profiles))) {

Unfortunately, this skips processes that have inherited a policy from
another process; this is especially common in shell scripts.

> +        if (open(CURRENT, "/proc/$file/attr/current")) {
> +	      while (<CURRENT>) {
> +	        if (m/(\S+)\s+\((\w+)\)$/) {
> +              $processes{$file}{'profile'} = $1;
> +              $processes{$file}{'mode'} = $2;
> +            } else {
> +              $processes{$file}{'profile'} = abs_path("/proc/$file/exe");
> +              $processes{$file}{'mode'} = 'unconstrained';
> +            }
> +	      }
> +	      close(CURRENT);
> +        }
>        }
>      }
>      closedir(PROC);
>    }
> -  return ($processes, $confined, $enforced, $complain);
> +  return (%processes);
>  }
>  
>  my $is_loaded = is_subdomain_loaded();
> @@ -135,49 +135,82 @@
>  
>  #print "subdomainfs is at $sd_mountpoint.\n" if $verbose;
>  
> -my $processes;
> -my $profiles;
> -my $enforced;
> -my $complain;
> +# processes is a hash table :
> +#   * keys : processes pid
> +#   * values : hash containing information about the running process:
> +#       * 'profile' : name of the profile applied to the running process
> +#       * 'mode' : mode of the profile applied to the running process
> +my %processes = ();
> +my %enforced_processes = (); 
> +my %complain_processes = ();
> +my %unconstrained_processes = ();
>  
> -($profiles, $enforced, $complain) = count_profiles($sd_mountpoint);
> +# profiles is a hash table :
> +#  * keys : profile name
> +#  * value : profile mode
> +my %profiles;
> +my @enforced_profiles = ();
> +my @complain_profiles = ();
>  
> +%profiles = get_profiles($sd_mountpoint);
> + at enforced_profiles = grep { $profiles{$_} eq 'enforce' } keys %profiles;
> + at complain_profiles = grep { $profiles{$_} eq 'complain' } keys %profiles;
> +
>  # we consider the case where no profiles are loaded to be "disabled" as well
> -my $rc = ($profiles == 0) ? 2 : 0;
> +my $rc = (keys(%profiles) == 0) ? 2 : 0;
>  
>  if ($check_enabled) {
>    exit $rc;
>  }
>  
>  if ($count_profiled) {
> -  print "$profiles\n";
> +  print scalar(keys(%profiles));
>    exit $rc;
>  }
>  
>  if ($count_enforced) {
> -  print "$enforced\n";
> +  print $#enforced_profiles+1;
>    exit $rc;
>  }
>  
>  if ($count_complain) {
> -  print "$complain\n";
> +  print $#complain_profiles+1;
>    exit $rc;
>  }

I think it's worthwhile to keep the newlines in the above, for human
readers.

>  
>  
>  if ($verbose) {
> -  print "$profiles profiles are loaded.\n";
> -  print "$enforced profiles are in enforce mode.\n";
> -  print "$complain profiles are in complain mode.\n";
> +  print keys(%profiles) . " profiles are loaded.\n";
> +  print $#enforced_profiles + 1 . " profiles are in enforce mode.\n";
> +  for (@enforced_profiles) {
> +    print " " . $_ . "\n";
> +  }
> +  print $#complain_profiles + 1 . " profiles are in complain mode.\n";
> +  for (@complain_profiles) {
> +    print " " . $_ . "\n";
> +  }

Would you mind upping the indentation to 2 spaces or more just to make visually
identifying the section headers a little more distinct?

>  }
>  
> -($processes, $profiles, $enforced, $complain) = count_processes();
> -
> +%processes = get_processes(%profiles);
>  if ($verbose) {
> -  print "Out of $processes processes running:\n";
> -  print "$profiles processes have profiles defined.\n";
> -  print "$enforced processes have profiles in enforce mode.\n";
> -  print "$complain processes have profiles in complain mode.\n";
> +  for (keys(%processes)) {
> +    $enforced_processes{$_} = $processes{$_} if $processes{$_}{'mode'} eq 'enforce';
> +    $complain_processes{$_} = $processes{$_} if $processes{$_}{'mode'} eq 'complain';
> +    $unconstrained_processes{$_} = $processes{$_} if $processes{$_}{'mode'} eq 'unconstrained';

In our LKML submission we've switched to the keyword 'unconfined',
as it is the term we use when discussing processes that don't have an
apparmor policy defined. It'd be ideal if apparmor_status could work on
both old code and new.

> +  }
> +  print keys(%processes) . " processes have profiles defined.\n";
> +  print keys(%enforced_processes) . " processes are in enforce mode :\n";
> +  for (keys(%enforced_processes)) {
> +    print " " . $enforced_processes{$_}{'profile'} . " ($_) \n";
> +  }
> +  print keys(%complain_processes) . " processes are in complain mode.\n";
> +  for (keys(%complain_processes)) {
> +    print " " . $complain_processes{$_}{'profile'} . " ($_) \n";
> +  }
> +  print keys(%unconstrained_processes) . " processes are unconstrained but have a profile defined.\n";
> +  for (keys(%unconstrained_processes)) {
> +    print " " . $unconstrained_processes{$_}{'profile'} . " ($_) \n";
> +  }
>  }
>  
>  exit $rc;

Thanks Matthias, looks very nice.

-- 
Steve Beattie
SUSE Labs, Novell Inc. 
<sbeattie at suse.de>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://forge.novell.com/pipermail/apparmor-dev/attachments/20070530/eec9d0a9/attachment.pgp