Hello,
Am Montag, 24. November 2014 schrieb Steve Beattie:
> On Sat, Nov 22, 2014 at 12:38:02AM +0100, Christian Boltz wrote:
> > > Mmm, I'm still not entirely convinced, but it feels like it again
> > > might be an artifact of the hasher and/or a lack of use of classes
> > > for profiles.
> >
> > Imagine aa.py will have something like this one day (for non-clean
> > mode):
> >
> > for default_write_order as rule_type:
> > data += aa[profile][hat][rule_type].get_raw()
> > aa[profile][hat][rule_type].delete_all_rules()
> >
> > If you tell me another way to implement the last line, I'll use it
> > ;-)
> If you're deadset on keeping the references to rulesets in a
> dict-style structure indexed by a string rule_type, I can think of at
> least a couple of ways:
I don't insist on keeping it as a dict ;-)
All I want is something that is allows to have the rule type as a
variable so that we can
a) loop over all rule types
b) have something like the match_includes() function from part 3 of my
patchset that works for all rule types (with the rule_type as
parameter)
I don't really care about the data type (being it dict, a class or
something else) as long as the above is possible ;-)
> 1) we already have a mapping from rule_types to print functions. I'm
> assuming get_raw()/get_clean() are replacements for those functions,
> but you could still retain a mapping to classes, such that
>
> a[profile][hat][rule_type] = ruleset_map[rule_type]()
>
> works.
>
> 2) you could grab the type of the existing ruleset and ask it for a
> new object:
>
> t = type(a[profile][hat][rule_type])
> a[profile][hat][rule_type] = t()
>
> There are surely other ways.
TMTOWTDI? We are talking about python, not perl! ;-)
Anyway, your proposal 2) looks good, is future-proof and even works with
['include'] :-) (which exploded with v3)
Thanks for the idea!
This also means I no longer need the delete_all_rules() function which
you loved so much ;-))
> > Does this interesting[tm] behaviour have a real-world usecase?
> > ("confuse developers" doesn't count ;-)
>
> Well, I simplified things in my explanation. If you assign to field,
> the contents are local to the instance. But if you modify what the
> field refers to (e.g. list.append()), it's going to change for all
> instances, because the instances only have a stored reference to
> the modified object (i.e. the only thing stored in the instance is
> a pointer to the modified object, not the object itself).
My question was more about what the python devs smoked^Wthought when
implementing this behaviour ;-)
Thanks for the details nevertheless!
> > > At sort of a meta level, I know in lots of instances we try to
> > > avoid
> > > making unnecessary changes to policy, yet I think there is value
> > > in
> > > normalizing policy as well, which you could consider separating
> > > out a
> > > multiple capability rule into multiple one-capability-per-rule
> > > rules
> > > as being a type of normalization.
> >
> > We have get_clean() and get_raw() to explicitely choose between
> > original rules and "cleaned up" rules, so that wouldn't be a
> > problem.
> >
> > The only problem would be the data type of the *Rule.get_clean() -
> > currently it returns a string, so for multiple rules we could return
> > a multi-line string (breaks sorting) or an array of lines (which
> > would mean a little behaviour change).
> >
> > AFAIK capability rules are the only rule type with this problem, so
> > I'd like to avoid having a solution that makes our life harder for
> > all other rule types just to cover a corner case.
>
> Other than maybe having to convert to having lists of lines, if you
> encapsulate the logic for this in the capability specific classes, I'm
> not sure how it makes things more difficult for other rule types.
> And, given how long dbus rules can be, I'd bet other rules classes
> will want the ability to return multiple lines.
Well, there's a difference - multiple lines vs. multiple rules ;-)
For multi-capability rules, the perfect solution would be to return two
(or more) independent lines, basically:
[
' capability audit_write,',
' capability chown,'
]
which also means the sort() in CapabilityRuleset {sh,c}ould sort them to
the right place.
However for dbus rules, you don't want to sort the lines individually
(only the whole rule).
This means we'll have to return the dbus rule as a string with some \n,
like:
' dbus send \n
bus=session \n
path=/com/example/path \n
interface=com.example.Interface '
So I'm still not sure if multi-capability lines are worth special
handling - and if we really want it, I'd go the \n way like for dbus
rules. This breaks sorting, but avoids that we need to add special
handling for a corner case in one rule type. (I promise to fix it in the
perfect way if we get a bugreport about the broken sorting from someone
who is not reading this mailinglist ;-)
(not implemented in the patch yet - that's something for a follow-up
patch)
> > > > +# The key for representing bare rules such as "capability," or
> > > > "file," +ALL = '\0ALL'
> > >
> > > I'm not so keen on having a magic dict key to represent the bare
> > > keyword rules; one way I'd thought about handling this was to have
> > > a keyword rule be represented by a subclass of the rule type; e.g.
> > > CapabilityAllRule as a subclass of CapabilityRule, and trying to
> > > encapsulate most of the differences within that subclass.
> >
> > As discussed on IRC, I'm not too keen on adding another class just
> > to
> > avoid two "if ALL in self.capabilitiy" branches.
> >
> > I changed it to use self.all_caps to avoid having a magic key, even
> > if that makes some places slightly more interesting than they would
> > be with the ALL magic key.
>
> Another option, if you converted to storing internally as sets,
> would be to have a locally defined (frozen) ALL set, that contains
> all the known capabilities. Again, you'd have the bonus of having
> set operations work correctly.
That sounds like making things more complicated ;-)
"All capabilities" should just mean that, without checking against a
list of known capabilities.)
> (Even better if libapparmor exported in one place a list of all the
> known capabilities that the python tools could use to construct the
> ALL set, as well as to ease checking whether a capability is valid or
> not.)
Can we first fix the parser build so that the list of capabilities is
complete even if compiled on an old kernel (bugreport exists) before
thinking about that? ;-)
> > I changed that, and also added init_vars() that is called by
> > __init__
> > (contains just "pass" in BaseRule/BaseRuleset) so that the child
> > classes don't need to re-implement or call the parent __init__ when
> > they need to add some variables.
>
> Can you rename this function as _init_vars() to indicate that it is
> internal to this class hierarchy, and not to be invoked by outsiders?
Done.
> > > I suspect we will want a rule equality test method.
> >
> > Yes. I added is_equal() (to BaseRule) which calls
> > is_equal_localvars() in CapabilityRule.
>
> Err. Why is it not just an overridable method? Pretty much every rule
> type is going to have it's own equality test, I'd bet, If there are
> common subchecks (audit, deny, allow; comments, maybe), have the base
> class make those available.
That's exactly what I did ;-) - is_equal() is in the base class and
checks things like audit and deny. After that, it calls
is_equal_localvars() in CapabilityRule to check the capability-specific
things.
This way has the advantage that we don't need to use super() in
CapabilityRules to do the base checks.
> > The question is how strict is_equal should be, for example
> > capability chown vs. allow capability chown
> > capability chown, vs capability chown, # comment
> >
> > Technically they are equal, so I disabled the checks for everything
> > that doesn't change the meaning of a rule ("allow" keyword and
> > comment - and also rawrule because it includes those)
>
> Hrm. I can see where both types of equality tests, logical and strict,
> would be useful. So a common pythonic idiom would be to take an
> additional named argument, e.g. strict=, that when set to False
> returns logical equality, and when True, returns a strict (comments,
> modifiers, etc.) equality test result. What the default should be
> would probably be best driven by what the more common need is.
Ah, the usual answer - "both of them" ;-)
I added the strict= parameter - False by default because I think users
typically are interested in behaviour, not in having exactly the same
comment ;-)
> > > > + def set_log(self, parsed_log_event):
> > > > + '''parse and store log event'''
> > > > +
> > > > + if parsed_log_event['operation'] != 'capable':
> > > > + raise AppArmorBug('Invalid capability log event -
> > > > not a
> > > > capability event') +
> > > > + if not parsed_log_event['name']:
> > > > + raise AppArmorBug('Invalid capability log event -
> > > > name
> > > > is empty') +
> > > > + self.capability = [ parsed_log_event['name'] ]
> > > > +
> > > > + self.logmode = parsed_log_event['aamode']
> > >
> > > What is logmode saved for?
> >
> > aa-logprof uses it, and the default answer (allow or deny) depends
> > on
> > complain vs. enforce mode. Oh, and it prints a nice headline like
> > "Changes in compline mode" ;-)
> >
> > That's exactly how the perl tools worked. The question is if we want
> > to keep this behaviour, or want to change it.
>
> Right. I don't have a problem with the behavior, per se. Rather
> than have a completely different set of _log() operations, I would
The *_log() operations make sense (they are still unused, but that will
change sooner or later).
> store the log event as a separate optional argument to __init__()
> (e.g. log_event=None) that is stored off as-is, and then any
> information that tools want to report about the log event that (first)
> generated the rule can be pulled directly from the log event the Rule
> hands back, if there is one to hand back.
Please allow me to disagree ;-)
I want to have log events as normal rule objects so that we can pass
them to is_covered() etc. without a need to "convert" them.
(That obviously needs changes in log handling/parsing, see my /dev/brain
for details ;-)
If we find out the tools need more log event details (besides the rule-
relevant information) than just complain vs. enforce mode, we can change
the storage to a self.log_details dict later.
> (I had a thought of supporting a list of log events, so that you could
> report statistics on how many events were covered by a given rule
> during a tooling run, but given how poorly we handle large log files
> already, keeping references to multiple events won't help with our
> memory usage. But we could perhaps add a counter.)
Indeed, adding a counter shouldn't be too expensive ;-)
Can you please remind me when we have the log handling converted to rule
objects?
The updated patch (v4) is attached. Changes since v3:
- BaseRule:
- rename init_vars() to _init_vars()
- add 'strict' parameter to is_equal()
- BaseRuleset:
- remove delete_all_rules() function and merge it into __init__()
- CapabilityRule:
- change self.capability to set() (based on the patch from Steve)
Regards,
Christian Boltz
--
Tatsächlich gibt es nach wie vor sogar OS, die auf systemd und
upstart initial verzichten. Kann natürlich sein, dass man dazu
die Yast-Hängematte verlassen muss.
[Tobias Crefeld in opensuse-de]=== added directory 'utils/apparmor/rule'
=== added file 'utils/apparmor/rule/__init__.py'
--- utils/apparmor/rule/__init__.py 1970-01-01 00:00:00 +0000
+++ utils/apparmor/rule/__init__.py 2014-11-27 19:26:13 +0000
@@ -0,0 +1,247 @@
+# ----------------------------------------------------------------------
+# Copyright (C) 2013 Kshitij Gupta <[email protected]>
+# Copyright (C) 2014 Christian Boltz <[email protected]>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of version 2 of the GNU General Public
+# License as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# ----------------------------------------------------------------------
+
+from apparmor.common import AppArmorBug
+
+# setup module translations
+from apparmor.translations import init_translation
+_ = init_translation()
+
+
+class BaseRule(object):
+ '''Base class to handle and store a single rule'''
+
+ def __init__(self):
+ '''initialize variables needed by all rule types
+ Do not override in child class unless really needed - override _init_vars() instead'''
+ self.audit = False
+ self.deny = False
+ self.allow_keyword = False
+
+ self.inlinecomment = ''
+
+ self.rawrule = ''
+ self.logmode = '' # only set by set_log()
+
+ self._init_vars()
+
+ def _init_vars(self):
+ '''called by __init__() - override in child class to initialize more variables'''
+ raise AppArmorBug('_init_vars() needs to be implemented in the child class')
+
+ def get_raw(self, depth):
+ '''return raw rule (with original formatting, and leading whitespace in the depth parameter)'''
+ if self.rawrule:
+ return '%s%s' % (' ' * depth, self.rawrule)
+ else:
+ return self.get_clean(depth)
+
+ def is_equal(self, rule_obj, strict=False):
+ '''compare if rule_obj == self
+ Calls is_equal_localvars() to compare rule-specific variables'''
+
+ if self.audit != rule_obj.audit or self.deny != rule_obj.deny:
+ return False
+
+ if strict and (
+ self.allow_keyword != rule_obj.allow_keyword
+ or self.inlinecomment != rule_obj.inlinecomment
+ or self.rawrule != rule_obj.rawrule
+ ):
+ return False
+
+ return self.is_equal_localvars(rule_obj)
+
+ def modifiers_str(self):
+ '''return the allow/deny and audit keyword as string, including whitespace'''
+
+ if self.audit:
+ auditstr = 'audit '
+ else:
+ auditstr = ''
+
+ if self.deny:
+ allowstr = 'deny '
+ elif self.allow_keyword:
+ allowstr = 'allow '
+ else:
+ allowstr = ''
+
+ return '%s%s' % (auditstr, allowstr)
+
+ def parse_audit_allow(self, matches):
+ '''returns audit, deny, allow_keyword and comment from the matches object
+ - audit, deny and allow_keyword are True/False
+ - comment is the comment with a leading space'''
+ audit = False
+ if matches.group('audit'):
+ audit = True
+
+ deny = False
+ allow_keyword = False
+
+ allowstr = matches.group('allow')
+ if allowstr:
+ if allowstr.strip() == 'allow':
+ allow_keyword = True
+ elif allowstr.strip() == 'deny':
+ deny = True
+ else:
+ raise AppArmorBug("Invalid allow/deny keyword %s" % allowstr)
+
+ comment = ''
+ if matches.group('comment'):
+ # include a space so that we don't need to add it everywhere when writing the rule
+ comment = ' %s' % matches.group('comment')
+
+ return (audit, deny, allow_keyword, comment)
+
+
+class BaseRuleset(object):
+ '''Base class to handle and store a collection of rules'''
+
+ # decides if the (G)lob and Glob w/ (E)xt options are displayed
+ can_glob = True
+ can_glob_ext = False
+
+ def __init__(self):
+ '''initialize variables needed by all ruleset types
+ Do not override in child class unless really needed - override _init_vars() instead'''
+ self.rules = []
+ self._init_vars()
+
+ def _init_vars(self):
+ '''called by __init__() and delete_all_rules() - override in child class to initialize more variables'''
+ pass
+
+ def add_raw(self, rawrule):
+ '''parse rawrule (from profile file) and store it in a structured way'''
+
+ newrule = self.new_rule()
+ newrule.set_raw(rawrule)
+ self.rules.append(newrule)
+
+ def add_obj(self, rule_obj):
+ '''add a rule object'''
+ self.rules.append(rule_obj)
+
+ def get_raw(self, depth):
+ '''return all raw rules (if possible/not modified in their original formatting).
+ Returns an array of lines, with depth * leading whitespace'''
+
+ data = []
+ for rule in self.rules:
+ data.append(rule.get_raw(depth))
+
+ if data:
+ data.append('')
+
+ return data
+
+ def get_clean(self, depth):
+ '''return all rules (in clean/default formatting)
+ Returns an array of lines, with depth * leading whitespace'''
+
+ allow_rules = []
+ deny_rules = []
+
+ for rule in self.rules:
+ if rule.deny:
+ deny_rules.append(rule.get_clean(depth))
+ else:
+ allow_rules.append(rule.get_clean(depth))
+
+ allow_rules.sort()
+ deny_rules.sort()
+
+ cleandata = []
+
+ if deny_rules:
+ cleandata += deny_rules
+ cleandata.append('')
+
+ if allow_rules:
+ cleandata += allow_rules
+ cleandata.append('')
+
+ return cleandata
+
+ def is_obj_covered(self, rule_obj, check_allow_deny=True, check_audit=False):
+ '''return True if rule_obj is covered by existing rules, otherwise False'''
+
+ for rule in self.rules:
+ if rule.is_covered(rule_obj, check_allow_deny, check_audit):
+ return True
+
+ return False
+
+ def is_log_covered(self, parsed_log_event, check_allow_deny=True, check_audit=False):
+ '''return True if parsed_log_event is covered by existing rules, otherwise False'''
+
+ rule_obj = self.new_rule()
+ rule_obj.set_log(parsed_log_event)
+
+ return self.is_obj_covered(rule_obj, check_allow_deny, check_audit)
+
+ def is_raw_covered(self, rawrule, check_allow_deny=True, check_audit=False):
+ '''return True if rawrule is covered by existing rules, otherwise False'''
+
+ rule_obj = self.new_rule()
+ rule_obj.set_raw(rawrule)
+
+ return self.is_obj_covered(rule_obj, check_allow_deny, check_audit)
+
+ def delete_obj(self, rule_obj):
+ '''Delete rule_obj from rules'''
+
+ rule_to_delete = None
+ i = 0
+ for rule in self.rules:
+ i = i + 1
+ if rule.is_equal(rule_obj):
+ rule_to_delete = i
+ break
+
+ if rule_to_delete:
+ self.rules.pop(i-1)
+ else:
+ raise AppArmorBug('Attemp to delete non-existing rule %s' % rule_obj.get_raw(0))
+
+ def delete_raw(self, rawrule):
+ '''Delete rawrule from rules'''
+
+ rule_obj = self.new_rule()
+ rule_obj.set_raw(rawrule)
+
+ return self.delete_obj(rule_obj)
+
+ def delete_duplicates(self, include_rules):
+ '''Delete duplicate rules.
+ include_rules must be a *_rules object'''
+ deleted = []
+ if include_rules: # avoid breakage until we have a proper function to ensure all profiles contain all *_rules objects
+ for rule_obj in self.rules:
+ if include_rules.is_obj_covered(rule_obj, True, True):
+ self.delete_obj(rule_obj)
+ deleted.append(rule_obj)
+
+ return len(deleted)
+
+ def get_glob_ext(self, path_or_rule):
+ '''returns the next possible glob with extension (for file rules only).
+ For all other rule types, raise an exception'''
+ raise AppArmorBug("get_glob_ext is not available for this rule type!")
+
+
=== added file 'utils/apparmor/rule/capability.py'
--- utils/apparmor/rule/capability.py 1970-01-01 00:00:00 +0000
+++ utils/apparmor/rule/capability.py 2014-11-26 18:13:03 +0000
@@ -0,0 +1,124 @@
+# ----------------------------------------------------------------------
+# Copyright (C) 2013 Kshitij Gupta <[email protected]>
+# Copyright (C) 2014 Christian Boltz <[email protected]>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of version 2 of the GNU General Public
+# License as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# ----------------------------------------------------------------------
+
+from apparmor.regex import RE_PROFILE_CAP
+from apparmor.common import AppArmorBug, AppArmorException
+from apparmor.rule import BaseRule, BaseRuleset
+import re
+
+# setup module translations
+from apparmor.translations import init_translation
+_ = init_translation()
+
+
+class CapabilityRule(BaseRule):
+ '''Class to handle and store a single capability rule'''
+
+ def _init_vars(self):
+ self.capability = set() # enforces in-rule deduplicates ("capability chown chown,")
+ self.all_caps = False
+
+ def get_clean(self, depth):
+ '''return rule (in clean/default formatting)'''
+
+ space = ' ' * depth
+ if self.all_caps:
+ return('%s%scapability,%s' % (space, self.modifiers_str(), self.inlinecomment))
+ else:
+ caps = ' '.join(self.capability).strip() # XXX return multiple lines, one for each capability, instead?
+ if caps:
+ return('%s%scapability %s,%s' % (space, self.modifiers_str(), ' '.join(sorted(self.capability)), self.inlinecomment))
+ else:
+ raise AppArmorBug("Empty capability rule")
+
+ def set_raw(self, rawrule):
+ '''parse and store rawrule'''
+
+ matches = RE_PROFILE_CAP.search(rawrule)
+ if not matches:
+ raise AppArmorException(_("Invalid capability rule '%s'") % rawrule)
+
+ self.rawrule = rawrule.strip()
+
+ self.audit, self.deny, self.allow_keyword, self.inlinecomment = self.parse_audit_allow(matches)
+
+ if matches.group('capability'):
+ capability = matches.group('capability').strip()
+ self.capability = set(re.split("[ \t]+", capability))
+ else:
+ self.all_caps = True
+
+ def set_log(self, parsed_log_event):
+ '''parse and store log event'''
+
+ if parsed_log_event['operation'] != 'capable':
+ raise AppArmorBug('Invalid capability log event - not a capability event')
+
+ if not parsed_log_event['name']:
+ raise AppArmorBug('Invalid capability log event - name is empty')
+
+ self.capability = {parsed_log_event['name']}
+
+ self.logmode = parsed_log_event['aamode']
+
+ def set_param(self, capability, audit=False, deny=False):
+ self.capability = {capability}
+ self.audit = audit
+ self.deny = deny
+
+ def is_covered(self, rule_obj, check_allow_deny=True, check_audit=False):
+ '''check if rule_obj is covered by this rule object'''
+
+ if check_allow_deny and self.deny != rule_obj.deny:
+ return False
+
+ if not rule_obj.capability and not rule_obj.all_caps:
+ raise AppArmorBug('No capability specified')
+
+ if not self.all_caps:
+ if rule_obj.all_caps:
+ return False
+ if not rule_obj.capability.issubset(self.capability):
+ return False
+
+ if check_audit and rule_obj.audit != self.audit:
+ return False
+
+ if rule_obj.audit and not self.audit:
+ 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 ( self.capability != rule_obj.capability
+ or self.all_caps != rule_obj.all_caps ):
+ return False
+
+ return True
+
+class CapabilityRuleset(BaseRuleset):
+ '''Class to handle and store a collection of capability rules'''
+
+ def new_rule(self):
+ '''tiny helper function that allows to keep several functions to parent class'''
+ return CapabilityRule()
+
+ def get_glob(self, path_or_rule):
+ '''Return the next possible glob. For capability rules, that's always "capability," (all capabilities)'''
+ return 'capability,'
+
--
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
