[apparmor] [patch] is_known_rule(): check includes recursively

[Christian Boltz]

Hello,

Am Montag, 6. Juli 2015 schrieb Steve Beattie:
> On Mon, Jun 22, 2015 at 10:14:01PM +0200, Christian Boltz wrote:
> > is_known_rule() in aa.py checked only direct includes, but not
> > includes in the included files. As a result, aa-logprof asked about
> > things that are already covered by an indirect include.
> > 
> > For example, the dovecot/auth profile includes
> > abstractions/nameservice, and abstractions/nameservice includes
> > abstractions/nis, which contains "capability net_bind_service,".
> > Nevertheless, aa-logprof asked to add capability net_bind_service.
> > 
> > Reproducer: (asks for net_bind_service without this patch, should
> > not
> > ask for anything after applying the patch):
> > python3 aa-logprof -d ../profiles/apparmor.d/ -f <(echo 'type=AVC
> > msg=audit(1415403814.628:662): apparmor="ALLOWED"
> > operation="capable" profile="/usr/lib/dovecot/auth" pid=15454
> > comm="auth" capability=13  capname="net_bind_service"')
> > 
> > The patch adds code to check include files included by other include
> > files. Note that python doesn't allow to change a list while looping
> > over it, therefore we have to use "while incname" as workaround.
> > 
> > This fixes a regression for network rules (this patch is based on
> > the
> > old match_net_include() code). Funnily it "only" fixes capability
> > rule handling (without the "regression" part) because the old
> > match_cap_include() didn't do the recursive include handling.
> > 
> > 
> > 
> > [ 55-is_known_rule-check-includes-recursively.diff ]
> > 
> > === modified file utils/apparmor/aa.py
> > --- utils/apparmor/aa.py        2015-06-21 19:02:13.797642711 +0200
> > +++ utils/apparmor/aa.py        2015-06-22 21:56:15.601218138 +0200
> > @@ -4087,11 +4087,28 @@
> > 
> >          if profile[rule_type].is_covered(rule_obj, False):
> >              return True
> > 
> > -    for incname in profile['include'].keys():
> > +    includelist = list(profile['include'].keys())
> > +    checked = []
> > +
> > +    if includelist:
> > +        incname = includelist.pop(0)
> > +
> 
> > +    while incname:
> Why not restructure this as:
> 
>     while len(includelist) > 0:
>         incname = includelist.pop(0)
> 
> It gets rid of the need for the pre-loop test, the awkward
> if-then construction at the end of the loop *and* the need for the
> 59-is_known_rule-init-incname.diff patch.

Good idea, but even this is still to complex ;-) - "while includelist:" 
does what we want.

Here's the updated patch:


[ 55-is_known_rule-check-includes-recursively.diff ]

=== modified file utils/apparmor/aa.py
--- utils/apparmor/aa.py        2015-07-07 20:53:58.092849277 +0200
+++ utils/apparmor/aa.py        2015-07-07 20:58:08.017610565 +0200
@@ -4071,11 +4072,21 @@
         if profile[rule_type].is_covered(rule_obj, False):
             return True
 
-    for incname in profile['include'].keys():
+    includelist = list(profile['include'].keys())
+    checked = []
+
+    while includelist:
+        incname = includelist.pop(0)
+        checked.append(incname)
+
         if include[incname][incname].get(rule_type, False):
             if include[incname][incname][rule_type].is_covered(rule_obj, False):
                 return True
 
+        for childinc in include[incname][incname]['include'].keys():
+            if childinc not in checked:
+                includelist += [childinc]
+
     return False
 
 def reload_base(bin_path):



Regards,

Christian Boltz
-- 
> ...womit die Geschichte wieder von vorn losgeht...
Jaha, aber nun ist das eine vollkommen andere Situation: Jetzt hast Du
nämlich eine von Suse generierte kdmrc für ein von Suse geliefertes KDE,
und KDE ist eine vom Installationssupport abgedeckte Komponente.
Also ist das ein meldefähiger und supportberechtigter Bug! :-)
[> Gerald Martin und Kristian Köhntopp in suse-linux]