[apparmor] [patch] fix handling of adding to variables
Steve Beattie
steve at nxnw.org
Wed Apr 15 16:19:29 UTC 2015
On Wed, Apr 08, 2015 at 12:25:05AM +0200, Christian Boltz wrote:
> Hello,
>
> the LibreOffice profile uncovered that handling of @{var} += is broken:
>
> File ".../utils/apparmor/aa.py", line 3272, in store_list_var
> var[list_var] = set(var[list_var] + vlist)
> TypeError: unsupported operand type(s) for +: 'set' and 'list'
>
> This patch fixes it:
> - change separate_vars() to use and return a set instead of a list
> (FYI: separate_vars() is only called by store_list_var())
> - adoptstore_list_var() to expect a set
> - remove some old comments in these functions
> - explain the less-intuitive parameters of store_list_var()
>
> Also add some tests for separate_vars() and store_list_var().
> The tests were developed based on the old code, but not all of them
> succeed with the old code.
>
> As usual, the tests uncovered some interesting[tm] behaviour in
> separate_vars() (see the XXX comments and tell me what the really
> expected behaviour is ;-)
>
> Since this is broken in trunk and the 2.9 branch, I propose this patch
> for trunk and 2.9.
>
> Oh, and I noticed that serialize_profile_from_old_profile() also crashes
> when it hits "@{var} +=", but I didn't debug that yet.
>
>
> [ 33-fix-add-to-variable-and-add-tests.diff ]
Acked-by: Steve Beattie <steve at nxnw.org> for trunk and 2.9
Some comments on the questioned test cases inlined:
> === modified file utils/apparmor/aa.py
> --- utils/apparmor/aa.py 2015-04-08 00:09:02.931423160 +0200
> +++ utils/apparmor/aa.py 2015-04-08 00:13:59.273822091 +0200
> @@ -3241,13 +3241,12 @@
>
> def separate_vars(vs):
> """Returns a list of all the values for a variable"""
> - data = []
> + data = set()
>
> - #data = [i.strip('"') for i in vs.split()]
> RE_VARS = re.compile('\s*((\".+?\")|([^\"]\S+))\s*(.*)$')
> while RE_VARS.search(vs):
> matches = RE_VARS.search(vs).groups()
> - data.append(strip_quotes(matches[0]))
> + data.add(strip_quotes(matches[0]))
> vs = matches[3]
>
> return data
> @@ -3259,17 +3258,19 @@
> return False
>
> def store_list_var(var, list_var, value, var_operation, filename):
> - """Store(add new variable or add values to variable) the variables encountered in the given list_var"""
> + """Store(add new variable or add values to variable) the variables encountered in the given list_var
> + - the 'var' parameter will be modified
> + - 'list_var' is the variable name, for example '@{foo}'
> + """
> vlist = separate_vars(value)
> if var_operation == '=':
> if not var.get(list_var, False):
> var[list_var] = set(vlist)
> else:
> - #print('Ignored: New definition for variable for:',list_var,'=', value, 'operation was:',var_operation,'old value=', var[list_var])
> raise AppArmorException(_('Redefining existing variable %(variable)s: %(value)s in %(file)s') % { 'variable': list_var, 'value': value, 'file': filename })
> elif var_operation == '+=':
> if var.get(list_var, False):
> - var[list_var] = set(var[list_var] + vlist)
> + var[list_var] |= vlist
> else:
> raise AppArmorException(_('Values added to a non-existing variable %(variable)s: %(value)s in %(file)s') % { 'variable': list_var, 'value': value, 'file': filename })
> else:
> === modified file utils/test/test-aa.py
> --- utils/test/test-aa.py 2015-04-08 00:09:02.934422982 +0200
> +++ utils/test/test-aa.py 2015-04-07 23:14:27.316826914 +0200
> @@ -16,7 +16,7 @@
> import tempfile
> from common_test import read_file, write_file
>
> -from apparmor.aa import check_for_apparmor, get_profile_flags, set_profile_flags, is_skippable_file, is_skippable_dir, parse_profile_start, write_header, serialize_parse_profile_start
> +from apparmor.aa import check_for_apparmor, get_profile_flags, set_profile_flags, is_skippable_file, is_skippable_dir, parse_profile_start, separate_vars, store_list_var, write_header, serialize_parse_profile_start
> from apparmor.common import AppArmorException, AppArmorBug
>
> class AaTestWithTempdir(AATest):
> @@ -349,6 +349,66 @@
> with self.assertRaises(AppArmorBug):
> self._parse('xy', '/bar', '/bar') # not a profile start
>
> +class AaTest_separate_vars(AATest):
> + tests = [
> + ('' , set() ),
> + (' ' , set() ),
> + (' foo bar' , {'foo', 'bar' }),
> + ('foo " ' , {'foo' }), # XXX " is ignored
If this is the raw variable assignment string included in the profile,
then this is invalid syntax (the parser will want a close quote):
$ echo -e '@{FOO}=foo " \n profile /t { /@{FOO} r, }'
@{FOO}=foo "
profile /t { /@{FOO} r, }
$ echo -e '@{FOO}=foo " \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables
AppArmor parser error, in stdin line 1: Found unexpected character: '"'
> + (' " foo ' , {' "', 'foo' }), # XXX really?
Same. Note however, that a backslash escaped " is valid:
$ echo -e '@{FOO}= \" foo \n profile /t { /@{FOO} r, }'
@{FOO}= \" foo
profile /t { /@{FOO} r, }
$ echo -e '@{FOO}= \" foo \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables
@FOO = "foo" """
> + (' foo bar ' , {'foo', 'bar' }),
> + (' foo bar # comment' , {'foo', 'bar', 'comment' }), # XXX should comments be stripped?
Hrm, I think the parser has a bug here:
$ echo -e '@{FOO}= foo # comment \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables
@FOO = "comment" "#" "foo"
> + ('foo' , {'foo' }),
> + ('"foo" "bar baz"' , {'foo', 'bar baz' }),
> + ('foo "bar baz" xy' , {'foo', 'bar baz', 'xy' }),
> + ('foo "bar baz ' , {'foo', 'bar', 'baz' }), # half-quoted
Again, invalid syntax:
$ echo -e '@{FOO}= foo "bar baz \n profile /t { /@{FOO} r, }' | apparmor_parser -qQK --dump variables
AppArmor parser error, in stdin line 1: Found unexpected character: '"'
> + (' " foo" bar' , {' foo', 'bar' }),
> + ]
> +
> + def _run_test(self, params, expected):
> + result = separate_vars(params)
> + self.assertEqual(result, expected)
> +
> +
> +class AaTest_store_list_var(AATest):
> + tests = [
> + # old var value operation expected (False for exception)
> + ([ {} , 'foo' , '=' ], {'foo'} ), # set
> + ([ {} , 'foo bar' , '=' ], {'foo', 'bar'} ), # set multi
> + ([ {'@{var}': {'foo'}} , 'bar' , '=' ], False ), # redefine var
> + ([ {} , 'bar' , '+=' ], False ), # add to undefined var
> + ([ {'@{var}': {'foo'}} , 'bar' , '+=' ], {'foo', 'bar'} ), # add
> + ([ {'@{var}': {'foo'}} , 'bar baz' , '+=' ], {'foo', 'bar', 'baz'} ), # add multi
> + ([ {'@{var}': {'foo', 'xy'}} , 'bar baz' , '+=' ], {'foo', 'xy', 'bar', 'baz'} ), # add multi to multi
> + ([ {} , 'foo' , '-=' ], False ), # unknown operation
> + ]
> +
> + def _run_test(self, params, expected):
> + var = params[0]
> + value = params[1]
> + operation = params[2]
> +
> + if not expected:
> + with self.assertRaises(AppArmorException):
> + store_list_var(var, '@{var}', value, operation, 'somefile')
> + return
> +
> + # dumy value that must not be changed
> + var['@{foo}'] = {'one', 'two'}
> +
> + exp_var = {
> + '@{foo}': {'one', 'two'},
> + '@{var}': expected,
> + }
> +
> + store_list_var(var, '@{var}', value, operation, 'somefile')
> +
> + self.assertEqual(var.keys(), exp_var.keys())
> +
> + for key in exp_var:
> + self.assertEqual(var[key], exp_var[key])
> +
> +
> class AaTest_write_header(AATest):
> tests = [
> # name embedded_hat write_flags depth flags attachment prof.keyw. comment expected
--
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/20150415/58d2fea9/attachment.pgp>
More information about the AppArmor
mailing list