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 > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
