From f26b035e90a9bcc9bd2c99ffabfe31bcd8c0fe48 Mon Sep 17 00:00:00 2001 From: Christian Boltz Date: Thu, 28 May 2015 22:14:37 +0200 Subject: [PATCH] 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 for trunk and 2.9 --- utils/apparmor/aa.py | 26 ++++++++++++-------- utils/apparmor/regex.py | 2 +- utils/test/test-aa.py | 53 +++++++++++++++++++++++++++++++++++------ 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 36614ce8b..a774e2f43 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -652,8 +652,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 @@ -680,14 +681,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) diff --git a/utils/apparmor/regex.py b/utils/apparmor/regex.py index d164b3664..2bf2ff278 100644 --- a/utils/apparmor/regex.py +++ b/utils/apparmor/regex.py @@ -44,7 +44,7 @@ RE_PROFILE_BARE_FILE_ENTRY = re.compile(RE_AUDIT_DENY + RE_OWNER + 'file' + RE_C RE_PROFILE_PATH_ENTRY = re.compile(RE_AUDIT_DENY + RE_OWNER + '(file\s+)?([\"@/].*?)\s+(\S+)(\s+->\s*(.*?))?' + RE_COMMA_EOL) RE_PROFILE_NETWORK = re.compile(RE_AUDIT_DENY + 'network(?P
\s+.*)?' + RE_COMMA_EOL) RE_PROFILE_CHANGE_HAT = re.compile('^\s*\^(\"??.+?\"??)' + RE_COMMA_EOL) -RE_PROFILE_HAT_DEF = re.compile('^\s*(\^|hat\s+)(?P\"??.+?\"??)\s+((flags=)?\((?P.+)\)\s+)*\{' + RE_EOL) +RE_PROFILE_HAT_DEF = re.compile('^(?P\s*)(?P\^|hat\s+)(?P\"??.+?\"??)\s+((flags=)?\((?P.+)\)\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) diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index 9284806ac..f2234f9c9 100644 --- a/utils/test/test-aa.py +++ b/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 \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):