[apparmor] [patch] [06/38] Add FileRule and FileRuleset

Wed Sep 14 17:56:02 UTC 2016

On Wed, Sep 14, 2016 at 8:55 PM, Christian Boltz <apparmor at cboltz.de> wrote:
> Hello,
>
> Am Montag, 12. September 2016, 01:58:44 CEST schrieb Kshitij Gupta:
>> On Sat, Aug 13, 2016 at 2:17 AM, Christian Boltz wrote:
>> > Hello,
>> >
>> > $subject.
>> >
>> > These classes handle file rules, including file rules with leading
>> > perms, and are meant to replace lots of file rule code in aa.py and
>> > aa-mergeprof.
>> >
>> > Note: get_glob() and logprof_header_localvars() don't even look
>> > finalized and will be changed in a later patch. (Some other things
>> > will also be changed or added with later patches - but you probably
>> > won't notice them while reviewing this patch.)
>> >
>> >
>> > [ 06-add-FileRule.diff ]
>> >
>> > --- utils/apparmor/rule/file.py 2016-01-20 21:59:12.806060698 +0100
>> > +++ utils/apparmor/rule/file.py 2016-01-20 20:44:55.781788850 +0100
>
>> > +allow_exec_transitions          = ('ix', 'ux', 'Ux', 'px', 'Px',
>> > 'cx', 'Cx')  # 2 chars - len relevant for split_perms()
>> > +allow_exec_fallback_transitions = ('pix', 'Pix', 'cix', 'Cix',
>> > 'pux', 'PUx', 'cux', 'CUx')  # 3 chars - len relevant for
>> > split_perms()
>> you mean allowed?
>> if yes, then that reads better
>
> I get your point ;-)
>
> My reason to choose "allow" instead of "allowed" was to match the
> "allow" keyword.
>
>> > +deny_exec_transitions           = ('x')
>>
>> guessing this is used later?
>
> That was the plan, but grep says I didn't follow it ;-)
>
> So we can either drop it, or keep it for documentation purposes -
> opinions?

probably make it a comment then?
Ofcourse can take that up separately after the patch series (if you
remember it).
>
>> > +    def __init__(self, path, perms, exec_perms, target, owner,
>> > file_keyword=False, leading_perms=False, +
>> > audit=False, deny=False, allow_keyword=False, comment='',
>> > log_event=None): +        '''Initialize FileRule
> ...
>> > +        self.perms, self.all_perms, unknown_items =
>> > check_and_split_list(perms, file_permissions, FileRule.ALL,
>> > 'FileRule', 'permissions', allow_empty_list=True) +
>> > if unknown_items:
>> > +            raise AppArmorBug('Passed unknown perms to FileRule:
>> > %s' % str(unknown_items))
>> str(perms) preferable?
>
> Mentioning the problematic (unknown) items makes more sense IMHO - that
> avoids the need for searching the needle in the haystack
>
> Besides that, AppArmorBug exceptions should never happen to normal
> users ;-)
>
>> > +        if self.perms and 'a' in self.perms and 'w' in self.perms:
>> > +            raise AppArmorException("Conflicting permissions found:
>> > 'a' and 'w'") +
>> > +        if exec_perms is None:
>> > +            self.exec_perms = None
>> > +        elif type_is_str(exec_perms):
>> > +            if deny:
>> > +                if exec_perms != 'x':
>> > +                    raise AppArmorException(_("file deny rules only
>> > allow to use 'x' as execute mode, but not %s" % exec_perms))
>> Nitpick: s/^f/F/
>> similarly at other places also please.
>
> The "file" keyword (which is an optional prefix for file rules) is
> lowercase, and that's the reason why I kept it lowercase in the error
> message.
>
> If you think this reason isn't good enough, I can change it - but I'll
> do it as follow-up patch to avoid breaking some of the following 30
> patches ;-)
Add The ;-)

Feel free to not take this up.
>
>> > +            else:
>> > +                if exec_perms == 'x':
>> > +                    raise AppArmorException(_("Execute flag ('x')
>> > in file rule must specify the exec mode (ix, Px, Cx etc.)")) +
>> >           elif exec_perms not in allow_exec_transitions and
>> > exec_perms not in allow_exec_fallback_transitions: +
>> >     raise AppArmorBug('Unknown execute mode specified in file rule:
>> > %s' % exec_perms) +            self.exec_perms = exec_perms
>> > +        else:
>> > +            raise AppArmorBug('Passed unknown perms object to
>> > FileRule: %s' % str(perms)) +
>> > +        if type(owner) is not bool:
>> > +            raise AppArmorBug('non-boolean value passed to owner
>> > flag') +        self.owner = owner
>> > +
>> > +        if type(file_keyword) is not bool:
>> > +            raise AppArmorBug('non-boolean value passed to file
>> > keyword flag') +        self.file_keyword = file_keyword
>> > +
>> > +        if type(leading_perms) is not bool:
>> > +            raise AppArmorBug('non-boolean value passed to leading
>> > permissions flag') +        self.leading_perms = leading_perms
>>
>> Should we have all these param type checks/validations (as required by
>> constructor documentation) in just one place at the beginning? That
>> way we have "fail-fast" for bad param types, this also breaks flow
>> when reading.
>> Couldn't recall/find a similar usage in other rules so not sure what
>> we've been following.
>
> Other rules luckily have less parameters ;-)
>
> In general, I'm handling one parameter after the other in all *Rule
> __init__() functions.
>
> I can understand why you like "fail-fast" - it makes sense in most
> cases. However, IMHO it doesn't make too much sense in a function where
> 80% of the code is validation / erroring out and 20% is storing the
> values in a variable.
>
fair point.

>> > +        # XXX subset
>> > +
>> > +        # check for invalid combinations (bare 'file,' vs. path
>> > rule) +#       if (self.all_paths and not self.all_perms) or (not
>> > self.all_paths and self.all_perms): +#           raise
>> > AppArmorBug('all_paths and all_perms must be equal') +# elif
>>
>> indentation above looks off
>
> It's only a comment ;-)
>
> IIRC, this check caused false alerts, so I commented it out.
> Unfortunately, I don't remember what broke exactly.
should have added that comment *g*

>
>> > +        if self.all_paths and self.all_perms and not path and not
>> > perms and not target:
>> > +            return('%s%s%sfile,%s' % (space,
>> > self.modifiers_str(), owner, self.comment))  # plain 'file,' rule
>> these can definitely used named parameters
>
> That would make the line even longer and harder to read ;-)
> (Python will ensure that we have the right number of %s.)
>
> The easiest to read solution would probably be
>     return ''.join([space, self.modifiers_str(), owner, self.comment])
>
> Again, something for a follow-up patch ;-)
sure.

>
>> > +        elif not self.all_paths and not self.all_perms and path and
>> > perms: +            return('%s%s%s%s%s%s,%s' % (space,
>> > self.modifiers_str(), file_keyword, owner, path_and_perms, target,
>> > self.comment)) +        else:
>> > +            raise AppArmorBug('Invalid combination of path and
>> > perms in file rule - either specify path and perms, or none of
>> > them') +
>> > +    def _joint_perms(self):
>> > +        '''return the permissions as string'''
>> > +        perm_string = ''
>> > +        for perm in file_permissions:
>> > +            if perm in self.perms:
>> > +                perm_string = perm_string + perm
>>
>> can use filter and join over repeated concatenation:
>>
>> perm_string = "".join(filter(lambda perm: perm in self.perms,
>> file_permissions))
>
> That would put the code from 4 lines into one unreadable line ;-)
>
Maybe its just me, I've come to like chaining lambdas. :-)

>> > +        if self.exec_perms:
>> > +            perm_string = perm_string + self.exec_perms
>> > +
>> > +        return perm_string
>> > +
>> > +    def is_covered_localvars(self, other_rule):
>> > +        '''check if other_rule is covered by this rule object'''
>> > +
>>
>> > +        if not self._is_covered_aare(self.path,
> self.all_paths,         other_rule.path,        other_rule.all_paths,
> 'path'):
>> cant make-out the reason for the above spacing?
>
> It matches the spacing some lines below.
>
>> > +    def is_equal_localvars(self, rule_obj, strict):
>> > +        '''compare if rule-specific variables are equal'''
>> > +
>>
>> > +        if not type(rule_obj) == FileRule:
>> fun this we can write is as:
>> if type(rule_obj) is not FileRule
>
> Also sounds like something for the follow-up patch.
>
>> > +def split_perms(perm_string, deny):
>> > +    '''parse permission string
>> > +       - perm_string: the permission string to parse
>> > +       - deny: True if this is a deny rule
>> > +   '''
>> > +    perms = set()
>> > +    exec_mode = None
>> > +
>> > +    while perm_string:
>> > +        if perm_string[0] in file_permissions:
>> > +            perms.add(perm_string[0])
>> > +            perm_string = perm_string[1:]
>> > +        elif perm_string[0] == 'x':
>> > +            if not deny:
>> > +                raise AppArmorException(_("'x' must be preceded by
>> > an exec qualifier (i, P, C or U)")) +            exec_mode = 'x'
>> > +            perm_string = perm_string[1:]
>> > +        elif perm_string.startswith(allow_exec_transitions):
>> > +            if exec_mode:
>> > +                raise AppArmorException(_('conflicting execute
>> > permissions found: %s and %s' % (exec_mode, perm_string[0:2]))) +
>> >          exec_mode = perm_string[0:2]
>> > +            perm_string = perm_string[2:]
>> > +        elif
>> > perm_string.startswith(allow_exec_fallback_transitions) and not
>> > deny: +            if exec_mode:
>> > +                raise AppArmorException(_('conflicting execute
>> > permissions found: %s and %s' % (exec_mode, perm_string[0:3]))) +
>> >          exec_mode = perm_string[0:3]
>> > +            perm_string = perm_string[3:]
>>
>> not particularly happy about this 2/3 length thing fixed for
>> allow_exec_(fallback|)transitions.
>> Can't think of an elegant way to rewrite it, though getting the
>> matching perm and using it maybe a bit nice (may also allow combining
>> the two elif.
>
> Better ideas are always welcome ;-)
>
> Using a regex might be an idea, but I'm not sure if that makes the code
> more readable ;-)
>
> Hmm, maybe a helper function that returns the exec mode (2 or 3 chars
> long) and the remaining string? Anyway, that's another thing for a
> follow-up patch.
>
>> Mostly looks great.
>> Some minor comments for now.
>> Will try to revisit it after going through the whole series and tests
>
> Thanks for the review and the critical notes on details!
>
> I'd prefer to do the mentioned changes in a follow-up patch (maybe
> 40/38?) to avoid breaking one of the patches in the series.
>
Sure. Let's get this series to 42 ;-)

Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>

>
> Regards,
>
> Christian Boltz
> --
> If we get Flemish, we should get also Plat-Duutsch, Beirisch and
> Östereichish. (The last two are the same, but don't tell them that,
> because the inhabitants think they are not, because they are better
> then the other. :)   [houghi in opensuse]
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>


-- 
Regards,

Kshitij Gupta