Let set_profile_flags() change the flags for all hats

It did this in the old 2.8 code, but didn't in 2.9.x (first there was a
broken hat regex, then I commented out the hat handling to avoid
breakage caused by the broken regex).

This patch makes sure the hat flags get set when setting the flags for
the main profile.

Also change RE_PROFILE_HAT_DEF to use more named matches
(leadingwhitespace and hat_keyword). Luckily all code that uses the
regex uses named matches already, which means adding another (...) pair
doesn't hurt.

Finally adjust the tests:
- change _test_set_flags to accept another optional parameter
  expected_more_rules (used to specify the expected hat definition)
- add tests for hats (with '^foobar' and 'hat foobar' syntax)
- add tests for child profiles, one of them commented out (see below)


Remaining known issues (also added as TODO notes):

- The hat and child profile flags are *overwritten* with the flags used
  for the main profile. (That's well-known behaviour from 2.8 :-/ but we
  have more flags now, which makes this more annoying.)
  The correct behaviour would be to add or remove the specified flag,
  while keeping other flags unchanged.

- Child profiles are not handled/changed if you specify the 'program'
  parameter. This means:
  - 'aa-complain smbldap-useradd' or 'aa-complain /usr/sbin/smbldap-useradd'
    _will not_ change the flags for the nscd child profile
  - 'aa-complain /etc/apparmor.d/usr.sbin.smbldap-useradd' _will_ change
    the flags for the nscd child profile (and any other profile and
    child profile in that file)


Even with those remaining issues (which need bigger changes in
set_profile_flags() and maybe also in the whole flags handling), the
patch improves things and fixes the regression from the 2.8 code.



Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9

Bug: https://launchpad.net/bugs/1501913

utils/apparmor/aa.py

@@ -641,8 +641,9 @@ def set_profile_flags(prof_filename, program, newflags):
     """Reads the old profile file and updates the flags accordingly"""
     # TODO: count the number of matching lines (separated by profile and hat?) and return it
     #       so that code calling this function can make sure to only report success if there was a match
-    # TODO: use RE_PROFILE_HAT_DEF for matching the hat (regex_hat_flag is totally broken!)
-    #regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)\s*(#.*)?$')
+    # TODO: existing (unrelated) flags of hats and child profiles are overwritten - ideally, we should
+    #       keep them and only add or remove a given flag
+    # TODO: change child profile flags even if program is specified
 
     found = False
 
@@ -669,14 +670,19 @@ def set_profile_flags(prof_filename, program, newflags):
                         }
                         line = write_header(header_data, len(space)/2, profile, False, True)
                         line = '%s\n' % line[0]
-                #else:
-                #    match = regex_hat_flag.search(line)
-                #    if match:
-                #        hat, flags = match.groups()[:2]
-                #        if newflags:
-                #            line = '%s flags=(%s) {%s\n' % (hat, newflags, comment)
-                #        else:
-                #            line = '%s {%s\n' % (hat, comment)
+                elif RE_PROFILE_HAT_DEF.search(line):
+                    matches = RE_PROFILE_HAT_DEF.search(line)
+                    space = matches.group('leadingspace') or ''
+                    hat_keyword = matches.group('hat_keyword')
+                    hat = matches.group('hat')
+                    comment = matches.group('comment') or ''
+                    if comment:
+                        comment = ' %s' % comment
+
+                    if newflags:
+                        line = '%s%s%s flags=(%s) {%s\n' % (space, hat_keyword, hat, newflags, comment)
+                    else:
+                        line = '%s%s%s {%s\n' % (space, hat_keyword, hat, comment)
                 f_out.write(line)
     os.rename(temp_file.name, prof_filename)
 

utils/apparmor/regex.py

@@ -46,7 +46,7 @@ RE_PROFILE_NETWORK      = re.compile(RE_AUDIT_DENY + 'network(.*)' + RE_EOL)
 RE_NETWORK_FAMILY_TYPE = re.compile('\s+(\S+)\s+(\S+)\s*,$')
 RE_NETWORK_FAMILY = re.compile('\s+(\S+)\s*,$')
 RE_PROFILE_CHANGE_HAT   = re.compile('^\s*\^(\"??.+?\"??)' + RE_COMMA_EOL)
-RE_PROFILE_HAT_DEF      = re.compile('^\s*(\^|hat\s+)(?P<hat>\"??.+?\"??)\s+((flags=)?\((?P<flags>.+)\)\s+)*\{' + RE_EOL)
+RE_PROFILE_HAT_DEF      = re.compile('^(?P<leadingspace>\s*)(?P<hat_keyword>\^|hat\s+)(?P<hat>\"??.+?\"??)\s+((flags=)?\((?P<flags>.+)\)\s+)*\{' + RE_EOL)
 RE_PROFILE_DBUS         = re.compile(RE_AUDIT_DENY + '(dbus\s*,|dbus\s+[^#]*\s*,)' + RE_EOL)
 RE_PROFILE_MOUNT        = re.compile(RE_AUDIT_DENY + '((mount|remount|umount|unmount)(\s+[^#]*)?\s*,)' + RE_EOL)
 RE_PROFILE_SIGNAL       = re.compile(RE_AUDIT_DENY + '(signal\s*,|signal\s+[^#]*\s*,)' + RE_EOL)

utils/test/test-aa.py

@@ -106,7 +106,8 @@ class AaTest_get_profile_flags(AaTestWithTempdir):
             self._test_get_flags('/no-such-profile flags=(complain)', 'complain')
 
 class AaTest_set_profile_flags(AaTestWithTempdir):
-    def _test_set_flags(self, profile, old_flags, new_flags, whitespace='', comment='', more_rules='',
+    def _test_set_flags(self, profile, old_flags, new_flags, whitespace='', comment='',
+                        more_rules='', expected_more_rules='@-@-@',
                         expected_flags='@-@-@', check_new_flags=True, profile_name='/foo'):
         if old_flags:
             old_flags = ' %s' % old_flags
@@ -119,13 +120,16 @@ class AaTest_set_profile_flags(AaTestWithTempdir):
         else:
             expected_flags = ''
 
+        if expected_more_rules == '@-@-@':
+            expected_more_rules = more_rules
+
         if comment:
             comment = ' %s' % comment
 
         dummy_profile_content = '  #include <abstractions/base>\n  capability chown,\n  /bar r,'
         prof_template = '%s%s%s {%s\n%s\n%s\n}\n'
-        old_prof = prof_template % (whitespace, profile, old_flags,      comment, more_rules, dummy_profile_content)
-        new_prof = prof_template % (whitespace, profile, expected_flags, comment, more_rules, dummy_profile_content)
+        old_prof = prof_template % (whitespace, profile, old_flags,      comment, more_rules,          dummy_profile_content)
+        new_prof = prof_template % (whitespace, profile, expected_flags, comment, expected_more_rules, dummy_profile_content)
 
         self.file = write_file(self.tmpdir, 'profile', old_prof)
         set_profile_flags(self.file, profile_name, new_flags)
@@ -184,11 +188,46 @@ class AaTest_set_profile_flags(AaTestWithTempdir):
     def test_set_flags_12(self):
         self._test_set_flags('profile xy "/foo bar"', 'flags=(complain)', 'audit', profile_name='xy')
 
+    # test handling of hat flags
+    def test_set_flags_with_hat_01(self):
+        self._test_set_flags('/foo', 'flags=(complain)', 'audit',
+            more_rules='\n  ^foobar {\n}\n',
+            expected_more_rules='\n  ^foobar flags=(audit) {\n}\n'
+        )
 
-    # XXX regex_hat_flag in set_profile_flags() is totally broken - it matches for '   ' and '  X ', but doesn't match for ' ^foo {'
-    # oh, it matches on a line like 'dbus' and changes it to 'dbus flags=(...)' if there's no leading whitespace (and no comma)
-    #def test_set_flags_hat_01(self):
-    #    self._test_set_flags('  ^hat', '', 'audit')
+    def test_set_flags_with_hat_02(self):
+        self._test_set_flags('/foo', 'flags=(complain)', 'audit',
+            profile_name=None,
+            more_rules='\n  ^foobar {\n}\n',
+            expected_more_rules='\n  ^foobar flags=(audit) {\n}\n'
+        )
+
+    def test_set_flags_with_hat_03(self):
+        self._test_set_flags('/foo', 'flags=(complain)', 'audit',
+            more_rules='\n^foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost!
+            expected_more_rules='\n^foobar flags=(audit) { # comment\n}\n'
+        )
+
+    def test_set_flags_with_hat_04(self):
+        self._test_set_flags('/foo', '', 'audit',
+            more_rules='\n  hat foobar (attach_disconnected) { # comment\n}\n', # XXX attach_disconnected will be lost!
+            expected_more_rules='\n  hat foobar flags=(audit) { # comment\n}\n'
+        )
+
+    # test handling of child profiles
+    def test_set_flags_with_child_01(self):
+        self._test_set_flags('/foo', 'flags=(complain)', 'audit',
+            profile_name=None,
+            more_rules='\n  profile /bin/bar {\n}\n',
+            expected_more_rules='\n  profile /bin/bar flags=(audit) {\n}\n'
+        )
+
+    #def test_set_flags_with_child_02(self):
+        # XXX child profile flags aren't changed if profile parameter is not None
+        #self._test_set_flags('/foo', 'flags=(complain)', 'audit',
+        #    more_rules='\n  profile /bin/bar {\n}\n',
+        #    expected_more_rules='\n  profile /bin/bar flags=(audit) {\n}\n'
+        #)
 
 
     def test_set_flags_invalid_01(self):