[apparmor] [patch] Prevent 'wa' conflicts for file rules

[Christian Boltz]

Hello,

get_file_perms() and propose_file_rules() happily collect all file
permissions. This could lead to proposing 'wa' permissions in
aa-logprof, which then errored out because of conflicting permissions.

This patch adds a check to both functions that removes 'a' if 'w' is
present, and extends the tests to check this.


I propose this patch for trunk and 2.11.

Both functions (including this bug) were introduced together with
FileRule, so older releases are not affected.


[ 04-prevent-wa-conflicts.diff ]

=== modified file ./utils/apparmor/aa.py
--- utils/apparmor/aa.py        2017-07-16 21:43:30.714865518 +0200
+++ utils/apparmor/aa.py        2017-08-04 22:06:40.089466392 +0200
@@ -3420,6 +3420,9 @@
                     for perm in incperms[allow_or_deny][owner_or_all]:
                         perms[allow_or_deny][owner_or_all].add(perm)
 
+                    if 'a' in perms[allow_or_deny][owner_or_all] and 'w' in perms[allow_or_deny][owner_or_all]:
+                        perms[allow_or_deny][owner_or_all].remove('a')  # a is a subset of w, so remove it
+
             for incpath in incperms['paths']:
                 perms['paths'].add(incpath)
 
@@ -3444,6 +3447,9 @@
         merged_rule_obj.perms.add(perm)
         merged_rule_obj.raw_rule = None
 
+    if 'a' in merged_rule_obj.perms and 'w' in merged_rule_obj.perms:
+        merged_rule_obj.perms.remove('a')  # a is a subset of w, so remove it
+
     pathlist = {original_path} | existing_perms['paths'] | set(glob_common(original_path))
 
     for user_glob in user_globs:
=== modified file ./utils/test/test-aa.py
--- utils/test/test-aa.py       2017-07-11 13:33:54.882914000 +0200
+++ utils/test/test-aa.py       2017-08-04 22:04:29.821944241 +0200
@@ -781,6 +781,7 @@
 class AaTest_get_file_perms_2(AATest):
     tests = [
         ('/usr/share/common-licenses/foo/bar',      {'allow': {'all': {'r'},            'owner': {'w'}  }, 'deny': {'all':set(),    'owner': set()},    'paths': {'/usr/share/common-licenses/**'}              }),
+        ('/usr/share/common-licenses/what/ever',    {'allow': {'all': {'r'},            'owner': {'w'}  }, 'deny': {'all':set(),    'owner': set()},    'paths': {'/usr/share/common-licenses/**', '/usr/share/common-licenses/what/ever'}      }),
         ('/dev/null',                               {'allow': {'all': {'r', 'w', 'k'},  'owner': set()  }, 'deny': {'all':set(),    'owner': set()},    'paths': {'/dev/null'}                                  }),
         ('/foo/bar',                                {'allow': {'all': {'r', 'w'},       'owner': set()  }, 'deny': {'all':set(),    'owner': set()},    'paths': {'/foo/bar'}                                   }),  # exec perms not included
         ('/no/thing',                               {'allow': {'all': set(),            'owner': set()  }, 'deny': {'all':set(),    'owner': set()},    'paths': set()                                          }),
@@ -808,6 +809,7 @@
         profile['include']['abstractions/enchant'] = True  # includes abstractions/aspell
 
         profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/**  w,'))
+        profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/what/ever a,'))  # covered by the above 'w' rule, so 'a' should be ignored
         profile['file'].add(FileRule.parse('/dev/null rwk,'))
         profile['file'].add(FileRule.parse('/foo/bar rwix,'))
 
@@ -822,6 +824,7 @@
         (['/foo/bar',                           'rw'],  ['/foo/bar rw,']                                                                                                        ),
         (['/usr/lib/ispell/',                   'w'],   ['/{usr/,}lib{,32,64}/** rw,', '/usr/lib/ispell/ rw,']                                                                     ),
         (['/usr/lib/aspell/some.so',            'k'],   ['/usr/lib/aspell/* mrk,', '/usr/lib/aspell/*.so mrk,', '/{usr/,}lib{,32,64}/** mrk,', '/usr/lib/aspell/some.so mrk,']     ),
+        (['/foo/log',                           'w'],   ['/foo/log w,']                                                                                                            ),
     ]
 
     def _run_test(self, params, expected):
@@ -850,6 +853,7 @@
         profile['file'].add(FileRule.parse('owner /usr/share/common-licenses/**  w,'))
         profile['file'].add(FileRule.parse('/dev/null rwk,'))
         profile['file'].add(FileRule.parse('/foo/bar rwix,'))
+        profile['file'].add(FileRule.parse('/foo/log a,'))  # will be replaced with '/foo/log w,' (not 'wa')
 
         rule_obj = FileRule(params[0], params[1], None, FileRule.ALL, owner=False, log_event=True)
         proposals = propose_file_rules(profile, rule_obj)



Regards,

Christian Boltz
-- 
> I'm hesitant to accept this SR, it's fixing just a typo in description
> but will lead to rebuild of many packages in KDE3. I think this is not
> necessary and only waste of resources.
Don't worry. It's cold in Nuremberg atm, additional head produced
doesn't matter :)
[> dsterba and lnussel, https://build.opensuse.org/request/show/490049]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170804/a543688c/attachment-0001.pgp>