[apparmor] How I found several bugs in less than an hour - without even searching for them
John Johansen
john.johansen at canonical.com
Tue Oct 20 20:40:42 UTC 2015
On 07/05/2015 12:25 PM, Christian Boltz wrote:
> Hello,
>
> yes, that's possible, and you won't believe how easy it was! I even got
> additional 5% test coverage with this simple trick! Read on to see the
> most useful patch since sliced bread!
>
> Let's see if the clickbait approach in the subject helps to get this
> patch reviewed quickly ;-) - and now back to serious mode ;-)
>
well looking at the turn around time, I think we can definitively say
the clickbait approach sent this to the mental spam bucket :)
>
> [patch] Parse all parser simple_tests with the utils code
>
> This patch adds a testcase that parses all tests in the parser/tst/simple_tests/
> directory with parse_profile_data() to ensure that everything with valid
> syntax is accepted, and that all tests marked as FAIL raise an
> exception.
>
> This already resulted in
> - several patches to fix low-hanging fruits (including some bugs in the
> parser simple_tests itsself)
> - a list of tests that don't behave as expected. Those files get their
> expected result reverted to make sure we notice any change in the
> tools behaviour, especially changing to the really expected resulted.
> This method also makes sure that the testcase doesn't report any of
> the known failures.
> - a 5% improvement in test coverage - mostly caused by nearly completely
> covering parse_profile_data. BTW: there are no parser tests for
> pivot_root, and some FAIL tests with various rules outside of a
> profile would also be a good idea. (See aa.py parse_profile_data()
> coverage to get an idea which tests are missing.)
>
> As indicated above, the tools don't work as expected on all test
> profiles - most of the failures happen on expected-to-fail tests that
> pass parse_profile_data() without raising an exception. There are also
> some tests failing despite valid syntax, often with rarely used syntax
> like if conditions and qualifier blocks.
>
> Nevertheless, having 1311 tests with the expected result, 181 not raised
> exceptions, 21 completely unknown lines and 29 tests failing as syntax error
> doesn't look that bad ;-)
>
>
Acked-by: John Johansen <john.johansen at canonical.com>
>
> [ 69-test-parser-simple-tests.diff ]
>
> === modified file utils/test/test-parser-simple-tests.py
> --- utils/test/test-parser-simple-tests.py 2015-07-05 20:46:56.467393181 +0200
> +++ utils/test/test-parser-simple-tests.py 2015-07-05 18:46:41.638636268 +0200
> @@ -0,0 +1,395 @@
> +#! /usr/bin/env python
> +# ------------------------------------------------------------------
> +#
> +# Copyright (C) 2015 Christian Boltz <apparmor at cboltz.de>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of version 2 of the GNU General Public
> +# License published by the Free Software Foundation.
> +#
> +# ------------------------------------------------------------------
> +
> +import unittest
> +from common_test import AATest, setup_all_loops
> +import apparmor.aa as apparmor
> +
> +import os
> +from apparmor.common import open_file_read, AppArmorException
> +
> +# testcases that should raise an exception, but don't
> +exception_not_raised = [
> + 'capability/bad_1.sd',
> + 'capability/bad_2.sd',
> + 'capability/bad_3.sd',
> + 'capability/bad_4.sd',
> + 'change_hat/bad_parsing.sd',
> + 'dbus/bad_bind_1.sd',
> + 'dbus/bad_bind_2.sd',
> + 'dbus/bad_eavesdrop_1.sd',
> + 'dbus/bad_modifier_1.sd',
> + 'dbus/bad_modifier_2.sd',
> + 'dbus/bad_modifier_3.sd',
> + 'dbus/bad_modifier_4.sd',
> + 'dbus/bad_peer_1.sd',
> + 'dbus/bad_regex_01.sd',
> + 'dbus/bad_regex_02.sd',
> + 'dbus/bad_regex_03.sd',
> + 'dbus/bad_regex_04.sd',
> + 'dbus/bad_regex_05.sd',
> + 'dbus/bad_regex_06.sd',
> + 'file/bad_append_1.sd',
> + 'file/bad_append_2.sd',
> + 'file/bad_embedded_spaces_1.sd',
> + 'file/bad_re_brace_2.sd',
> + 'file/bad_re_brace_3.sd',
> + 'file/file/bad_append_1.sd',
> + 'file/file/bad_embedded_spaces_1.sd',
> + 'file/file/owner/bad_3.sd',
> + 'file/file/owner/bad_5.sd',
> + 'file/owner/bad_3.sd',
> + 'file/owner/bad_5.sd',
> + 'mount/bad_opt_10.sd',
> + 'mount/bad_opt_11.sd',
> + 'mount/bad_opt_12.sd',
> + 'mount/bad_opt_13.sd',
> + 'mount/bad_opt_14.sd',
> + 'mount/bad_opt_15.sd',
> + 'mount/bad_opt_16.sd',
> + 'mount/bad_opt_17.sd',
> + 'mount/bad_opt_18.sd',
> + 'mount/bad_opt_19.sd',
> + 'mount/bad_opt_1.sd',
> + 'mount/bad_opt_20.sd',
> + 'mount/bad_opt_21.sd',
> + 'mount/bad_opt_22.sd',
> + 'mount/bad_opt_23.sd',
> + 'mount/bad_opt_24.sd',
> + 'mount/bad_opt_2.sd',
> + 'mount/bad_opt_3.sd',
> + 'mount/bad_opt_4.sd',
> + 'mount/bad_opt_5.sd',
> + 'mount/bad_opt_6.sd',
> + 'mount/bad_opt_7.sd',
> + 'mount/bad_opt_8.sd',
> + 'mount/bad_opt_9.sd',
> + 'profile/flags/flags_bad10.sd',
> + 'profile/flags/flags_bad11.sd',
> + 'profile/flags/flags_bad12.sd',
> + 'profile/flags/flags_bad13.sd',
> + 'profile/flags/flags_bad15.sd',
> + 'profile/flags/flags_bad18.sd',
> + 'profile/flags/flags_bad19.sd',
> + 'profile/flags/flags_bad20.sd',
> + 'profile/flags/flags_bad2.sd',
> + 'profile/flags/flags_bad3.sd',
> + 'profile/flags/flags_bad4.sd',
> + 'profile/flags/flags_bad5.sd',
> + 'profile/flags/flags_bad6.sd',
> + 'profile/flags/flags_bad7.sd',
> + 'profile/flags/flags_bad8.sd',
> + 'profile/flags/flags_bad_debug_1.sd',
> + 'profile/flags/flags_bad_debug_2.sd',
> + 'profile/flags/flags_bad_debug_3.sd',
> + 'profile/flags/flags_bad_debug_4.sd',
> + 'profile/simple_bad_no_close_brace4.sd',
> + 'ptrace/bad_01.sd',
> + 'ptrace/bad_02.sd',
> + 'ptrace/bad_03.sd',
> + 'ptrace/bad_04.sd',
> + 'ptrace/bad_05.sd',
> + 'ptrace/bad_06.sd',
> + 'ptrace/bad_07.sd',
> + 'ptrace/bad_08.sd',
> + 'ptrace/bad_10.sd',
> + 'signal/bad_01.sd',
> + 'signal/bad_02.sd',
> + 'signal/bad_03.sd',
> + 'signal/bad_04.sd',
> + 'signal/bad_05.sd',
> + 'signal/bad_06.sd',
> + 'signal/bad_07.sd',
> + 'signal/bad_08.sd',
> + 'signal/bad_09.sd',
> + 'signal/bad_10.sd',
> + 'signal/bad_11.sd',
> + 'signal/bad_12.sd',
> + 'signal/bad_13.sd',
> + 'signal/bad_14.sd',
> + 'signal/bad_15.sd',
> + 'signal/bad_16.sd',
> + 'signal/bad_17.sd',
> + 'signal/bad_18.sd',
> + 'signal/bad_19.sd',
> + 'signal/bad_20.sd',
> + 'signal/bad_21.sd',
> + 'unix/bad_attr_1.sd',
> + 'unix/bad_attr_2.sd',
> + 'unix/bad_attr_3.sd',
> + 'unix/bad_attr_4.sd',
> + 'unix/bad_bind_1.sd',
> + 'unix/bad_bind_2.sd',
> + 'unix/bad_create_1.sd',
> + 'unix/bad_create_2.sd',
> + 'unix/bad_listen_1.sd',
> + 'unix/bad_listen_2.sd',
> + 'unix/bad_modifier_1.sd',
> + 'unix/bad_modifier_2.sd',
> + 'unix/bad_modifier_3.sd',
> + 'unix/bad_modifier_4.sd',
> + 'unix/bad_opt_1.sd',
> + 'unix/bad_opt_2.sd',
> + 'unix/bad_opt_3.sd',
> + 'unix/bad_opt_4.sd',
> + 'unix/bad_peer_1.sd',
> + 'unix/bad_regex_01.sd',
> + 'unix/bad_regex_02.sd',
> + 'unix/bad_regex_03.sd',
> + 'unix/bad_regex_04.sd',
> + 'unix/bad_shutdown_1.sd',
> + 'unix/bad_shutdown_2.sd',
> + 'vars/boolean/boolean_bad_1.sd',
> + 'vars/boolean/boolean_bad_2.sd',
> + 'vars/boolean/boolean_bad_3.sd',
> + 'vars/boolean/boolean_bad_4.sd',
> + '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',
> + 'vars/vars_bad_7.sd',
> + 'vars/vars_bad_8.sd',
> + 'vars/vars_bad_trailing_comma_1.sd',
> + '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',
> + 'vars/vars_dbus_bad_04.sd',
> + 'vars/vars_dbus_bad_05.sd',
> + 'vars/vars_dbus_bad_06.sd',
> + 'vars/vars_dbus_bad_07.sd',
> + 'vars/vars_file_evaluation_7.sd',
> + 'vars/vars_file_evaluation_8.sd',
> + 'vars/vars_recursion_1.sd',
> + 'vars/vars_recursion_2.sd',
> + 'vars/vars_recursion_3.sd',
> + 'vars/vars_recursion_4.sd',
> + 'vars/vars_simple_assignment_10.sd',
> + 'vars/vars_simple_assignment_3.sd',
> + 'vars/vars_simple_assignment_8.sd',
> + 'vars/vars_simple_assignment_9.sd',
> + 'xtrans/simple_bad_conflicting_x_10.sd',
> + 'xtrans/simple_bad_conflicting_x_11.sd',
> + 'xtrans/simple_bad_conflicting_x_12.sd',
> + 'xtrans/simple_bad_conflicting_x_13.sd',
> + 'xtrans/simple_bad_conflicting_x_14.sd',
> + 'xtrans/simple_bad_conflicting_x_15.sd',
> + 'xtrans/simple_bad_conflicting_x_1.sd',
> + 'xtrans/simple_bad_conflicting_x_2.sd',
> + 'xtrans/simple_bad_conflicting_x_3.sd',
> + 'xtrans/simple_bad_conflicting_x_4.sd',
> + 'xtrans/simple_bad_conflicting_x_5.sd',
> + 'xtrans/simple_bad_conflicting_x_6.sd',
> + 'xtrans/simple_bad_conflicting_x_7.sd',
> + 'xtrans/simple_bad_conflicting_x_8.sd',
> + 'xtrans/simple_bad_conflicting_x_9.sd',
> + 'xtrans/x-conflict.sd',
> +]
> +
> +# testcases with lines that don't match any regex and end up as "unknown line"
> +unknown_line = [
> + # 'other' keyword
> + 'file/allow/ok_other_1.sd',
> + 'file/allow/ok_other_2.sd',
> + 'file/ok_other_1.sd',
> + 'file/ok_other_2.sd',
> + 'file/ok_other_3.sd',
> +
> + # permissions before path
> + 'file/file/front_perms_ok_1.sd',
> + 'file/front_perms_ok_1.sd',
> + 'profile/local/local_named_profile_ok1.sd',
> + 'profile/local/local_profile_ok1.sd',
> + 'xtrans/simple_ok_cx_1.sd',
> +
> + # permissions before path and owner / audit {...} blocks
> + 'file/file/owner/ok_1.sd',
> + 'file/owner/ok_1.sd',
> + 'profile/entry_mods_audit_ok1.sd',
> +
> + # namespace
> + 'profile/profile_ns_named_ok1.sd', # profile keyword?
> + 'profile/profile_ns_named_ok2.sd', # profile keyword?
> + 'profile/profile_ns_named_ok3.sd', # profile keyword?
> + 'profile/profile_ns_ok1.sd',
> + 'profile/profile_ns_ok2.sd',
> + 'profile/profile_ns_ok3.sd', # profile keyword?
> + 'profile/re_named_ok4.sd', # profile keyword
> + 'profile/re_ok4.sd',
> +]
> +
> +# testcases with various unexpected failures
> +syntax_failure = [
> + # profile keyword?
> + 'profile/re_named_ok2.sd',
> +
> + # Syntax Error: Unexpected hat definition found (external hat)
> + 'change_hat/new_style4.sd',
> +
> + # Syntax Errors caused by boolean conditions (parse_profile_data() gets confused by the closing '}')
> + 'conditional/defined_1.sd',
> + 'conditional/defined_2.sd',
> + 'conditional/else_1.sd',
> + 'conditional/else_2.sd',
> + 'conditional/else_3.sd',
> + 'conditional/else_if_1.sd',
> + 'conditional/else_if_2.sd',
> + 'conditional/else_if_3.sd',
> + 'conditional/else_if_5.sd',
> + 'conditional/ok_1.sd',
> + 'conditional/ok_2.sd',
> + 'conditional/ok_3.sd',
> + 'conditional/ok_4.sd',
> + 'conditional/ok_5.sd',
> + 'conditional/ok_6.sd',
> + 'conditional/ok_7.sd',
> + 'conditional/ok_8.sd',
> + 'conditional/ok_9.sd',
> + 'conditional/stress_1.sd',
> +
> + # unexpected uppercase vs. lowercase in *x rules
> + 'file/ok_5.sd', # Invalid mode UX
> + 'file/ok_2.sd', # Invalid mode RWM
> + 'file/ok_4.sd', # Invalid mode iX
> + 'xtrans/simple_ok_pix_1.sd', # Invalid mode pIx
> + 'xtrans/simple_ok_pux_1.sd', # Invalid mode rPux
> +
> + # misc
> + 'rlimits/test1.sd', # Ambiguous value 60m in rlimit rttime rule XXX needs testcase fix
> + 'vars/vars_simple_assignment_12.sd', # Redefining existing variable @{BAR} ('\' not handled)
> + 'rewrite/alias_good_5.sd', # Values added to a non-existing variable @{FOO} (defined in include, lp:1331856)
> +]
> +
> +class TestParseParserTests(AATest):
> + tests = [] # filled by parse_test_profiles()
> +
> + def _run_test(self, params, expected):
> + with open_file_read(params['file']) as f_in:
> + data = f_in.readlines()
> +
> + if params['disabled']:
> + # skip disabled testcases
> + return
> +
> + if params['tools_wrong']:
> + # if the tools are marked as being wrong about a profile, expect the opposite result
> + # this makes sure we notice any behaviour change, especially not being wrong anymore
> + expected = not expected
> +
> + if expected:
> + apparmor.parse_profile_data(data, params['file'], 0)
> + else:
> + with self.assertRaises(AppArmorException):
> + apparmor.parse_profile_data(data, params['file'], 0)
> +
> +def parse_test_profiles(file_with_path):
> + '''parse the test-related headers of a profile (for example EXRESULT) and add the profile to the set of tests'''
> + exresult = None
> + exresult_found = False
> + description = None
> + todo = False
> + disabled = False
> +
> + with open_file_read(file_with_path) as f_in:
> + data = f_in.readlines()
> +
> + relfile = os.path.relpath(file_with_path, apparmor.profile_dir)
> +
> + for line in data:
> + if line.startswith('#=EXRESULT '):
> + exresult = line.split()[1]
> + if exresult == 'PASS':
> + exresult == True
> + exresult_found = True
> + elif exresult == 'FAIL':
> + exresult = False
> + exresult_found = True
> + else:
> + raise Exception('%s contains unknown EXRESULT %s' % (file_with_path, exresult))
> +
> + elif line.upper().startswith('#=DESCRIPTION '):
> + description = line.split()[1]
> +
> + elif line.rstrip() == '#=TODO':
> + todo = True
> +
> + elif line.rstrip() == '#=DISABLED':
> + disabled = True
> +
> + if not exresult_found:
> + raise Exception('%s does not contain EXRESULT' % file_with_path)
> +
> + if not description:
> + raise Exception('%s does not contain description' % file_with_path)
> +
> + tools_wrong = False
> + if relfile in exception_not_raised and not exresult:
> + if exresult:
> + raise Exception("%s listed in exception_not_raised, but has EXRESULT PASS" % file_with_path)
> + tools_wrong = 'EXCEPTION_NOT_RAISED'
> + elif relfile in unknown_line:
> + if not exresult:
> + raise Exception("%s listed in unknown_line, but has EXRESULT FAIL" % file_with_path)
> + tools_wrong = 'UNKNOWN_LINE'
> + elif relfile in syntax_failure:
> + if not exresult:
> + raise Exception("%s listed in syntax_failure, but has EXRESULT FAIL" % file_with_path)
> + tools_wrong = 'SYNTAX_FAILURE'
> +
> + params = {
> + 'file': file_with_path,
> + 'relfile': relfile,
> + 'todo': todo,
> + 'disabled': disabled,
> + 'tools_wrong': tools_wrong,
> + 'exresult': exresult,
> + }
> +
> + TestParseParserTests.tests.append((params, exresult))
> +
> +
> +def find_and_setup_test_profiles(profile_dir):
> + '''find all profiles in the given profile_dir, excluding
> + - skippable files
> + - include directories
> + - files in the main directory (readme, todo etc.)
> + '''
> + profile_dir = os.path.abspath(profile_dir)
> +
> + apparmor.profile_dir = profile_dir
> +
> + for root, dirs, files in os.walk(profile_dir):
> + relpath = os.path.relpath(root, profile_dir)
> +
> + if relpath == '.':
> + # include files are checked as part of the profiles that include them (also, they don't contain EXRESULT)
> + dirs.remove('includes')
> + dirs.remove('include_tests')
> + dirs.remove('includes-preamble')
> +
> + for file in files:
> + file_with_path = os.path.join(root, file)
> + if not apparmor.is_skippable_file(file) and relpath != '.':
> + parse_test_profiles(file_with_path)
> +
> +
> +find_and_setup_test_profiles('../../parser/tst/simple_tests/')
> +
> +setup_all_loops(__name__)
> +if __name__ == '__main__':
> + unittest.main(verbosity=2)
>
>
>
> Regards,
>
> Christian Boltz
>
> PS: Non-random sig - I already moved my laptop to a not-so-hot [1] room
>
> [1] I have to avoid the word "cooler" because it is definitively _not_
> cool here.
>
More information about the AppArmor
mailing list