Several fixes for variable handling

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.



Acked-by: John Johansen <john.johansen@canonical.com> for trunk and 2.9
(which also implies 2.10)

Note: 2.9 doesn't have test-parser-simple-tests.py, therefore it won't
get that part of the patch.

utils/apparmor/aa.py

@@ -2768,7 +2768,7 @@ def parse_profile_data(data, file, do_include):
 
             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):
@@ -3133,12 +3133,16 @@ def parse_unix_rule(line):
 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
 
@@ -3266,6 +3270,8 @@ def write_rlimits(prof_data, depth):
 def var_transform(ref):
     data = []
     for value in ref:
+        if not value:
+            value = '""'
         data.append(quote_if_needed(value))
     return ' '.join(data)
 

utils/test/test-aa.py

@@ -17,7 +17,8 @@ import os
 
 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 @@ class AaTest_separate_vars(AATest):
         (''                             , 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 @@ class AaTest_write_header(AATest):
         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)

utils/test/test-parser-simple-tests.py

@@ -167,8 +167,6 @@ exception_not_raised = [
     '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 @@ exception_not_raised = [
     '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',