[apparmor] [patch] fix aa-complain to work with quoted profile names

[Steve Beattie]

Hey Christian,

On Wed, Jun 11, 2014 at 12:11:29AM +0200, Christian Boltz wrote:
> the attached patch fixes a crash in aa-complain when a profile name is 
> quoted. It also makes sure aa-complain actually adds the complain flag 
> in such cases. (aa-enforce etc. will also benefit from this fix.)
> 
> Note: superfluous quotes will be removed when saving the profile (for 
> example with aa-cleanprof), but they are kept if needed, like in
>     profile "/bin/foo bar"
> (tested with aa-complain and aa-cleanprof - and also with "rcapparmor 
> reload", where the initscript bailed out because my profile filename 
> contained a space...)
> 
> The patch also adds some TODO notes.
> 
> References: https://bugs.launchpad.net/apparmor/+bug/1296218
> 
> 
> There are other regexes that handle quotes: 
>     RE_PROFILE_ALIAS
>     RE_PROFILE_CHANGE_HAT
>     RE_PROFILE_HAT_DEF
> They probably also need to be changed to work with quotes (can someone 
> test them, please?), but that can be a separate patch.
> 
> I also noticed that aa-cleanprof (and therefore probably all python 
> tools) adds additional quotes in file rules, so
>   "/bin/foo bar" mrix,
> becomes
>   ""/bin/foo bar"" mrix,
> and in the next run
>   """/bin/foo bar""" mrix,
> 
> One more patch to write...

Just one? Surely you jest...

> === modified file 'utils/apparmor/aa.py'
> --- utils/apparmor/aa.py	2014-05-22 17:43:10 +0000
> +++ utils/apparmor/aa.py	2014-05-28 22:00:45 +0000
> @@ -630,7 +630,11 @@
>  
>  def set_profile_flags(prof_filename, program, newflags):
>      """Reads the old profile file and updates the flags accordingly"""
> -    regex_bin_flag = re.compile('^(\s*)(("??/.+?"??)|(profile\s+("??.+?"??)))\s+((flags=)?\((.*)\)\s+)?\{\s*(#.*)?$')
> +    regex_bin_flag = re.compile('^(\s*)("?(/.+?)"??|(profile\s+"?(.+?)"??))\s+((flags=)?\((.*)\)\s+)?\{\s*(#.*)?$')

A different way to do this regex without having to
conditionally check on whether one of the matches fields has
been set would be to use python's named groups (and maybe
non-matching groups as well, to cope with the nested groups).
https://docs.python.org/3/howto/regex.html#non-capturing-and-named-groups
covers how to do so. RE_HAS_COMMENT_SPLIT is also an example of this.

> +    # TODO: use RE_PROFILE_START (only difference: doesn't have a match group for the leading space)
> +    # TODO: also use the global regex for matching the hat
> +    # 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
>      regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)\s*(#.*)?$')
>      if os.path.isfile(prof_filename):
>          with open_file_read(prof_filename) as f_in:
> @@ -648,13 +652,18 @@
>                          matches = match.groups()
>                          space = matches[0]
>                          binary = matches[1]

Minor nit, if you're setting binary to either matches[2] or matches[4]
below, is there any reason to keep the assignment above?

> +                        profile = matches[1]  # profile name including quotes and "profile" keyword
> +                        if matches[2]:
> +                            binary = matches[2]
> +                        else:
> +                            binary = matches[4]
>                          flag = matches[6] or 'flags='
>                          flags = matches[7]
>                          if binary == program or program is None:
>                              if newflags:
> -                                line = '%s%s %s(%s) {%s\n' % (space, binary, flag, newflags, comment)
> +                                line = '%s%s %s(%s) {%s\n' % (space, profile, flag, newflags, comment)
>                              else:
> -                                line = '%s%s {%s\n' % (space, binary, comment)
> +                                line = '%s%s {%s\n' % (space, profile, comment)
>                      else:
>                          match = regex_hat_flag.search(line)
>                          if match:
> @@ -2607,7 +2616,7 @@
>          profiles[p] = deepcopy(profile_data[p])
>  
>  ## Profile parsing regex
> -RE_PROFILE_START = re.compile('^\s*(("??/.+?"??)|(profile\s+("??.+?"??)))\s+((flags=)?\((.+)\)\s+)?\{\s*(#.*)?$')
> +RE_PROFILE_START = re.compile('^\s*("?(/.+?)"??|(profile\s+"?(.+?)"??))\s+((flags=)?\((.+)\)\s+)?\{\s*(#.*)?$')

I sure would like testcases for these...

Anyway. If you address the minor nit, I think this is all okay and thus,
Acked-by: Steve Beattie <steve at nxnw.org>.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140618/5756133c/attachment.pgp>