[apparmor] [PATCH] enhance aa-status to deal with lack of interface patch

[Kees Cook]

On Tue, Apr 24, 2012 at 11:33:09PM -0700, Steve Beattie wrote:
> On Tue, Apr 24, 2012 at 04:59:08PM -0700, Kees Cook wrote:
> > Handle lacking the interface patch, and examine the "enabled"
> > parameter for determining the state of AppArmor on the system.
> > 
> > Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=661153
> > 
> > Index: apparmor-debian/utils/aa-status
> > ===================================================================
> > --- apparmor-debian.orig/utils/aa-status	2011-05-27 12:08:50.000000000 -0700
> > +++ apparmor-debian/utils/aa-status	2012-04-24 12:42:39.540597212 -0700
> > @@ -14,8 +14,7 @@
> >  
> >  def cmd_enabled():
> >      '''Returns error code if AppArmor is not enabled'''
> > -    if get_profiles() == {}:
> > -        sys.exit(2)
> > +    find_apparmorfs()
> >  
> 
> I think this changes the behavior when apparmor is loaded and there are
> no profiles loaded:
> 
>   # without patch
>   $ sudo cat /sys/kernel/security/apparmor/profiles | wc -l
>   0
>   $ sudo aa-status --enabled
>   $ echo $?
>   2
> 
>   # with patch
>   $ sudo cat /sys/kernel/security/apparmor/profiles | wc -l
>   0
>   $ sudo ./aa-status --enabled
>   $ echo $?
>   0

Yeah. I've kind of always felt that this was a mis-feature. The things
the init system wants to know are:

- is AppArmor capable of functioning? (in kernel and enabled)
- is it time to start loading profiles? (... in Ubuntu we cheated by
    looking at if profiles are already loaded -- since all systems with
    AppArmor will have preloaded the network-early-start profiles. This
    is, however, a catch-22 and takes advantage of a specific side-effect
    of how Ubuntu loads profiles.)

So, if we can eliminate the need for the second thing above, then the
problem goes away, and --enabled can stop lying. :)

If I remember correctly, doing the profile count was to avoid loading
profiles in certain conditions. Unfortunately, I can't remember for sure
what those situations were. I think it was: LiveCD and release upgrades,
but I'm pretty sure both are covered now.

I'd like to have --enabled not lie, and work out the bugs this triggers
as separate issues. If this isn't cool for now, how about having it only
lie if the legacy "profiles" interface exists?

> >  def cmd_profiled():
> >      '''Prints the number of loaded profiles'''
> > @@ -72,19 +71,15 @@
> >      '''Fetch loaded profiles'''
> >  
> >      profiles = {}
> > -
> > -    if os.path.exists("/sys/module/apparmor"):
> > -        stdmsg("apparmor module is loaded.")
> > -    else:
> > -        errormsg("apparmor module is not loaded.")
> > -        sys.exit(1)
> > -
> >      apparmorfs = find_apparmorfs()
> > -    if not apparmorfs:
> > -        errormsg("apparmor filesystem is not mounted.")
> > -        sys.exit(3)
> >  
> > +    # Kernel with the stock kernel cannot read profiles, but shouldn't
> > +    # be considered a fatal failure mode.
> >      apparmor_profiles = os.path.join(apparmorfs, "profiles")
> > +    if not os.path.exists(apparmor_profiles):
> > +        errormsg("AppArmor running without interface patch -- cannot determine loaded profiles.")
> > +        return profiles
> > +
> 
> This should possibly be its own error code state, I think, to
> distinguish from "apparmor enabled, no policy loaded" to "apparmor
> loaded, but the version doesn't let me determine how many profiles
> are loaded". (It's up to the calling program to treat the return codes
> as fatal events -- there's no real other way for aa-status to return
> complex results.)

Well... as I mention in the comment, I don't think it's actually
an error condition. I think aa-status should attempt to handle it,
but emit a warning (which this does). It just means it can't tell the
user about processes that are running that are unconfined by a loaded
profile. It can still report all the other states sanely (since it reads
/proc/$pid/attr/current).

> >      if not os.access(apparmor_profiles, os.R_OK):
> >          errormsg("You do not have enough privilege to read the profile set.")
> >          sys.exit(4)
> > @@ -134,11 +129,29 @@
> >  
> >  def find_apparmorfs():
> >      '''Finds AppArmor mount point'''
> > +
> > +    apparmor_module = "/sys/module/apparmor"
> > +    if os.path.exists(apparmor_module):
> > +        stdmsg("AppArmor available in kernel.")
> > +    else:
> > +        errormsg("AppArmor not available in kernel.")
> > +        sys.exit(1)
> > +
> > +    apparmor_enabled = os.path.join(apparmor_module, "parameters", "enabled")
> > +    if not os.access(apparmor_enabled, os.R_OK):
> > +        errormsg("You do not have enough privilege to check AppArmor parameters.")
> > +        sys.exit(2)
> 
> This should be sys.exit(4), no? From aa-status(1):
> 
>   Upon exiting, aa-status will set its return value to the following values:
>   [SNIP]
>   4   if the user running the script doesn't have enough privileges to
>       read the apparmor control files.

Ah, yes. I will fix that.

> Also, it's not a huge thing, but I'd appreciate a blank line here, to make the
> sys.exit(2) more visible. I'm sure this violates some PEP somewhere.

Sure.

> > +    if open(apparmor_enabled, "r").readline().strip() != "Y":
> > +        errormsg("AppArmor not enabled. (Kernel not booted with \"security=apparmor\"?)")
> > +        sys.exit(3)
> 
> Similarly, this should probably sys.exit(1):
> 
>   1   if apparmor is not enabled/loaded.

Okay.

-Kees

-- 
Kees Cook