Hello,

this patch changes aa.py to use FileRule and FileRuleset for parsing and 
saving profiles.

In detail, this means:
- add 'file' to the list of rule classes to enable it at various places
- store file rules in aa[profile][hat]['file'] (not 'path' as before)
  to be consistent with the FileRule name
- drop the no longer needed delete_path_duplicates() - this is now
  handled by FileRuleset like in all other rule classes.
  (same change in cleanprofile.py)
- replace usage of RE_PROFILE_BARE_FILE_ENTRY and RE_PROFILE_PATH_ENTRY
  with FileRule.match()
- drop write_path_rules() and write_paths() and replace them with the
  new write_file() function.
- adjust several code sections to use write_file and 'file' instead of
  'path'

FileRule doesn't drop optional keywords ('allow' and 'file'), therefore
adjust cleanprof_test.out to the changed behaviour. (If someone insists
on dropping optional keywords in aa-cleanprof, that's something for a
future patch.)

Also adjust the list of known failures in test-parser-simple-tests.py -
switching to FileRule avoids several test failures (and introduces a few
new ones ;-)



IMPORTANT:

This patch introduces a "brain split" which means
- parsing and writing the profile and aa-cleanprof use the new location
  (aa[profile][hat]['file'])
- aa-logprof and aa-genprof still save data to the old location
  (aa[profile][hat]['allow']['path']) and probably ask superfluous
  questions because there are no rules existing in the old location

TL;DR: don't try aa-logprof or aa-genprof with only this patch applied.

I know this isn't ideal, but still better than an even bigger and
totally unreadable patch ;-)


[ 14-switch-to-FileRule.diff ]

=== modified file ./utils/apparmor/aa.py
--- utils/apparmor/aa.py        2016-02-04 17:51:59.552181829 +0100
+++ utils/apparmor/aa.py        2016-02-04 20:19:14.734161664 +0100
@@ -36,7 +36,7 @@
 
 import apparmor.ui as aaui
 
-from apparmor.aamode import (str_to_mode, mode_to_str, split_mode,
+from apparmor.aamode import (str_to_mode, mode_to_str,
                              mode_to_str_user, mode_contains, AA_OTHER,
                              flatten_mode, owner_flatten_mode)
 
@@ -44,7 +44,6 @@
                             RE_PROFILE_ALIAS,
                             RE_PROFILE_BOOLEAN, RE_PROFILE_VARIABLE, 
RE_PROFILE_CONDITIONAL,
                             RE_PROFILE_CONDITIONAL_VARIABLE, 
RE_PROFILE_CONDITIONAL_BOOLEAN,
-                            RE_PROFILE_BARE_FILE_ENTRY, RE_PROFILE_PATH_ENTRY,
                             RE_PROFILE_CHANGE_HAT,
                             RE_PROFILE_HAT_DEF, RE_PROFILE_MOUNT,
                             RE_PROFILE_PIVOT_ROOT,
@@ -56,13 +55,14 @@
 from apparmor.rule.capability import CapabilityRuleset, CapabilityRule
 from apparmor.rule.change_profile import ChangeProfileRuleset, 
ChangeProfileRule
 from apparmor.rule.dbus       import DbusRuleset,       DbusRule
+from apparmor.rule.file       import FileRuleset,       FileRule
 from apparmor.rule.network    import NetworkRuleset,    NetworkRule
 from apparmor.rule.ptrace     import PtraceRuleset,    PtraceRule
 from apparmor.rule.rlimit     import RlimitRuleset,    RlimitRule
 from apparmor.rule.signal     import SignalRuleset,    SignalRule
-from apparmor.rule import parse_modifiers, quote_if_needed
+from apparmor.rule import quote_if_needed
 
-ruletypes = ['capability', 'change_profile', 'dbus', 'network', 'ptrace', 
'rlimit', 'signal']
+ruletypes = ['capability', 'change_profile', 'dbus', 'file', 'network', 
'ptrace', 'rlimit', 'signal']
 
 from apparmor.yasti import SendDataToYast, GetDataFromYast, shutdown_yast
 
@@ -457,6 +464,7 @@
 
     profile['capability']       = CapabilityRuleset()
     profile['dbus']             = DbusRuleset()
+    profile['file']             = FileRuleset()
     profile['change_profile']   = ChangeProfileRuleset()
     profile['network']          = NetworkRuleset()
     profile['ptrace']           = PtraceRuleset()
@@ -2016,22 +2014,6 @@
             newpath = re.sub('/[^/]+(\.[^/]+)$', '/*' + match.groups()[0], 
newpath)
     return newpath
 
-def delete_path_duplicates(profile, incname, allow):
-    deleted = []
-    for entry in profile[allow]['path'].keys():
-        if entry == '#include <%s>' % incname:
-            continue
-        # XXX Make this code smart enough to know that bare file rules
-        #     makes some path rules unnecessary. For example, "/dev/random r,"
-        #     would no longer be needed if "file," was present.
-        cm, am, m = match_include_to_path(incname, allow, entry)
-        if cm and mode_contains(cm, profile[allow]['path'][entry]['mode']) and 
mode_contains(am, profile[allow]['path'][entry]['audit']):
-            deleted.append(entry)
-
-    for entry in deleted:
-        profile[allow]['path'].pop(entry)
-    return len(deleted)
-
 def delete_duplicates(profile, incname):
     deleted = 0
     # Allow rules covered by denied rules shouldn't be deleted
@@ -2041,16 +2023,10 @@
         for rule_type in ruletypes:
             deleted += 
profile[rule_type].delete_duplicates(include[incname][incname][rule_type])
 
-        deleted += delete_path_duplicates(profile, incname, 'allow')
-        deleted += delete_path_duplicates(profile, incname, 'deny')
-
     elif filelist.get(incname, False):
         for rule_type in ruletypes:
             deleted += 
profile[rule_type].delete_duplicates(filelist[incname][incname][rule_type])
 
-        deleted += delete_path_duplicates(profile, incname, 'allow')
-        deleted += delete_path_duplicates(profile, incname, 'deny')
-
     return deleted
 
 def match_includes(profile, rule_type, rule_obj):
@@ -2735,86 +2734,6 @@
             # Conditional Boolean defined
             pass
 
-        elif RE_PROFILE_BARE_FILE_ENTRY.search(line):
-            matches = RE_PROFILE_BARE_FILE_ENTRY.search(line)
-
-            if not profile:
-                raise AppArmorException(_('Syntax Error: Unexpected bare file 
rule found in file: %(file)s line: %(line)s') % { 'file': file, 'line': lineno 
+ 1 })
-
-            audit, deny, allow_keyword, comment = parse_modifiers(matches)
-            # TODO: honor allow_keyword and comment
-            if deny:
-                allow = 'deny'
-            else:
-                allow = 'allow'
-
-            mode = apparmor.aamode.AA_BARE_FILE_MODE
-            if not matches.group('owner'):
-                mode |= AA_OTHER(apparmor.aamode.AA_BARE_FILE_MODE)
-
-            path_rule = profile_data[profile][hat][allow]['path'][ALL]
-            path_rule['mode'] = mode
-            path_rule['audit'] = set()
-            if audit:
-                path_rule['audit'] = mode
-            path_rule['file_prefix'] = True
-
-        elif RE_PROFILE_PATH_ENTRY.search(line):
-            matches = RE_PROFILE_PATH_ENTRY.search(line).groups()
-
-            if not profile:
-                raise AppArmorException(_('Syntax Error: Unexpected path entry 
found in file: %(file)s line: %(line)s') % { 'file': file, 'line': lineno + 1 })
-
-            audit = False
-            if matches[0]:
-                audit = True
-
-            allow = 'allow'
-            if matches[1] and matches[1].strip() == 'deny':
-                allow = 'deny'
-
-            user = False
-            if matches[2]:
-                user = True
-
-            file_prefix = False
-            if matches[3]:
-                file_prefix = True
-
-            path = strip_quotes(matches[4].strip())
-            mode = matches[5]
-            nt_name = matches[7]
-            if nt_name:
-                nt_name = nt_name.strip()
-
-            p_re = convert_regexp(path)
-            try:
-                re.compile(p_re)
-            except:
-                raise AppArmorException(_('Syntax Error: Invalid Regex 
%(path)s in file: %(file)s line: %(line)s') % { 'path': path, 'file': file, 
'line': lineno + 1 })
-
-            if not validate_profile_mode(mode, allow, nt_name):
-                raise AppArmorException(_('Invalid mode %(mode)s in file: 
%(file)s line: %(line)s') % {'mode': mode, 'file': file, 'line': lineno + 1 })
-
-            tmpmode = set()
-            if user:
-                tmpmode = str_to_mode('%s::' % mode)
-            else:
-                tmpmode = str_to_mode(mode)
-
-            profile_data[profile][hat][allow]['path'][path]['mode'] = 
profile_data[profile][hat][allow]['path'][path].get('mode', set()) | tmpmode
-
-            if file_prefix:
-                profile_data[profile][hat][allow]['path'][path]['file_prefix'] 
= True
-
-            if nt_name:
-                profile_data[profile][hat][allow]['path'][path]['to'] = nt_name
-
-            if audit:
-                profile_data[profile][hat][allow]['path'][path]['audit'] = 
profile_data[profile][hat][allow]['path'][path].get('audit', set()) | tmpmode
-            else:
-                profile_data[profile][hat][allow]['path'][path]['audit'] = 
set()
-
         elif re_match_include(line):
             # Include files
             include_name = re_match_include(line)
@@ -2985,6 +2904,13 @@
                 else:
                     initial_comment = initial_comment + line + '\n'
 
+        elif FileRule.match(line):
+            # leading permissions could look like a keyword, therefore handle 
file rules after everything else
+            if not profile:
+                raise AppArmorException(_('Syntax Error: Unexpected path entry 
found in file: %(file)s line: %(line)s') % { 'file': file, 'line': lineno + 1 })
+
+            profile_data[profile][hat]['file'].add(FileRule.parse(line))
+
         elif not RE_RULE_HAS_COMMA.search(line):
             # Bah, line continues on to the next line
             if RE_HAS_COMMENT_SPLIT.search(line):
@@ -3282,76 +3208,10 @@
 
     return data
 
-def write_path_rules(prof_data, depth, allow):
-    pre = '  ' * depth
+def write_file(prof_data, depth):
     data = []
-    allowstr = set_allow_str(allow)
-
-    if prof_data[allow].get('path', False):
-        for path in sorted(prof_data[allow]['path'].keys()):
-            filestr = ''
-            if prof_data[allow]['path'][path].get('file_prefix', False):
-                filestr = 'file'
-            mode = prof_data[allow]['path'][path]['mode']
-            audit = prof_data[allow]['path'][path]['audit']
-            tail = ''
-            if prof_data[allow]['path'][path].get('to', False):
-                tail = ' -> %s' % prof_data[allow]['path'][path]['to']
-            user, other = split_mode(mode)
-            user_audit, other_audit = split_mode(audit)
-
-            while user or other:
-                ownerstr = ''
-                tmpmode = 0
-                tmpaudit = False
-                if user - other:
-                    # if no other mode set
-                    ownerstr = 'owner '
-                    tmpmode = user - other
-                    tmpaudit = user_audit
-                    user = user - tmpmode
-                else:
-                    if user_audit - other_audit & user:
-                        ownerstr = 'owner '
-                        tmpaudit = user_audit - other_audit & user
-                        tmpmode = user & tmpaudit
-                        user = user - tmpmode
-                    else:
-                        ownerstr = ''
-                        tmpmode = user | other
-                        tmpaudit = user_audit | other_audit
-                        user = user - tmpmode
-                        other = other - tmpmode
-
-                if path == ALL:
-                    path = ''
-
-                if tmpmode & tmpaudit:
-                    modestr = mode_to_str(tmpmode & tmpaudit)
-                    if modestr:
-                        modestr = ' ' + modestr
-                    path = quote_if_needed(path)
-                    if filestr and path:
-                        filestr += ' '
-                    data.append('%saudit %s%s%s%s%s%s,' % (pre, allowstr, 
ownerstr, filestr, path, modestr, tail))
-                    tmpmode = tmpmode - tmpaudit
-
-                if tmpmode:
-                    modestr = mode_to_str(tmpmode)
-                    if modestr:
-                        modestr = ' ' + modestr
-                    path = quote_if_needed(path)
-                    if filestr and path:
-                        filestr += ' '
-                    data.append('%s%s%s%s%s%s%s,' % (pre, allowstr, ownerstr, 
filestr, path, modestr, tail))
-
-        data.append('')
-    return data
-
-def write_paths(prof_data, depth):
-    data = write_path_rules(prof_data, depth, 'deny')
-    data += write_path_rules(prof_data, depth, 'allow')
-
+    if prof_data.get('file', False):
+        data = prof_data['file'].get_clean(depth)
     return data
 
 def write_rules(prof_data, depth):
@@ -3368,7 +3228,7 @@
     data += write_pivot_root(prof_data, depth)
     data += write_unix(prof_data, depth)
     data += write_links(prof_data, depth)
-    data += write_paths(prof_data, depth)
+    data += write_file(prof_data, depth)
     data += write_change_profile(prof_data, depth)
 
     return data
@@ -3530,7 +3390,7 @@
                          'pivot_root': write_pivot_root,
                          'unix': write_unix,
                          'link': write_links,
-                         'path': write_paths,
+                         'file': write_file,
                          'change_profile': write_change_profile,
                          }
         default_write_order = [ 'alias',
@@ -3546,7 +3406,7 @@
                                 'pivot_root',
                                 'unix',
                                 'link',
-                                'path',
+                                'file',
                                 'change_profile',
                               ]
         # prof_correct = True  # XXX correct?
@@ -3563,7 +3423,7 @@
                     'pivot_root': True, # not handled otherwise yet
                     'unix': True, # not handled otherwise yet
                     'link': False,
-                    'path': False,
+                    'file': False,
                     'change_profile': False,
                     'include_local_started': False, # unused
                     }
@@ -3813,78 +3673,6 @@
                     #To-Do
                     pass
 
-            elif RE_PROFILE_BARE_FILE_ENTRY.search(line):
-                matches = RE_PROFILE_BARE_FILE_ENTRY.search(line).groups()
-
-                allow = 'allow'
-                if matches[1] and matches[1].strip() == 'deny':
-                    allow = 'deny'
-
-                mode = apparmor.aamode.AA_BARE_FILE_MODE
-                if not matches[2]:
-                    mode |= AA_OTHER(apparmor.aamode.AA_BARE_FILE_MODE)
-
-                audit = set()
-                if matches[0]:
-                    audit = mode
-
-                path_rule = write_prof_data[hat][allow]['path'][ALL]
-                if path_rule.get('mode', set()) & mode and \
-                   (not audit or path_rule.get('audit', set()) & audit) and \
-                   path_rule.get('file_prefix', set()):
-                    if not segments['path'] and True in segments.values():
-                        data += write_prior_segments(write_prof_data[name], 
segments, line)
-                    segments['path'] = True
-                    write_prof_data[hat][allow]['path'].pop(ALL)
-                    data.append(line)
-
-            elif RE_PROFILE_PATH_ENTRY.search(line):
-                matches = RE_PROFILE_PATH_ENTRY.search(line).groups()
-                audit = False
-                if matches[0]:
-                    audit = True
-                allow = 'allow'
-                if matches[1] and matches[1].split() == 'deny':
-                    allow = 'deny'
-
-                user = False
-                if matches[2]:
-                    user = True
-
-                path = strip_quotes(matches[4].strip())
-                mode = matches[5]
-                nt_name = matches[7]
-                if nt_name:
-                    nt_name = nt_name.strip()
-
-                tmpmode = set()
-                if user:
-                    tmpmode = str_to_mode('%s::' % mode)
-                else:
-                    tmpmode = str_to_mode(mode)
-
-                if not write_prof_data[hat][allow]['path'].get(path):
-                    correct = False
-                else:
-                    if not 
write_prof_data[hat][allow]['path'][path].get('mode', set()) & tmpmode:
-                        correct = False
-
-                    if nt_name and not 
write_prof_data[hat][allow]['path'][path].get('to', False) == nt_name:
-                        correct = False
-
-                    if audit and not 
write_prof_data[hat][allow]['path'][path].get('audit', set()) & tmpmode:
-                        correct = False
-
-                if correct:
-                    if not segments['path'] and True in segments.values():
-                        data += write_prior_segments(write_prof_data[name], 
segments, line)
-                    segments['path'] = True
-                    write_prof_data[hat][allow]['path'].pop(path)
-                    data.append(line)
-                else:
-                    #To-Do
-                    pass
-
             elif re_match_include(line):
                 include_name = re_match_include(line)
                 if profile:
@@ -3928,6 +3716,20 @@
                 else:
                     #To-Do
                     pass
+            elif FileRule.match(line):
+                # leading permissions could look like a keyword, therefore 
handle file rules after everything else
+                file_obj = FileRule.parse(line)
+
+                if write_prof_data[hat]['file'].is_covered(file_obj, True, 
True):
+                    if not segments['file'] and True in segments.values():
+                        data += write_prior_segments(write_prof_data[name], 
segments, line)
+                    segments['file'] = True
+                    write_prof_data[hat]['file'].delete(file_obj)
+                    data.append(line)
+                else:
+                    #To-Do
+                    pass
+
             else:
                 if correct:
                     data.append(line)
=== modified file ./utils/apparmor/cleanprofile.py
--- utils/apparmor/cleanprofile.py      2015-12-03 22:04:36.782275414 +0100
+++ utils/apparmor/cleanprofile.py      2016-02-04 20:15:13.227661220 +0100
@@ -13,7 +13,6 @@
 #
 # ----------------------------------------------------------------------
 import apparmor.aa as apparmor
-from apparmor.regex import re_match_include
 
 class Prof(object):
     def __init__(self, filename):
@@ -71,39 +71,4 @@
                 else:
                     deleted += 
self.other.aa[program][hat][ruletype].delete_duplicates(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)
-            deleted += delete_path_duplicates(self.profile.aa[program][hat], 
self.other.aa[program][hat], 'deny', self.same_file)
-
         return deleted
-
-def delete_path_duplicates(profile, profile_other, allow, same_profile=True):
-    deleted = []
-    # Check if any individual rule makes any rule superfluous
-    for rule in profile[allow]['path'].keys():
-        for entry in profile_other[allow]['path'].keys():
-            if rule == entry:
-                # Check the modes
-                cm = profile[allow]['path'][rule]['mode']
-                am = profile[allow]['path'][rule]['audit']
-                # If modes of rule are a superset of rules implied by entry we 
can safely remove it
-                if apparmor.mode_contains(cm, 
profile_other[allow]['path'][entry]['mode']) and apparmor.mode_contains(am, 
profile_other[allow]['path'][entry]['audit']):
-                    if not same_profile:
-                        deleted.append(entry)
-                continue
-            if re_match_include(rule) or re_match_include(entry):
-                continue
-            # Check if the rule implies entry
-            if apparmor.matchliteral(rule, entry):
-                # Check the modes
-                cm = profile[allow]['path'][rule]['mode']
-                am = profile[allow]['path'][rule]['audit']
-                # If modes of rule are a superset of rules implied by entry we 
can safely remove it
-                if apparmor.mode_contains(cm, 
profile_other[allow]['path'][entry]['mode']) and apparmor.mode_contains(am, 
profile_other[allow]['path'][entry]['audit']):
-                    deleted.append(entry)
-
-    for entry in deleted:
-        profile_other[allow]['path'].pop(entry)
-
-    return len(deleted)
-
=== modified file ./utils/test/cleanprof_test.out
--- utils/test/cleanprof_test.out       2016-02-01 21:31:56.419302952 +0100
+++ utils/test/cleanprof_test.out       2016-02-01 18:15:00.895533604 +0100
@@ -20,8 +20,8 @@
 
   unix (receive) type=dgram,
 
-  /home/*/** r,
-  /home/foo/** w,
+  allow /home/*/** r,
+  allow /home/foo/** w,
 
   change_profile,
 
@@ -34,7 +34,7 @@
   }
 }
 /usr/bin/other/cleanprof/test/profile {
-  /home/*/** rw,
-  /home/foo/bar r,
+  allow /home/*/** rw,
+  allow /home/foo/bar r,
 
 }
=== modified file ./utils/test/test-parser-simple-tests.py
--- utils/test/test-parser-simple-tests.py      2016-02-01 21:31:56.415302976 
+0100
+++ utils/test/test-parser-simple-tests.py      2016-01-26 22:22:31.505637218 
+0100
@@ -53,17 +53,9 @@
     'dbus/bad_regex_04.sd',
     'dbus/bad_regex_05.sd',
     'dbus/bad_regex_06.sd',
-    'file/bad_append_1.sd',
-    'file/bad_append_2.sd',
-    'file/bad_embedded_spaces_1.sd',
+    'file/bad_re_brace_1.sd',
     'file/bad_re_brace_2.sd',
     'file/bad_re_brace_3.sd',
-    'file/file/bad_append_1.sd',
-    'file/file/bad_embedded_spaces_1.sd',
-    'file/file/owner/bad_3.sd',
-    'file/file/owner/bad_5.sd',
-    'file/owner/bad_3.sd',
-    'file/owner/bad_5.sd',
     'mount/bad_opt_10.sd',
     'mount/bad_opt_11.sd',
     'mount/bad_opt_12.sd',
@@ -188,17 +180,10 @@
     'xtrans/simple_bad_conflicting_x_11.sd',
     'xtrans/simple_bad_conflicting_x_12.sd',
     'xtrans/simple_bad_conflicting_x_13.sd',
-    'xtrans/simple_bad_conflicting_x_14.sd',
-    'xtrans/simple_bad_conflicting_x_15.sd',
-    'xtrans/simple_bad_conflicting_x_1.sd',
     'xtrans/simple_bad_conflicting_x_2.sd',
-    'xtrans/simple_bad_conflicting_x_3.sd',
     'xtrans/simple_bad_conflicting_x_4.sd',
-    'xtrans/simple_bad_conflicting_x_5.sd',
     'xtrans/simple_bad_conflicting_x_6.sd',
-    'xtrans/simple_bad_conflicting_x_7.sd',
     'xtrans/simple_bad_conflicting_x_8.sd',
-    'xtrans/simple_bad_conflicting_x_9.sd',
     'xtrans/x-conflict.sd',
 ]
 
@@ -211,11 +196,9 @@
     'file/ok_other_2.sd',
     'file/ok_other_3.sd',
 
-    # permissions before path
+    # 'unsafe' keyword
     'file/file/front_perms_ok_1.sd',
     'file/front_perms_ok_1.sd',
-    'profile/local/local_named_profile_ok1.sd',
-    'profile/local/local_profile_ok1.sd',
     'xtrans/simple_ok_cx_1.sd',
 
     # permissions before path and owner / audit {...} blocks
@@ -267,6 +250,9 @@
     'file/ok_5.sd',  # Invalid mode UX
     'file/ok_2.sd',  # Invalid mode RWM
     'file/ok_4.sd',  # Invalid mode iX
+    'file/ok_embedded_spaces_4.sd',  # \-escaped space
+    'file/file/ok_embedded_spaces_4.sd',  # \-escaped space
+    'file/ok_quoted_4.sd',  # quoted string including \"
     'xtrans/simple_ok_pix_1.sd',  # Invalid mode pIx
     'xtrans/simple_ok_pux_1.sd',  # Invalid mode rPux
 




Regards,

Christian Boltz
-- 
> Einfach mal nach Maengelexemplaren suchen (z.B. bei Amazon-
> Marketplace). Habe mir dort gerade den Latex-Begleiter
ich wusste garnicht das Amazon auch eine SM-Abteilung hat:
Latex-Begleiter und das auch noch zu kaufen ... ;-)
[> Heinz W. Pahlke und Rolf Masfelder in suse-linux]

Attachment: signature.asc
Description: This is a digitally signed message part.

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to