[apparmor] [patch] Several fixes for variable handling

[John Johansen]

On 11/15/2015 11:37 AM, Christian Boltz wrote:
> Hello,
> 
> parsing variables was broken in several ways:
> - empty quotes (representing an intentionally empty value) were lost,
>   causing parser failures
> - items consisting of only one letter were lost due to a bug in RE_VARS
> - RE_VARS didn't start with ^, which means leading garbage (= syntax
>   errors) was ignored
> - trailing garbage was also ignored
> 
> This patch fixes those issues in separate_vars() and changes
> var_transform() to write out empty quotes (instead of nothing) for empty
> values.
> 
> Also add some tests for separate_vars() with empty quotes and adjust
> several tests with invalid syntax to expect an AppArmorException.
> 
> var_transform() gets some tests added.
> 
> Finally, remove 3 testcases from the "fails to raise an exception" list
> in test-parser-simple-tests.py.
> 
> 
> I propose this patch for trunk and 2.9 (except the test-parser-simple-tests.py
> changes because this file isn't in 2.9)
> 
> 
Acked-by: John Johansen <john.johansen at canonical.com>

> 
> [ 20-variable-fixes.diff ]
> 
> === modified file ./utils/apparmor/aa.py
> --- utils/apparmor/aa.py        2015-11-15 20:30:42.251877998 +0100
> +++ utils/apparmor/aa.py        2015-11-15 20:01:17.937507807 +0100
> @@ -2800,7 +2800,7 @@
>  
>              list_var = strip_quotes(matches[0])
>              var_operation = matches[1]
> -            value = strip_quotes(matches[2])
> +            value = matches[2]
>  
>              if profile:
>                  if not profile_data[profile][hat].get('lvar', False):
> @@ -3165,12 +3165,16 @@
>  def separate_vars(vs):
>      """Returns a list of all the values for a variable"""
>      data = set()
> +    vs = vs.strip()
>  
> -    RE_VARS = re.compile('\s*((\".+?\")|([^\"]\S+))\s*(.*)$')
> +    RE_VARS = re.compile('^(("[^"]*")|([^"\s]+))\s*(.*)$')
>      while RE_VARS.search(vs):
>          matches = RE_VARS.search(vs).groups()
>          data.add(strip_quotes(matches[0]))
> -        vs = matches[3]
> +        vs = matches[3].strip()
> +
> +    if vs:
> +        raise AppArmorException('Variable assignments contains invalid parts (unbalanced quotes?): %s' % vs)
>  
>      return data
>  
> @@ -3298,6 +3302,8 @@
>  def var_transform(ref):
>      data = []
>      for value in ref:
> +        if not value:
> +            value = '""'
>          data.append(quote_if_needed(value))
>      return ' '.join(data)
>  
> === modified file ./utils/test/test-aa.py
> --- utils/test/test-aa.py       2015-11-15 20:30:42.251877998 +0100
> +++ utils/test/test-aa.py       2015-11-15 20:32:26.799188737 +0100
> @@ -17,7 +17,8 @@
>  
>  from apparmor.aa import (check_for_apparmor, get_interpreter_and_abstraction, create_new_profile,
>       get_profile_flags, set_profile_flags, is_skippable_file, is_skippable_dir,
> -     parse_profile_start, parse_profile_data, separate_vars, store_list_var, write_header, serialize_parse_profile_start)
> +     parse_profile_start, parse_profile_data, separate_vars, store_list_var, write_header,
> +     var_transform, serialize_parse_profile_start)
>  from apparmor.common import AppArmorException, AppArmorBug
>  
>  class AaTestWithTempdir(AATest):
> @@ -484,20 +485,29 @@
>          (''                             , set()                      ),
>          ('       '                      , set()                      ),
>          ('  foo bar'                    , {'foo', 'bar'             }),
> -        ('foo "  '                      , {'foo'                    }), # XXX " is ignored
> -        (' " foo '                      , {' "', 'foo'              }), # XXX really?
> +        ('foo "  '                      , AppArmorException          ),
> +        (' " foo '                      , AppArmorException          ), # half-quoted
>          ('  foo bar   '                 , {'foo', 'bar'             }),
> -        ('  foo bar   # comment'        , {'foo', 'bar', 'comment'  }), # XXX should comments be stripped?
> +        ('  foo bar   # comment'        , {'foo', 'bar', '#', 'comment'}), # XXX should comments be stripped?
>          ('foo'                          , {'foo'                    }),
>          ('"foo" "bar baz"'              , {'foo', 'bar baz'         }),
>          ('foo "bar baz" xy'             , {'foo', 'bar baz', 'xy'   }),
> -        ('foo "bar baz '                , {'foo', 'bar', 'baz'      }), # half-quoted
> +        ('foo "bar baz '                , AppArmorException          ), # half-quoted
>          ('  " foo" bar'                 , {' foo', 'bar'            }),
> +        ('  " foo" bar x'               , {' foo', 'bar', 'x'       }),
> +        ('""'                           , {''                       }), # empty value
> +        ('"" foo'                       , {'', 'foo'                }), # empty value + 'foo'
> +        ('"" foo "bar"'                 , {'', 'foo', 'bar'         }), # empty value + 'foo' + 'bar' (bar has superfluous quotes)
> +        ('"bar"'                        , {'bar'                    }), # 'bar' with superfluous quotes
>      ]
>  
>      def _run_test(self, params, expected):
> -        result = separate_vars(params)
> -        self.assertEqual(result, expected)
> +        if expected == AppArmorException:
> +            with self.assertRaises(expected):
> +                separate_vars(params)
> +        else:
> +            result = separate_vars(params)
> +            self.assertEqual(result, expected)
>  
>  
>  class AaTest_store_list_var(AATest):
> @@ -579,6 +589,17 @@
>          result = write_header(prof_data, depth, name, embedded_hat, write_flags)
>          self.assertEqual(result, [expected])
>  
> +class AaTest_var_transform(AATest):
> +    tests = [
> +        (['foo', ''],           'foo ""'        ),
> +        (['foo', 'bar'],        'foo bar'       ),
> +        ([''],                  '""'            ),
> +        (['bar baz', 'foo'],    '"bar baz" foo' ),
> +    ]
> +
> +    def _run_test(self, params, expected):
> +        self.assertEqual(var_transform(params), expected)
> +
>  class AaTest_serialize_parse_profile_start(AATest):
>      def _parse(self, line, profile, hat, prof_data_profile, prof_data_external):
>          # 'correct' is always True in the code that uses serialize_parse_profile_start() (set some lines above the function call)
> === modified file ./utils/test/test-parser-simple-tests.py
> --- utils/test/test-parser-simple-tests.py      2015-11-15 20:30:42.251877998 +0100
> +++ utils/test/test-parser-simple-tests.py      2015-11-11 00:58:06.378155721 +0100
> @@ -167,8 +167,6 @@
>      'vars/boolean/boolean_bad_6.sd',
>      'vars/boolean/boolean_bad_7.sd',
>      'vars/boolean/boolean_bad_8.sd',
> -    'vars/vars_bad_1.sd',
> -    'vars/vars_bad_2.sd',
>      'vars/vars_bad_3.sd',
>      'vars/vars_bad_4.sd',
>      'vars/vars_bad_5.sd',
> @@ -178,7 +176,6 @@
>      'vars/vars_bad_trailing_comma_2.sd',
>      'vars/vars_bad_trailing_comma_3.sd',
>      'vars/vars_bad_trailing_comma_4.sd',
> -    'vars/vars_bad_trailing_garbage_1.sd',
>      'vars/vars_dbus_bad_01.sd',
>      'vars/vars_dbus_bad_02.sd',
>      'vars/vars_dbus_bad_03.sd',
> 
> 
> Regards,
> 
> Christian Boltz
>