[apparmor] [patch 3/3] use capability rule class in aa.py and cleanprof.py
Steve Beattie
steve at nxnw.org
Fri Nov 21 18:52:17 UTC 2014
On Sat, Nov 15, 2014 at 11:46:41PM +0100, Christian Boltz wrote:
> Hello,
>
> this patch changes aa.py and cleanprof.py to use the new capabiliy rule
> class.
>
> The most important details in the change are:
> - the capability rules are stored in
> aa[profile][hat]['capability'] instead of
> aa[profile][hat]['allow']['capability'] and
> aa[profile][hat]['deny']['capability']
> (allow/deny is handled inside the capability_rules class)
Good.
> - profile_known_capability() now just returns True or False. Before, it
> returned different values for deny and allow, but the calling code
> doesn't care about this detail anyway.
For something that returns a boolean value, it feels like an awkwardly
named function. is_known_capability?
> Some things are still a bit ugly (and commented as such). On the long
> term, I plan to change logparser so that it returns a set of *_rule
> classes instead of the current array with parsed events. This will
> remove some of the ugly tricks I had to add.
>
> We should also come up with a function that initializes the structure
> for each profile in aa[profile][hat] - with that, we could drop several
> safety checks I had to add to avoid problems with profiles that don't
> contain a capability rule.
Ideally, we move to profiles/hats being encapsulated in a class of
their own, for which the __init__() method can properly initialize
the fields of the instance. In the short term, can't we add a
CapabilityRuleset when we start parsing a new profile/hat?
> I also had to add several
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> in serialize_profile_from_old_profile(). That's needed to avoid writing
> rules twice.
Is this rule writing duplication because we no longer have an 'allow'
and 'deny' layer in the hasher hierarchy for capabilities?
> On the positive side, when we have converted everything to
> rule classes, we can delete the four lines above each of these added
> lines :-)
>
> The patch also contains a small bugfix - when choosing "deny" for a
> capability, the audit flag was ignored. Maybe it also fixes some other
> small bugs we didn't even notice before ;-)
>
>
> Let me finalize this patch series with the line count statistics:
> 1-add-base-and-capability-rule-class.diff - 329 lines added, 0 removed
> 2-add-capability-rule-test.diff - 657 lines added, 0 removed
> 3-use-capability-rule-class.diff - 62 lines added, 107 removed
>
> That all said - I'm looking forward for feedback ;-)
>
> Enjoy reviewing the patches!
>
>
> Regards,
>
> Christian Boltz
> --
> PHP bietet einige Möglicheiten etwas falsch zu machen.
> Die phpBB Entwickler haben wohl viele dieser Möglichkeiten genutzt.
> [Kommentar auf http://www.pro-linux.de/news/2007/12106.html]
> === modified file 'utils/apparmor/aa.py'
> --- utils/apparmor/aa.py 2014-11-15 11:51:24 +0000
> +++ utils/apparmor/aa.py 2014-11-15 21:14:56 +0000
> @@ -52,6 +52,8 @@
>
> import apparmor.rules as aarules
>
> +from apparmor.rule.capability import capability_rules, capability_rule
> +
> from apparmor.yasti import SendDataToYast, GetDataFromYast, shutdown_yast
>
> # setup module translations
> @@ -89,7 +91,7 @@
> # To store the globs entered by users so they can be provided again
> user_globs = []
>
> -# The key for representing bare rules such as "capability," or "file,"
> +# The key for representing bare "file," rules
> ALL = '\0ALL'
>
> ## Variables used under logprof
> @@ -1628,16 +1630,16 @@
> if deleted:
> aaui.UI_Info(_('Deleted %s previous matching profile entries.') % deleted)
>
> - aa[profile][hat]['allow']['capability'][capability]['set'] = True
> - aa[profile][hat]['allow']['capability'][capability]['audit'] = audit_toggle
> + else:
> + aa[profile][hat]['capability'].add_raw('%s capability %s,'% (audit, capability)) # XXX yes, ugly ;-)
I agree it's ugly, it suggests our interfaces into the classes are
suboptimal. Why can't we have something like:
cap_rule = CapabilityRule(capability, audit=audit)
aa[profile][hat]['capability'].add(cap_rule)
rather than playing games consing up strings. (Yes, you could write the
two lines as a one-liner if you really wanted and skip the temporary
cap_rule variable).
> + aaui.UI_Info(_('Adding capability %s to profile.') % capability)
>
> changed[profile] = True
>
> - aaui.UI_Info(_('Adding capability %s to profile.') % capability)
> done = True
>
> elif ans == 'CMD_DENY':
> - aa[profile][hat]['deny']['capability'][capability]['set'] = True
> + aa[profile][hat]['capability'].add_raw('%s deny capability %s,'% (audit, capability)) # XXX yes, ugly ;-)
> changed[profile] = True
>
> aaui.UI_Info(_('Denying capability %s to profile.') % capability)
> @@ -2121,20 +2123,6 @@
> deleted += 1
> return deleted
>
> -def delete_cap_duplicates(profilecaps, inccaps):
> - deleted = []
> - if profilecaps and inccaps:
> - for capname in profilecaps.keys():
> - # XXX The presence of a bare capability rule ("capability,") should
> - # cause more specific capability rules
> - # ("capability audit_control,") to be deleted
> - if inccaps[capname].get('set', False) == 1:
> - deleted.append(capname)
> - for capname in deleted:
> - profilecaps.pop(capname)
> -
> - return len(deleted)
> -
> def delete_path_duplicates(profile, incname, allow):
> deleted = []
> for entry in profile[allow]['path'].keys():
> @@ -2161,9 +2149,7 @@
>
> deleted += delete_net_duplicates(profile['deny']['netdomain'], include[incname][incname]['deny']['netdomain'])
>
> - deleted += delete_cap_duplicates(profile['allow']['capability'], include[incname][incname]['allow']['capability'])
> -
> - deleted += delete_cap_duplicates(profile['deny']['capability'], include[incname][incname]['deny']['capability'])
> + deleted += profile['capability'].delete_duplicates(include[incname][incname]['capability'])
>
> deleted += delete_path_duplicates(profile, incname, 'allow')
> deleted += delete_path_duplicates(profile, incname, 'deny')
> @@ -2173,9 +2159,7 @@
>
> deleted += delete_net_duplicates(profile['deny']['netdomain'], filelist[incname][incname]['deny']['netdomain'])
>
> - deleted += delete_cap_duplicates(profile['allow']['capability'], filelist[incname][incname]['allow']['capability'])
> -
> - deleted += delete_cap_duplicates(profile['deny']['capability'], filelist[incname][incname]['deny']['capability'])
> + deleted += profile['capability'].delete_duplicates(filelist[incname][incname]['capability'])
>
> deleted += delete_path_duplicates(profile, incname, 'allow')
> deleted += delete_path_duplicates(profile, incname, 'deny')
> @@ -2206,7 +2190,9 @@
> def match_cap_includes(profile, cap):
> newincludes = []
> for incname in include.keys():
> - if valid_include(profile, incname) and include[incname][incname]['allow']['capability'][cap].get('set', False) == 1:
> + # XXX type check should go away once we init all profiles correctly
> + # XXX using a capability_rule object instead of covered_raw() might be nice/faster
> + if valid_include(profile, incname) and type(include[incname][incname]['capability']) == capability_rules and include[incname][incname]['capability'].covered_raw("capability %s," % cap):
> newincludes.append(incname)
>
> return newincludes
> @@ -2516,7 +2502,8 @@
>
> for capability in prelog[aamode][profile][hat]['capability'].keys():
> # If capability not already in profile
> - if not aa[profile][hat]['allow']['capability'][capability].get('set', False):
> + # XXX remove first check when we have proper profile initialisation
> + if aa[profile][hat].get('capability', False) and not aa[profile][hat]['capability'].covered_raw("capability %s," % capability):
> log_dict[aamode][profile][hat]['capability'][capability] = True
>
> nd = prelog[aamode][profile][hat]['netdomain']
> @@ -2704,6 +2691,10 @@
> profile_data[profile][profile]['repo']['url'] = repo_data['url']
> profile_data[profile][profile]['repo']['user'] = repo_data['user']
>
> + # init rule classes (if not done yet)
> + if not profile_data[profile][hat].get('capability', False):
> + profile_data[profile][hat]['capability'] = capability_rules()
> +
> elif RE_PROFILE_END.search(line):
> # If profile ends and we're not in one
> if not profile:
> @@ -2719,21 +2710,14 @@
> initial_comment = ''
>
> elif RE_PROFILE_CAP.search(line):
> - matches = RE_PROFILE_CAP.search(line)
> -
> if not profile:
> raise AppArmorException(_('Syntax Error: Unexpected capability entry found in file: %(file)s line: %(line)s') % { 'file': file, 'line': lineno + 1 })
>
> - audit, allow, allow_keyword, comment = parse_audit_allow(matches)
> - # TODO: honor allow_keyword and comment
> -
> - capability = ALL
> - if matches.group('capability'):
> - capability = matches.group('capability').strip()
> - # TODO: can contain more than one capability- split it?
> -
> - profile_data[profile][hat][allow]['capability'][capability]['set'] = True
> - profile_data[profile][hat][allow]['capability'][capability]['audit'] = audit
> + # init rule class (if not done yet)
> + if not profile_data[profile][hat].get('capability', False):
> + profile_data[profile][hat]['capability'] = capability_rules()
> +
> + profile_data[profile][hat]['capability'].add_raw(line)
>
> elif RE_PROFILE_LINK.search(line):
> matches = RE_PROFILE_LINK.search(line).groups()
> @@ -3373,29 +3357,8 @@
> def write_list_vars(prof_data, depth):
> return write_pair(prof_data, depth, '', 'lvar', '', ' = ', '', var_transform)
>
> -def write_cap_rules(prof_data, depth, allow):
> - pre = ' ' * depth
> - data = []
> - allowstr = set_allow_str(allow)
> -
> - if prof_data[allow].get('capability', False):
> - for cap in sorted(prof_data[allow]['capability'].keys()):
> - audit = ''
> - if prof_data[allow]['capability'][cap].get('audit', False):
> - audit = 'audit '
> - if prof_data[allow]['capability'][cap].get('set', False):
> - if cap == ALL:
> - data.append('%s%s%scapability,' % (pre, audit, allowstr))
> - else:
> - data.append('%s%s%scapability %s,' % (pre, audit, allowstr, cap))
> - data.append('')
> -
> - return data
> -
> def write_capabilities(prof_data, depth):
> - #data = write_single(prof_data, depth, '', 'set_capability', 'set capability ', ',')
> - data = write_cap_rules(prof_data, depth, 'deny')
> - data += write_cap_rules(prof_data, depth, 'allow')
> + data = prof_data['capability'].get_clean(depth)
> return data
>
> def write_net_rules(prof_data, depth, allow):
> @@ -3886,6 +3849,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
>
> # then write everything else
> for segs in default_write_order:
> @@ -3927,25 +3892,7 @@
> profile = None
>
> elif RE_PROFILE_CAP.search(line):
> - matches = RE_PROFILE_CAP.search(line).groups()
> - audit = False
> - if matches[0]:
> - audit = matches[0]
> -
> - allow = 'allow'
> - if matches[1] and matches[1].strip() == 'deny':
> - allow = 'deny'
> -
> - capability = ALL
> - if matches[2]:
> - capability = matches[2].strip()
> -
> - if not write_prof_data[hat][allow]['capability'][capability].get('set', False):
> - correct = False
> - if not write_prof_data[hat][allow]['capability'][capability].get(audit, False) == audit:
> - correct = False
> -
> - if correct:
> + if write_prof_data[hat]['capability'].covered_raw(line, True, True):
> if not segments['capability'] and True in segments.values():
> for segs in list(filter(lambda x: segments[x], segments.keys())):
> depth = len(line) - len(line.lstrip())
> @@ -3955,13 +3902,11 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['capability'] = True
> - write_prof_data[hat][allow]['capability'].pop(capability)
> + write_prof_data[hat]['capability'].delete_raw(line)
> data.append(line)
> -
> - #write_prof_data[hat][allow]['capability'][capability].pop(audit)
> -
> - #Remove this line
> else:
> # To-Do
> pass
> @@ -3996,6 +3941,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['link'] = True
> write_prof_data[hat][allow]['link'].pop(link)
> data.append(line)
> @@ -4020,6 +3967,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['change_profile'] = True
> write_prof_data[hat]['change_profile'].pop(cp)
> data.append(line)
> @@ -4050,6 +3999,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['alias'] = True
> if profile:
> write_prof_data[hat]['alias'].pop(from_name)
> @@ -4079,6 +4030,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['rlimit'] = True
> write_prof_data[hat]['rlimit'].pop(from_name)
> data.append(line)
> @@ -4104,6 +4057,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['lvar'] = True
> write_prof_data[hat]['lvar'].pop(bool_var)
> data.append(line)
> @@ -4135,6 +4090,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['lvar'] = True
> if profile:
> write_prof_data[hat]['lvar'].pop(list_var)
> @@ -4173,6 +4130,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['path'] = True
> write_prof_data[hat][allow]['path'].pop(ALL)
> data.append(line)
> @@ -4221,6 +4180,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['path'] = True
> write_prof_data[hat][allow]['path'].pop(path)
> data.append(line)
> @@ -4241,6 +4202,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['include'] = True
> write_prof_data[hat]['include'].pop(include_name)
> data.append(line)
> @@ -4294,6 +4257,8 @@
> write_prof_data[name]['allow'].pop(segs)
> if write_prof_data[name]['deny'].get(segs, False):
> write_prof_data[name]['deny'].pop(segs)
> + if write_prof_data[name].get(segs, False):
> + write_prof_data[name][segs].delete_all_rules()
> segments['netdomain'] = True
>
> elif RE_PROFILE_CHANGE_HAT.search(line):
> @@ -4408,19 +4373,20 @@
> return 0
>
> def profile_known_capability(profile, capname):
> - if profile['deny']['capability'][capname].get('set', False):
> - return -1
> + rule = capability_rule()
> + rule.set_raw('capability %s,' % capname) # having a capability_rule object as parameter would be even better ;-)
Or, you know, have the initializer for the class take an optional
(or maybe not even optional) capname argument.
> - if profile['allow']['capability'][capname].get('set', False):
> - return 1
> + # XXX get rid of type checks after we have a proper function to initialize a profile
> + if type(profile['capability']) == capability_rules:
> + if profile['capability'].covered_obj(rule, False):
> + return True
>
> for incname in profile['include'].keys():
> - if include[incname][incname]['deny']['capability'][capname].get('set', False):
> - return -1
> - if include[incname][incname]['allow']['capability'][capname].get('set', False):
> - return 1
> + if type(include[incname][incname]['capability']) == capability_rules:
> + if include[incname][incname]['capability'].covered_obj(rule, False):
> + return True
>
> - return 0
> + return False
>
> def profile_known_network(profile, family, sock_type):
> if netrules_access_check(profile['deny']['netdomain'], family, sock_type):
>
> === modified file 'utils/apparmor/cleanprofile.py'
> --- utils/apparmor/cleanprofile.py 2014-09-05 21:21:00 +0000
> +++ utils/apparmor/cleanprofile.py 2014-11-15 18:48:23 +0000
> @@ -65,8 +65,8 @@
> deleted += apparmor.aa.delete_duplicates(self.other.aa[program][hat], inc)
>
> #Clean the duplicates of caps in other profile
> - deleted += delete_cap_duplicates(self.profile.aa[program][hat]['allow']['capability'], self.other.aa[program][hat]['allow']['capability'], self.same_file)
> - deleted += delete_cap_duplicates(self.profile.aa[program][hat]['deny']['capability'], self.other.aa[program][hat]['deny']['capability'], self.same_file)
> + if self.same_file:
> + deleted += self.other.aa[program][hat]['capability'].delete_duplicates(self.profile.aa[program][hat]['capability'])
Doesn't this need the same type check, to make sure that
self.other.aa[program][hat]['capability'] is not None?
> #Clean the duplicates of path in other profile
> deleted += delete_path_duplicates(self.profile.aa[program][hat], self.other.aa[program][hat], 'allow', self.same_file)
> @@ -108,17 +108,6 @@
>
> return len(deleted)
>
> -def delete_cap_duplicates(profilecaps, profilecaps_other, same_profile=True):
> - deleted = []
> - if profilecaps and profilecaps_other and not same_profile:
> - for capname in profilecaps.keys():
> - if profilecaps_other[capname].get('set', False):
> - deleted.append(capname)
> - for capname in deleted:
> - profilecaps_other.pop(capname)
> -
> - return len(deleted)
> -
> def delete_net_duplicates(netrules, netrules_other, same_profile=True):
> deleted = 0
> hasher_obj = apparmor.aa.hasher()
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
--
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141121/f74a0624/attachment.pgp>
More information about the AppArmor
mailing list