[apparmor] [patch 11/11] utils: add simple parsing of multi-line rules
Christian Boltz
apparmor at cboltz.de
Thu Mar 6 21:10:16 UTC 2014
Hello,
Am Mittwoch, 5. März 2014 schrieb Steve Beattie:
> D-Bus rules in particular seem to get written as multi-line rules.
> This patch adds very simple hackish support for multiple lines.
> Essentially, what it does is if the parsing of a line doesn't match
> anything and falls all the way through, it saves the line and
> prepends it to the next line that occurs in the profile, but *only*
> if the line does not have a trailing comma to indicate the end of a
> rule. If the trailing comma exists, then it assumes that it's a rule
> that it doesn't understand and aborts.
Just for the records: this works for all rule types, not only for dbus
rules.
> With this patch, the simpler tools (aa-enforce, aa-complain, etc.) can
> parse policies containing multi-line rules to an extent and continue
> to function correctly. Again, aa-logprof and aa-genprof may have
> issues on the writing back of profiles, so some assistance testing
> here would be appreciated.
>
> Some testcases are added to exercise the regex that looks for a rule
> with a trailing comma but can still handle rules that have (,) or {,}
> in them.
>
> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---
> utils/apparmor/aa.py | 29 +++++++++++++++++-
> utils/apparmor/tools.py | 5 ---
> utils/test/test-dbus_parse.py | 66
> ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95
> insertions(+), 5 deletions(-)
>
> Index: b/utils/apparmor/aa.py
> ===================================================================
> --- a/utils/apparmor/aa.py
> +++ b/utils/apparmor/aa.py
> @@ -2615,7 +2615,8 @@ RE_PROFILE_CHANGE_HAT = re.compile('^\s*
> RE_PROFILE_HAT_DEF =
> re.compile('^\s*\^(\"??.+?\"??)\s+((flags=)?\((.+)\)\s+)*\{\s*(#.*)?$
> ') RE_NETWORK_FAMILY_TYPE = re.compile('\s+(\S+)\s+(\S+)\s*,$')
> RE_NETWORK_FAMILY = re.compile('\s+(\S+)\s*,$')
> -RE_PROFILE_DBUS =
> re.compile('^\s*(audit\s+)?(allow\s+|deny\s+)?(dbus[^#]*)\s*(#.*)?$')
> +RE_PROFILE_DBUS =
> re.compile('^\s*(audit\s+)?(allow\s+|deny\s+)?(dbus[^#]*\s*,)\s*(#.*)
> ?$') +RE_RULE_HAS_COMMA =
> re.compile('[^,)({}]*(\(([^),]*,)*[^),]*\)|\{([^},]*,)*[^},]*\})*[^,)
> ({}]*,\s*(#.*)?$')
Is it intentional that RE_RULE_HAS_COMMA does not start with ^ ?
Besides that, it would be nice to split the regex into multiple strings
to avoid headache ;-)
A good beginning would be something like:
RE_RULE_HAS_COMMA = re.compile('[^,)({}]*' + '(' + '\(([^),]*,)*' +
'[^),]*\)' + '|' + .........
My "debugging" split (with added whitespace) looks like this:
[^,)({}]* ( \( ([^),]*,)* [^),]*\) | \{ ([^},]*,)* [^},]*\}
)* [^,)({}]*,\s*(#.*)?$
I'd guess (untested!) you can simplify
( \( ([^),]*,)* [^),]*\)
to
\( [^)]* \)
because * can also mean "zero times".
The same applies to {...} in rules.
Note: it's perfectly valid to have something like {foo,} and {bar,} so
replacing * with + will break on valid rules.
> def parse_profile_data(data, file, do_include):
> profile_data = hasher()
> @@ -2625,6 +2626,7 @@ def parse_profile_data(data, file, do_in
> repo_data = None
> parsed_profiles = []
> initial_comment = ''
> + lastline = None
>
> if do_include:
> profile = file
> @@ -2633,8 +2635,12 @@ def parse_profile_data(data, file, do_in
> line = line.strip()
> if not line:
> continue
> + # we're dealing with a multiline statement
> + if lastline:
> + line = '%s %s' % (lastline, line)
You can probably (untested ;-) also set "lastline = None" here (since
you added it to line, it isn't needed anymore). This also makes the
following >10 "lastline = None" superfluous.
> # Starting line of a profile
> if RE_PROFILE_START.search(line):
> + lastline = None
> matches = RE_PROFILE_START.search(line).groups()
>
> if profile:
> @@ -2691,6 +2697,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][profile]['repo']['user'] =
> repo_data['user']
>
> elif RE_PROFILE_END.search(line):
> + lastline = None
> # If profile ends and we're not in one
> if not profile:
> raise AppArmorException(_('Syntax Error: Unexpected
> End of Profile reached in file: %s line: %s') % (file, lineno + 1))
> @@ -2705,6 +2712,7 @@ def parse_profile_data(data, file, do_in
> initial_comment = ''
>
> elif RE_PROFILE_CAP.search(line):
> + lastline = None
> matches = RE_PROFILE_CAP.search(line).groups()
>
> if not profile:
> @@ -2724,6 +2732,7 @@ def parse_profile_data(data, file, do_in
>
> profile_data[profile][hat][allow]['capability'][capability]['audit']
> = audit
>
> elif RE_PROFILE_LINK.search(line):
> + lastline = None
> matches = RE_PROFILE_LINK.search(line).groups()
>
> if not profile:
> @@ -2752,6 +2761,7 @@ def parse_profile_data(data, file, do_in
>
> profile_data[profile][hat][allow]['link'][link]['audit'] = set()
>
> elif RE_PROFILE_CHANGE_PROFILE.search(line):
> + lastline = None
> matches = RE_PROFILE_CHANGE_PROFILE.search(line).groups()
>
> if not profile:
> @@ -2761,6 +2771,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][hat]['changes_profile'][cp] = True
>
> elif RE_PROFILE_ALIAS.search(line):
> + lastline = None
> matches = RE_PROFILE_ALIAS.search(line).groups()
>
> from_name = strip_quotes(matches[0])
> @@ -2774,6 +2785,7 @@ def parse_profile_data(data, file, do_in
> filelist[file]['alias'][from_name] = to_name
>
> elif RE_PROFILE_RLIMIT.search(line):
> + lastline = None
> matches = RE_PROFILE_RLIMIT.search(line).groups()
>
> if not profile:
> @@ -2785,6 +2797,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][hat]['rlimit'][from_name] = to_name
>
> elif RE_PROFILE_BOOLEAN.search(line):
> + lastline = None
> matches = RE_PROFILE_BOOLEAN.search(line)
>
> if not profile:
> @@ -2796,6 +2809,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][hat]['lvar'][bool_var] = value
>
> elif RE_PROFILE_VARIABLE.search(line):
> + lastline = None
> # variable additions += and =
> matches = RE_PROFILE_VARIABLE.search(line).groups()
>
> @@ -2813,18 +2827,22 @@ def parse_profile_data(data, file, do_in
> store_list_var(filelist[file]['lvar'], list_var,
> value, var_operation)
>
> elif RE_PROFILE_CONDITIONAL.search(line):
> + lastline = None
> # Conditional Boolean
> pass
>
> elif RE_PROFILE_CONDITIONAL_VARIABLE.search(line):
> + lastline = None
> # Conditional Variable defines
> pass
>
> elif RE_PROFILE_CONDITIONAL_BOOLEAN.search(line):
> + lastline = None
> # Conditional Boolean defined
> pass
>
> elif RE_PROFILE_PATH_ENTRY.search(line):
> + lastline = None
> matches = RE_PROFILE_PATH_ENTRY.search(line).groups()
>
> if not profile:
> @@ -2874,6 +2892,7 @@ def parse_profile_data(data, file, do_in
>
> profile_data[profile][hat][allow]['path'][path]['audit'] = set()
>
> elif re_match_include(line):
> + lastline = None
> # Include files
> include_name = re_match_include(line)
> if include_name.startswith('local/'):
> @@ -2901,6 +2920,7 @@ def parse_profile_data(data, file, do_in
> load_include(include_name)
>
> elif RE_PROFILE_NETWORK.search(line):
> + lastline = None
> matches = RE_PROFILE_NETWORK.search(line).groups()
>
> if not profile:
> @@ -2931,6 +2951,7 @@ def parse_profile_data(data, file, do_in
>
> profile_data[profile][hat][allow]['netdomain']['audit']['all'] =
> audit # True
>
> elif RE_PROFILE_DBUS.search(line):
> + lastline = None
> matches = RE_PROFILE_DBUS.search(line).groups()
>
> if not profile:
> @@ -2954,6 +2975,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][hat][allow]['dbus'] = dbus_rules
>
> elif RE_PROFILE_CHANGE_HAT.search(line):
> + lastline = None
> matches = RE_PROFILE_CHANGE_HAT.search(line).groups()
>
> if not profile:
> @@ -2966,6 +2988,7 @@ def parse_profile_data(data, file, do_in
> profile_data[profile][hat]['declared'] = True
>
> elif RE_PROFILE_HAT_DEF.search(line):
> + lastline = None
> # An embedded hat syntax definition starts
> matches = RE_PROFILE_HAT_DEF.search(line).groups()
> if not profile:
> @@ -2989,6 +3012,7 @@ def parse_profile_data(data, file, do_in
> filelist[file]['profiles'][profile][hat] = True
>
> elif line[0] == '#':
> + lastline = None
As written above, please drop all those repeated "lastline = None" in
each of the if/elif branches.
> # Handle initial comments
> if not profile:
> if line.startswith('# Last Modified:'):
> @@ -3007,6 +3031,9 @@ def parse_profile_data(data, file, do_in
> else:
> initial_comment = initial_comment + line + '\n'
>
> + elif not RE_RULE_HAS_COMMA.search(line):
> + # Bah, line continues on to the next line
> + lastline = line
> else:
> raise AppArmorException(_('Syntax Error: Unknown line
> found in file: %s line: %s') % (file, lineno + 1))
>
> Index: b/utils/test/test-dbus_parse.py
> ===================================================================
> --- a/utils/test/test-dbus_parse.py
> +++ b/utils/test/test-dbus_parse.py
Hmm, that's a strange filename for testing a regex that works for all
rule types ;-)
> @@ -26,5 +26,71 @@ class AAParseDBUSTest(unittest.TestCase)
> self.assertEqual(dstring, dbus.serialize(),
> 'dbus object returned "%s", expected "%s"' %
> (dbus.serialize(), dstring))
>
> +
> +class AARegexHasComma(unittest.TestCase):
> +
> + def _check(self, line, expected=True):
> + result = aa.RE_RULE_HAS_COMMA.search(line)
> + if expected:
> + self.assertTrue(result, 'Couldn\'t find a comma in "%s"'
> % line) + else:
> + self.assertEqual(None, result, 'Found an unexpected comma
> in "%s"' % line) +
> + def test_with_comma_01(self):
> + '''simple comma check'''
> + self._check('dbus send,')
> +
> + def test_with_comma_paren_01(self):
> + '''simple comma check w/commas embedded in parens'''
> + self._check('dbus (r, w, bind, eavesdrop),')
> +
> + def test_with_comma_paren_02(self):
> + '''simple comma check w/commas embedded in parens'''
> + self._check('dbus (r, w,, bind, eavesdrop) ,')
> +
> + def test_with_comma_paren_03(self):
> + '''simple comma check w/empty parens'''
> + self._check('dbus () ,')
> +
> + def test_with_comma_curly_brace_01(self):
> + '''simple comma check w/commas embedded in curly braces'''
> +
> self._check('member={Hello,AddMatch,RemoveMatch,GetNameOwner,NameHasO
> wner,StartServiceByName} , ') +
> + def test_with_comma_curly_brace_02(self):
> + '''simple comma check w/commas embedded in curly braces'''
> +
> self._check('member={Hello,,,,,,AddMatch,RemoveMatch,GetNameOwner,Nam
> eHasOwner,StartServiceByName} , ') +
> + def test_with_comma_curly_brace_03(self):
> + '''simple comma check w/empty curly braces'''
> + self._check('member={} , ')
> +
> + def test_without_comma_01(self):
> + '''simple no comma check'''
> + self._check('dbus eavesdrop # ', False)
> +
> + def test_without_comma_paren_01(self):
> + '''simple no comma check w/commas embedded in parens'''
> + self._check('dbus (r, w, bind, eavesdrop)', False)
> +
> + def test_without_comma_paren_02(self):
> + '''simple no comma check w/commas embedded in parens'''
> + self._check('dbus (r, w,,,, bind, eavesdrop)', False)
> +
> + def test_without_comma_paren_03(self):
> + '''simple no comma check w/empty parens'''
> + self._check('dbus () ', False)
> +
> + def test_without_comma_curly_brace_01(self):
> + '''simple no comma check w/commas embedded in curly braces'''
> +
> self._check('member={Hello,AddMatch,RemoveMatch,GetNameOwner,NameHasO
> wner,StartServiceByName}', False) +
> + def test_without_comma_curly_brace_02(self):
> + '''simple no comma check w/commas embedded in curly braces'''
> +
> self._check('member={Hello,,,,,,AddMatch,RemoveMatch,GetNameOwner,Nam
> eHasOwner,StartServiceByName} ', False) +
> + def test_without_comma_curly_brace_03(self):
> + '''simple no comma check w/empty curly braces'''
> + self._check('member={} ', False)
> +
> if __name__ == '__main__':
> unittest.main()
I'd merge the testcases into arrays, so that you have one array for
"with comma" and one for "without comma", and then loop over the arrays.
This should keep the code more readable, even if we add 50 additional
test rules.
Speaking about additional test rules - please also add testcases for:
member={Hello,AddMatch,RemoveMatch,
dbus (r, w,
audit "/tmp/foo, # bar"
audit "/tmp/foo, # bar" rw,
audit "/tmp/foo, # bar" rw, # comment
The expected results should be obvious, even if those testcases are a
bit ;-) evil.
Regards,
Christian Boltz
--
> In case someone reads this and does not understand irony: this is not
> a valid solution for something you want to submit to openSUSE:Factory
OF course Im aware that such subversive hack will not be accepted there,
Im just exploring the endless possibilities of evil ;-)
[> Stephan Kulow and Cristian Rodríguez in opensuse-packaging]
More information about the AppArmor
mailing list