[apparmor] [patch] [06/38] Add FileRule and FileRuleset
Christian Boltz
apparmor at cboltz.de
Wed Sep 14 15:25:38 UTC 2016
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?
> > + 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 ;-)
> > + 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.
> > + # 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.
> > + 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 ;-)
> > + 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 ;-)
> > + 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.
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]
-------------- 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/20160914/081457e2/attachment.pgp>
More information about the AppArmor
mailing list