Hello,

Am Dienstag, 25. November 2014 schrieb Steve Beattie:
> On Sun, Nov 23, 2014 at 01:01:31AM +0100, Christian Boltz wrote:
> > Am Freitag, 21. November 2014 schrieb Steve Beattie:
> > > On Sat, Nov 15, 2014 at 11:46:41PM +0100, Christian Boltz wrote:

> So while testing with the v3 versions of the patches applied, the
> delete_all_rules() invocation is getting called on non-capability
> segments (or else the capability key is getting a hasher wrongly
> assigned to it); e.g.:

> File "/home/ubuntu/bzr/apparmor/utils/apparmor/aa.py", line 4198, in
> serialize_profile_from_old_profile
> write_prof_data[name][segs].delete_all_rules()
>   AttributeError: 'collections.defaultdict' object has no attribute
> 'delete_all_rules'
> 
> That's from running aa-logprof with added child execs and an added
> file rule.

Nice catch, fixed.

> I *really* don't get how serialize_profile_from_old_profile() "works",
> so I'm not quite sure what's going wrong or why.

Yes, serialize_profile_from_old_profile() is indeed not easy to 
understand - it also took me some time. We'll have to rewrite that 
function one day. ;-)


The updated patch (v4) is attached.

Changes since v3:
- document keys used in aa[profile][hat]
- fix crash in write_capabilities()
- improve resetting capability ruleset object in write_prior_segments()
  and serialize_profile_from_old_profile() (includes correct handling 
  for include rules "for free")


The usual line statistics:
v4-1-add-base-and-capability-rule-class.diff - 371 lines added, 0 removed
v4-2-add-capability-rule-test.diff - 806 lines added, 0 removed
v4-3-use-capability-rule-class.diff - 68 lines added, 112 removed


Regards,

Christian Boltz
-- 
["Glatzen"] Man verkloppt keine behinderte Menschen.
Neulich in der U-Bahn saß eine Omi neben so einem armen Menschen und sah
ihn sehr nachdenklich an, strich mit der Hand über die Glatze und sagt:
Du armer Mensch, erst die schwere Chemotherapie und dann muß du auch
noch diese schweren orthopädischen Schuhe tragen".
[Rolf-Hubert Pobloth in suse-linux]
=== modified file 'utils/apparmor/aa.py'
--- utils/apparmor/aa.py	2014-11-27 17:34:45 +0000
+++ utils/apparmor/aa.py	2014-11-27 19:36:28 +0000
@@ -52,6 +52,8 @@
 
 import apparmor.rules as aarules
 
+from apparmor.rule.capability import CapabilityRuleset, CapabilityRule
+
 from apparmor.yasti import SendDataToYast, GetDataFromYast, shutdown_yast
 
 # setup module translations
@@ -89,13 +91,19 @@
 # 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
 ### Were our
 t = hasher()  # dict()
 transitions = hasher()
+
+# keys used in aa[profile][hat]:
+# a) rules (as dict): alias, change_profile, include, lvar, rlimit
+# b) rules (as hasher): allow, deny
+# c) one for each rule class
+# d) other: declared, external, flags, name, profile
 aa = hasher()  # Profiles originally in sd, replace by aa
 original_aa = hasher()
 extras = hasher()  # Inactive profiles from extras
@@ -1547,13 +1555,15 @@
             for hat in hats:
                 for capability in sorted(log_dict[aamode][profile][hat]['capability'].keys()):
                     # skip if capability already in profile
-                    if profile_known_capability(aa[profile][hat], capability):
+                    capability_obj = CapabilityRule()
+                    capability_obj.set_param(capability)
+                    if is_known_rule(aa[profile][hat], 'capability', capability_obj):
                         continue
                     # Load variables? Don't think so.
                     severity = sev_db.rank('CAP_%s' % capability)
                     default_option = 1
                     options = []
-                    newincludes = match_cap_includes(aa[profile][hat], capability)
+                    newincludes = match_includes(aa[profile][hat], 'capability', capability_obj)
                     q = aaui.PromptQuestion()
 
                     if newincludes:
@@ -1626,16 +1636,20 @@
                                 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:
+                                capability_obj = CapabilityRule()
+                                capability_obj.set_param(capability, audit=audit)
+                                aa[profile][hat]['capability'].add_obj(capability_obj)
+                                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
+                            capability_obj = CapabilityRule()
+                            capability_obj.set_param(capability, audit=audit, deny=True)
+                            aa[profile][hat]['capability'].add_obj(capability_obj)
                             changed[profile] = True
 
                             aaui.UI_Info(_('Denying capability %s to profile.') % capability)
@@ -2120,20 +2134,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():
@@ -2160,9 +2160,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')
@@ -2172,9 +2170,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')
@@ -2203,9 +2199,17 @@
     return False
 
 def match_cap_includes(profile, cap):
+    # still used by aa-mergeprof
+    capability_obj = CapabilityRule()
+    capability_obj.set_param(cap)
+    return match_includes(profile, 'capability', capability_obj)
+
+def match_includes(profile, rule_type, rule_obj):
     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 CapabilityRule object instead of is_raw_covered() might be nice/faster
+        if valid_include(profile, incname) and include[incname][incname].get(rule_type, False) and include[incname][incname][rule_type].is_obj_covered(rule_obj):
             newincludes.append(incname)
 
     return newincludes
@@ -2515,7 +2519,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'].is_raw_covered("capability %s," % capability):
                         log_dict[aamode][profile][hat]['capability'][capability] = True
 
                 nd = prelog[aamode][profile][hat]['netdomain']
@@ -2703,6 +2708,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'] = CapabilityRuleset()
+
         elif RE_PROFILE_END.search(line):
             # If profile ends and we're not in one
             if not profile:
@@ -2718,21 +2727,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'] = CapabilityRuleset()
+
+            profile_data[profile][hat]['capability'].add_raw(line)
 
         elif RE_PROFILE_LINK.search(line):
             matches = RE_PROFILE_LINK.search(line).groups()
@@ -3372,29 +3374,10 @@
 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 = []
+    if prof_data.get('capability', False):
+        data = prof_data['capability'].get_clean(depth)
     return data
 
 def write_net_rules(prof_data, depth, allow):
@@ -3824,10 +3807,14 @@
                 depth = len(line) - len(line.lstrip())
                 data += write_methods[segs](prof_data, int(depth / 2))
                 segments[segs] = False
+                # delete rules from prof_data to avoid duplication (they are in data now)
                 if prof_data['allow'].get(segs, False):
                     prof_data['allow'].pop(segs)
                 if prof_data['deny'].get(segs, False):
                     prof_data['deny'].pop(segs)
+                if prof_data.get(segs, False):
+                    t = type(prof_data[segs])
+                    prof_data[segs] = t()
             return data
 
         #data.append('reading prof')
@@ -3888,16 +3875,20 @@
                 if profile:
                     depth = int(len(line) - len(line.lstrip()) / 2) + 1
 
-                    # first write sections that were modified (and remove them from write_prof_data)
+                    # first write sections that were modified
                     #for segs in write_methods.keys():
                     for segs in default_write_order:
                         if segments[segs]:
                             data += write_methods[segs](write_prof_data[name], depth)
                             segments[segs] = False
+                            # delete rules from write_prof_data to avoid duplication (they are in data now)
                             if write_prof_data[name]['allow'].get(segs, False):
                                 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):
+                                t = type(write_prof_data[name][segs])
+                                write_prof_data[name][segs] = t()
 
                     # then write everything else
                     for segs in default_write_order:
@@ -3939,34 +3930,12 @@
                     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'].is_raw_covered(line, True, True):
                     if not segments['capability'] and True in segments.values():
                         data += write_prior_segments(write_prof_data[name], segments, line)
                     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
@@ -4342,20 +4311,18 @@
 
     return 0
 
-def profile_known_capability(profile, capname):
-    if profile['deny']['capability'][capname].get('set', False):
-        return -1
-
-    if profile['allow']['capability'][capname].get('set', False):
-        return 1
+def is_known_rule(profile, rule_type, rule_obj):
+    # XXX get rid of get() checks after we have a proper function to initialize a profile
+    if profile.get(rule_type, False):
+        if profile[rule_type].is_obj_covered(rule_obj, 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 include[incname][incname].get(rule_type, False):
+            if include[incname][incname][rule_type].is_obj_covered(rule_obj, 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-21 19:08:06 +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'])
 
             #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

Reply via email to