From 39f84c3767e6fef76f1bcfe074172086b75658bb Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Tue, 3 Sep 2024 12:10:39 -0300 Subject: [PATCH 1/2] utils: fix file handling of old perms with owner When the profile already contains a "file" rule containing the owner prefix and the tool is trying to handle a new file entry, it tries to show it in the logprof header as "old mode". The issue is that when the owner rule is an implicit all files permission, then the object "FileRule" is used instead of the set of permissions. When subtracting FileRule from set() a TypeError exception is thrown. Fix this by "translating" FileRule.ALL perms to "mrwlkix". Fixes: https://gitlab.com/apparmor/apparmor/-/issues/429 Signed-off-by: Georgia Garcia --- utils/apparmor/rule/file.py | 17 +++++++++++++++-- utils/test/test-file.py | 10 ++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/utils/apparmor/rule/file.py b/utils/apparmor/rule/file.py index 267a05f8e..a0c6b6fe8 100644 --- a/utils/apparmor/rule/file.py +++ b/utils/apparmor/rule/file.py @@ -26,6 +26,7 @@ allow_exec_transitions = ('ix', 'ux', 'Ux', 'px', 'Px', 'cx', 'Cx') # 2 chars - allow_exec_fallback_transitions = ('pix', 'Pix', 'cix', 'Cix', 'pux', 'PUx', 'cux', 'CUx') # 3 chars - len relevant for split_perms() deny_exec_transitions = ('x') file_permissions = ('m', 'r', 'w', 'a', 'l', 'k', 'link', 'subset') # also defines the write order +implicit_all_permissions = ('m', 'r', 'w', 'l', 'k') class FileRule(BaseRule): @@ -356,9 +357,21 @@ class FileRule(BaseRule): old_mode = '' if self.original_perms: - original_perms_all = self._join_given_perms(self.original_perms['allow']['all'], None) + original_perms_set = {} + for who in ['all', 'owner']: + original_perms_set[who] = {} + original_perms_set[who]['perms'] = self.original_perms['allow'][who] + original_perms_set[who]['exec_perms'] = None + + if self.original_perms['allow'][who] == FileRule.ALL: + original_perms_set[who]['perms'] = set(implicit_all_permissions) + original_perms_set[who]['exec_perms'] = 'ix' + + original_perms_all = self._join_given_perms(original_perms_set['all']['perms'], + original_perms_set['all']['exec_perms']) original_perms_owner = self._join_given_perms( - self.original_perms['allow']['owner'] - self.original_perms['allow']['all'], None) # only list owner perms that are not covered by other perms + original_perms_set['owner']['perms'] - original_perms_set['all']['perms'], # only list owner perms that are not covered by other perms + original_perms_set['owner']['exec_perms']) if original_perms_all and original_perms_owner: old_mode = '%s + owner %s' % (original_perms_all, original_perms_owner) diff --git a/utils/test/test-file.py b/utils/test/test-file.py index dcf424d6a..05c496e69 100644 --- a/utils/test/test-file.py +++ b/utils/test/test-file.py @@ -866,6 +866,16 @@ class FileLogprofHeaderTest(AATest): obj.original_perms = {'allow': {'all': set(), 'owner': set()}} self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('New Mode'), _('rw')]) + def test_implicit_original_perms(self): + obj = FileRule.create_instance('/foo rw,') + obj.original_perms = {'allow': {'all': FileRule.ALL, 'owner': set()}} + self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('Old Mode'), _('mrwlkix'), _('New Mode'), _('rw')]) + + def test_owner_implicit_original_perms(self): + obj = FileRule.create_instance('/foo rw,') + obj.original_perms = {'allow': {'all': set(), 'owner': FileRule.ALL}} + self.assertEqual(obj.logprof_header(), [_('Path'), '/foo', _('Old Mode'), _('owner mrwlkix'), _('New Mode'), _('rw')]) + class FileEditHeaderTest(AATest): def _run_test(self, params, expected): From 2097e82d4a846a00e6fc7daad786483195d167c0 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Tue, 3 Sep 2024 09:47:11 -0300 Subject: [PATCH 2/2] utils: 'owner' should come before 'file' When both the owner and file keywords were used, the clean rule generated would have owner after file which is not accepted by the parser. Fixes: https://gitlab.com/apparmor/apparmor/-/issues/430 Signed-off-by: Georgia Garcia --- utils/apparmor/rule/file.py | 2 +- utils/test/test-file.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/utils/apparmor/rule/file.py b/utils/apparmor/rule/file.py index a0c6b6fe8..67dbf8d14 100644 --- a/utils/apparmor/rule/file.py +++ b/utils/apparmor/rule/file.py @@ -229,7 +229,7 @@ class FileRule(BaseRule): if self.all_paths and self.all_perms and not path and not perms and not target: return ('%s%s%sfile,%s' % (space, self.modifiers_str(), owner, self.comment)) # plain 'file,' rule elif not self.all_paths and not self.all_perms and path and perms: - return ('%s%s%s%s%s%s,%s' % (space, self.modifiers_str(), file_keyword, owner, path_and_perms, target, self.comment)) + return ('%s%s%s%s%s%s,%s' % (space, self.modifiers_str(), owner, file_keyword, path_and_perms, target, self.comment)) else: raise AppArmorBug('Invalid combination of path and perms in file rule - either specify path and perms, or none of them') diff --git a/utils/test/test-file.py b/utils/test/test-file.py index 05c496e69..472d3351b 100644 --- a/utils/test/test-file.py +++ b/utils/test/test-file.py @@ -406,6 +406,9 @@ class WriteFileTest(AATest): (' deny file /foo r,', 'deny file /foo r,'), (' deny file /foo wr,', 'deny file /foo rw,'), (' allow file /foo Pxrm -> bar,', 'allow file /foo mrPx -> bar,'), + (' deny owner file /foo r,', 'deny owner file /foo r,'), + (' deny owner file /foo wr,', 'deny owner file /foo rw,'), + (' allow owner file /foo Pxrm -> bar,', 'allow owner file /foo mrPx -> bar,'), (' deny owner /foo r,', 'deny owner /foo r,'), (' deny owner /foo wr,', 'deny owner /foo rw,'), (' allow owner /foo Pxrm -> bar,', 'allow owner /foo mrPx -> bar,'), @@ -420,6 +423,9 @@ class WriteFileTest(AATest): (' deny file r /foo,', 'deny file r /foo,'), (' deny file wr /foo ,', 'deny file rw /foo,'), (' allow file Pxmr /foo -> bar,', 'allow file mrPx /foo -> bar,'), + (' deny owner file r /foo ,', 'deny owner file r /foo,'), + (' deny owner file wr /foo ,', 'deny owner file rw /foo,'), + (' allow owner file Pxrm /foo -> bar,', 'allow owner file mrPx /foo -> bar,'), (' deny owner r /foo ,', 'deny owner r /foo,'), (' deny owner wr /foo ,', 'deny owner rw /foo,'), (' allow owner Pxrm /foo -> bar,', 'allow owner mrPx /foo -> bar,'),