[apparmor] [patch] [13/38] Add ANY_EXEC to FileRule

[Christian Boltz]

Hello,

Am Donnerstag, 22. September 2016, 10:38:36 CEST schrieb Steve Beattie:
> On Fri, Aug 12, 2016 at 10:54:09PM +0200, Christian Boltz wrote:
> > aa-logprof needs to check if an exec rule for a given path exists.
> > 
> > This patch adds a __FileAnyExec class to FileRule, as well as
> > ANY_EXEC (which should be used externally, similar to ALL), and
> > adjusts several checks to allow it as a special execute mode.
> > 
> > This will allow to use is_covered() (or aa.py is_known_rule()) to
> > find out if execute is permitted, which replaces aa.py
> > profile_known_exec() in one of the following patches.
> > 
> > As usual, also add some tests.
> > 
> > [ 13-FileRule-add-ANY_EXEC.diff ]
> > 
> > === modified file ./utils/apparmor/rule/file.py
> > --- utils/apparmor/rule/file.py	2016-02-21 15:43:58.009985520 +0100
> > +++ utils/apparmor/rule/file.py	2016-02-21 16:05:39.673508607 +0100
> > @@ -235,12 +242,14 @@
> > 
> >              return False
> >          
> >          # TODO: handle fallback modes?
> > 
> > -        if other_rule.exec_perms and self.exec_perms !=
> > other_rule.exec_perms: 
> > +        if other_rule.exec_perms == self.ANY_EXEC and 
> > self.exec_perms: 
> > +            pass  # avoid hitting the 'elif' branch
> > 
> > +        elif other_rule.exec_perms and self.exec_perms != 
> > other_rule.exec_perms:
> >              return False
> 
> Could you give a more explanatory comment than merely wanting to skip
> the elif: test? Or restructure the conditionals that make it clear in
> what situations we're returning False here for?

Basically the if condition is better than any comment. The "pass" gets 
hit if other_rule.exec_perms is ANY_EXEC and self.exec_perms is set 
(which means != None, so Px, Cx, ix, Ux etc. are valid).

Restructuring the code (doing the check from "elif" first doesn't look 
like a good idea because it would result in a more complicated condition 
(self.exec_perms != None + a check for ANY_EXEC). (Yes, I thought about 
that because I'm not the biggest fan of "pass" - but in this case, it 
really gives us easier to understand code.)


I agree that the comment could be better.

What about this:

    pass # other_rule has ANY_EXEC and self has an exec rule set -> covered, so avoid hitting the 'elif' branch

If nobody objects, I'll adjust the patch.

> That said, that's not enough of a criticism to block my
> Acked-by: Steve Beattie <steve at nxnw.org>. Thanks.

Thanks for reviewing the patches!


Regards,

Christian Boltz
-- 
> what do I need to avoid?
* Belgian "Beer". At any cost.
[> Richard Brown and Henne Vogelsang in opensuse-project]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160922/90e6fa2f/attachment-0001.pgp>