[apparmor] [1/4] Add ChangeProfileRule and ChangeProfileRuleset classes

[Seth Arnold]

On Sat, May 09, 2015 at 10:35:54PM +0200, Christian Boltz wrote:

Hi Christian, just some quick notes from a first read...

> [ 01-add-ChangeProfileRule.diff ]
> 
...
> +
> +class ChangeProfileRule(BaseRule):
> +    '''Class to handle and store a single network rule'''
> +

"network rule" is a holdover..

> +    # Nothing external should reference this class, all external users
> +    # should reference the class field ChangeProfileRule.ALL
> +    class __ChangeProfileAll(object):
> +        pass
> +
> +    ALL = __ChangeProfileAll
> +
> +    def __init__(self, execcond, targetprofile, audit=False, deny=False, allow_keyword=False,
> +                 comment='', log_event=None):
> +
> +        '''
> +            CHANGE_PROFILE RULE = 'change_profile' [ EXEC COND ] [ -> PROGRAMCHILD ]
> +        '''
> +
> +        super(ChangeProfileRule, self).__init__(audit=audit, deny=deny,
> +                                             allow_keyword=allow_keyword,
> +                                             comment=comment,
> +                                             log_event=log_event)
> +
> +        self.execcond = None
> +        self.all_execconds = False
> +        if execcond == ChangeProfileRule.ALL:
> +            self.all_execconds = True
> +        elif type(execcond) == str:
> +            if execcond.strip():
> +                if execcond.startswith('/'):
> +                    self.execcond = execcond
> +                else:
> +                    raise AppArmorException('Exec condition in change_profile rule does not start with /: %s' % str(execcond))
> +            else:
> +                raise AppArmorBug('Empty exec condition in change_profile rule')
> +        else:
> +            raise AppArmorBug('Passed unknown object to ChangeProfileRule: %s' % str(execcond))

I'm quite confused by the all_execconds portions of this class; "if"
statements nested three levels deep, with multiple else clauses, is a lot
to digest. I had trouble seeing what all_execconds buys compared to doing
the other comparisons with execcond directly.

There's also mixed parsing here, with strip() and startswith(), and some
extra parsing going on in the _parse() function. I wonder if there is a
better division of parsing available.

> +
> +        self.targetprofile = None
> +        self.all_targetprofiles = False
> +        if targetprofile == ChangeProfileRule.ALL:
> +            self.all_targetprofiles = True
> +        elif type(targetprofile) == str:
> +            if targetprofile.strip():
> +                self.targetprofile = targetprofile
> +            else:
> +                raise AppArmorBug('Empty target profile in change_profile rule')
> +        else:
> +            raise AppArmorBug('Passed unknown object to ChangeProfileRule: %s' % str(targetprofile))
> +
> +    @classmethod
> +    def _match(cls, raw_rule):
> +        return RE_PROFILE_CHANGE_PROFILE_2.search(raw_rule)
> +
> +    @classmethod
> +    def _parse(cls, raw_rule):
> +        '''parse raw_rule and return ChangeProfileRule'''
> +
> +        matches = cls._match(raw_rule)
> +        if not matches:
> +            raise AppArmorException(_("Invalid change_profile rule '%s'") % raw_rule)
> +
> +        audit, deny, allow_keyword, comment = parse_modifiers(matches)
> +
> +        if matches.group('execcond'):
> +            execcond = strip_quotes(matches.group('execcond'))
> +        else:
> +            execcond = ChangeProfileRule.ALL
> +
> +        if matches.group('targetprofile'):
> +            targetprofile = strip_quotes(matches.group('targetprofile'))
> +        else:
> +            targetprofile = ChangeProfileRule.ALL
> +
> +        return ChangeProfileRule(execcond, targetprofile,
> +                           audit=audit, deny=deny, allow_keyword=allow_keyword, comment=comment)
> +
> +    def get_clean(self, depth=0):
> +        '''return rule (in clean/default formatting)'''
> +
> +        space = '  ' * depth
> +
> +        if self.all_execconds:
> +            execcond = ''
> +        elif self.execcond:
> +            execcond = ' %s' % quote_if_needed(self.execcond)
> +        else:
> +            raise AppArmorBug('Empty execcond in change_profile rule')
> +
> +        if self.all_targetprofiles:
> +            targetprofile = ''
> +        elif self.targetprofile:
> +            targetprofile = ' -> %s' % quote_if_needed(self.targetprofile)
> +        else:
> +            raise AppArmorBug('Empty target profile in change_profile rule')
> +
> +        return('%s%schange_profile%s%s,%s' % (space, self.modifiers_str(), execcond, targetprofile, self.comment))
> +
> +    def is_covered_localvars(self, other_rule):
> +        '''check if other_rule is covered by this rule object'''
> +
> +        if not other_rule.execcond and not other_rule.all_execconds:
> +            raise AppArmorBug('No execcond specified in other change_profile rule')
> +
> +        if not other_rule.targetprofile and not other_rule.all_targetprofiles:
> +            raise AppArmorBug('No target profile specified in other change_profile rule')
> +
> +        if not self.all_execconds:
> +            if other_rule.all_execconds:
> +                return False
> +            if other_rule.execcond != self.execcond:
> +                # TODO: honor globbing and variables
> +                return False
> +
> +        if not self.all_targetprofiles:
> +            if other_rule.all_targetprofiles:
> +                return False
> +            if other_rule.targetprofile != self.targetprofile:
> +                return False
> +
> +        # still here? -> then it is covered
> +        return True
> +
> +    def is_equal_localvars(self, rule_obj):
> +        '''compare if rule-specific variables are equal'''
> +
> +        if not type(rule_obj) == ChangeProfileRule:
> +            raise AppArmorBug('Passed non-change_profile rule: %s' % str(rule_obj))
> +
> +        if (self.execcond != rule_obj.execcond
> +                or self.all_execconds != rule_obj.all_execconds):
> +            return False
> +
> +        if (self.targetprofile != rule_obj.targetprofile
> +                or self.all_targetprofiles != rule_obj.all_targetprofiles):
> +            return False
> +
> +        return True
> +
> +
> +class ChangeProfileRuleset(BaseRuleset):
> +    '''Class to handle and store a collection of change_profile rules'''
> +
> +    def get_glob(self, path_or_rule):
> +        '''Return the next possible glob. For change_profile rules, that can be "change_profile EXECCOND,",
> +           "change_profile -> TARGET_PROFILE," or "change_profile," (all change_profile).
> +           Also, EXECCOND filename can be globbed'''
> +        # XXX implement all options mentioned above ;-)
> +        return 'change_profile,'

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150526/2b7f6368/attachment.pgp>