[apparmor] [patch 3/3] use capability rule class in aa.py and cleanprof.py

[Christian Boltz]

Hello,

Am Freitag, 21. November 2014 schrieb Steve Beattie:
> On Sat, Nov 15, 2014 at 11:46:41PM +0100, Christian Boltz wrote:
> > this patch changes aa.py and cleanprof.py to use the new capabiliy
> > rule class.
> > 
> > The most important details in the change are:
> > - the capability rules are stored in
> > 
> >   aa[profile][hat]['capability'] instead of
> >   aa[profile][hat]['allow']['capability'] and
> >   aa[profile][hat]['deny']['capability']
> >   (allow/deny is handled inside the capability_rules class)
> 
> Good.

:-)

> > - profile_known_capability() now just returns True or False. Before,
> > it> 
> >   returned different values for deny and allow, but the calling code
> >   doesn't care about this detail anyway.
> 
> For something that returns a boolean value, it feels like an awkwardly
> named function. is_known_capability?

Well, yes.

The final function will most probably be

    def is_known_rule(profile, rule_type, rule_obj):
        if profile[rule_type].is_covered(obj):
            [...]

and work for all rule types :-)

> > Some things are still a bit ugly (and commented as such). On the
> > long
> > term, I plan to change logparser so that it returns a set of *_rule
> > classes instead of the current array with parsed events. This will
> > remove some of the ugly tricks I had to add.
> > 
> > We should also come up with a function that initializes the
> > structure
> > for each profile in aa[profile][hat] - with that, we could drop
> > several safety checks I had to add to avoid problems with profiles
> > that don't contain a capability rule.
> 
> Ideally, we move to profiles/hats being encapsulated in a class of
> their own, for which the __init__() method can properly initialize
> the fields of the instance. 

Right, but that will be another patch (probably not this month, unless 
someone volunteers ;-)

> In the short term, can't we add a
> CapabilityRuleset when we start parsing a new profile/hat?

In theory, yes. We'll need to find all places where we initialize a new 
profile/hat - and finding places where it explodes was easier for 
obvious reasons ;-)

I searched for places that might need to be changed, see the attached 
init-profile-locations.diff. This patch comes without warranty ;-)  Also 
also note that I only checked aa.py. I wouldn't be surprised if
aa-cleanprof or aa-mergeprof also need to be changed.
(Any volunteer to test and implement that?)

> > I also had to add several
> > +                    if write_prof_data[name].get(segs, False):
> > +                       
> > write_prof_data[name][segs].delete_all_rules() in
> > serialize_profile_from_old_profile(). That's needed to avoid
> > writing rules twice.
> 
> Is this rule writing duplication because we no longer have an 'allow'
> and 'deny' layer in the hasher hierarchy for capabilities?

Exactly. See also this:

> > On the positive side, when we have converted everything to
> > rule classes, we can delete the four lines above each of these added
> > lines :-)

which deletes the rules from the "old" allow and deny branches.

> > === modified file 'utils/apparmor/aa.py'
> > --- utils/apparmor/aa.py	2014-11-15 11:51:24 +0000
> > +++ utils/apparmor/aa.py	2014-11-15 21:14:56 +0000

> > @@ -1628,16 +1630,16 @@
> > 
> >                                  if deleted:
> >                                      aaui.UI_Info(_('Deleted %s
> >                                      previous matching profile
> >                                      entries.') % deleted)> 
> > -                           
> > aa[profile][hat]['allow']['capability'][capability]['set'] = True -
> >                           
> > aa[profile][hat]['allow']['capability'][capability]['audit'] =
> > audit_toggle +                            else:
> > +                               
> > aa[profile][hat]['capability'].add_raw('%s capability %s,'% (audit,
> > capability))  # XXX yes, ugly ;-)
> I agree it's ugly, it suggests our interfaces into the classes are
> suboptimal. Why can't we have something like:
> 
> 		cap_rule = CapabilityRule(capability, audit=audit)
> 		aa[profile][hat]['capability'].add(cap_rule)
> 
> rather than playing games consing up strings. (Yes, you could write
> the two lines as a one-liner if you really wanted and skip the
> temporary cap_rule variable).

Using strings is just a temporary solution - on the long term, I want to 
use rule objects everywhere. 

Even if I initially didn't want to do it now, I thought about it again 
and changed the code to use rule objects.

This means two lines to initialize the rule object - one to create the 
object and a call to the newly added set_param() function (no, I don't 
want to do that in __init__)

> > @@ -4408,19 +4373,20 @@
> > 
> >      return 0
> >  
> >  def profile_known_capability(profile, capname):
> > -    if profile['deny']['capability'][capname].get('set', False):
> > -        return -1
> > +    rule = capability_rule()
> > +    rule.set_raw('capability %s,' % capname)  # having a
> > capability_rule object as parameter would be even better ;-)
> Or, you know, have the initializer for the class take an optional
> (or maybe not even optional) capname argument.

See above - it's now using CapabilityRule set_param().

Besides that, I implemented the is_known_rule() function that will work 
for all rule types, and replaces profile_known_capability()

I also added a match_includes() function so that we can search for 
possible include files with a rule object. This function replaces 
match_cap_includes() (for aa.py usage), however aa-mergeprof still needs 
match_cap_includes() so I rewrote it as a wrapper for match_includes().

I also simplified the type check at various places - checking with
    aa[profile][hat].get(rule_type, False)
also works - and has the advantage that it doesn't restrict the 
functions to capability rule objects.

> > === modified file 'utils/apparmor/cleanprofile.py'
> > --- utils/apparmor/cleanprofile.py	2014-09-05 21:21:00 +0000
> > +++ utils/apparmor/cleanprofile.py	2014-11-15 18:48:23 +0000
> > @@ -65,8 +65,8 @@
> > 
> >                  deleted +=
> >                  apparmor.aa.delete_duplicates(self.other.aa[progra
> >                  m][hat], inc)>              
> >              #Clean the duplicates of caps in other profile
> > 
> > -            deleted +=
> > delete_cap_duplicates(self.profile.aa[program][hat]['allow']['capab
> > ility'], self.other.aa[program][hat]['allow']['capability'],
> > self.same_file) -            deleted +=
> > delete_cap_duplicates(self.profile.aa[program][hat]['deny']['capabi
> > lity'], self.other.aa[program][hat]['deny']['capability'],
> > self.same_file) +            if self.same_file:
> > +                deleted +=
> > self.other.aa[program][hat]['capability'].delete_duplicates(self.pr
> > ofile.aa[program][hat]['capability'])
> Doesn't this need the same type check, to make sure that
> self.other.aa[program][hat]['capability'] is not None?

It didn't explode while I tested it, so I'd guess "no" ;-)


That all said - patch v3 is attached.

Line counts:
v3-1-add-base-and-capability-rule-class.diff - 374 lines added, 0 
removed
v3-2-add-capability-rule-test.diff - 791 lines added, 0 removed
v3-3-use-capability-rule-class.diff - 77 lines added, 111 removed


Regards,

Christian Boltz
-- 
Aus der Beschreibung entnehme ich, daß deine Fonts nach Typ 3
konvertiert werden (Finger im Hals) und deine Bilder auf Screen-
Qualität (Fuß zum Finger dazusteck...)     [Ratti in suse-linux]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: init-profile-locations.diff
Type: text/x-patch
Size: 2103 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141123/bc96d179/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-3-use-capability-rule-class.diff
Type: text/x-patch
Size: 20815 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141123/bc96d179/attachment-0003.bin>