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

[Seth Arnold]

On Fri, Aug 04, 2017 at 10:17:42PM +0200, Christian Boltz wrote:
> 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.
> 

Acked-by: Seth Arnold <seth.arnold at canonical.com>
Acked for both

Thanks

> [ 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]



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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170804/f85a221a/attachment.pgp>