From 0ec6ce96d2b2a6894c3d2d4dd9f76f4a62e5555b Mon Sep 17 00:00:00 2001 From: Steve Beattie Date: Mon, 23 Mar 2015 12:43:57 -0700 Subject: [PATCH] parser: fix compilation failure of deny link rules, expand equality tests Merge from trunk commits 2909, 2910, 2911, and 2912 BugLink: http://bugs.launchpad.net/bugs/1433829 The apparmor_parser fails to compile deny rules with only link permissions. Eg. deny /f l, deny l /f, deny link /f -> /d, Will all fail to compile with the following assert apparmor_parser: aare_rules.cc:99: Node* convert_file_perms(int, uint32_t, uint32_t, bool): Assertion `perms != 0' failed. NOTE: this is a minimal patch a bigger patch that cleans-up and separates and reorganizes file, link, exec, and change_profile rules is needed parser: Expand Equality tests This adds several new equality tests and turned up a couple of more bugs https://launchpad.net/bugs/1433829 https://launchpad.net/bugs/1434018 - add link/link subset tests - add pix, Pix, cix, Cix, pux, Pux, cux, Cux and specified profile transitions (/f px -> b ...) - test equality of leading and trailing permission file rules ie. /foo rw, == rw /foo, - test that specific x match overrides generic x rule. ie. /** ix, /foo px, is different than /** ix, /foo ix, - test that deny removes permission /f[abc] r, deny /fb r, is differnt than /f[abc] r, In addition to adding the new tests, it changes the output of the equality tests, so that if the $verbose variable is not set successful tests only output a period, with failed tests outputing the full info. If verbose is set the full test info is output as before. It also does: - make the verbose output of equality.sh honor whether or not the environment variable VERBOSE is set - thereby making the output verbose when 'make check V=1' or 'make check VERBOSE=1' is given from within the parser/ directory. This will make distribution packagers happy when diagnosing build failures caused by test failures. - if verbose output is not emitted and the tests were successful, emit a newline before printing PASS. - verify audit and audit allow is equal - verify audit differs from deny and audit deny - verify deny differs from audit deny - make the verbose text a little more useful for some cases - correct overlap exec tests to substitute in looped perms Signed-off-by: John Johansen Signed-off-by: Steve Beattie Acked-by: Seth Arnold --- parser/parser_regex.c | 9 +- parser/tst/Makefile | 2 + parser/tst/equality.sh | 177 +++++++++++++++++- .../simple_tests/file/ok_audit_deny_link.sd | 9 + parser/tst/simple_tests/file/ok_deny_link.sd | 9 + 5 files changed, 195 insertions(+), 11 deletions(-) create mode 100644 parser/tst/simple_tests/file/ok_audit_deny_link.sd create mode 100644 parser/tst/simple_tests/file/ok_deny_link.sd diff --git a/parser/parser_regex.c b/parser/parser_regex.c index 77ed6c913..b1d213295 100644 --- a/parser/parser_regex.c +++ b/parser/parser_regex.c @@ -491,9 +491,14 @@ static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry) * out by a deny rule, as both pieces of the link pair must * match. audit info for the link is carried on the second * entry of the pair + * + * So if a deny rule only record it if there are permissions other + * than link in the entry. + * TODO: split link and change_profile entries earlier */ - if (entry->deny && (entry->mode & AA_LINK_BITS)) { - if (!dfarules->add_rule(tbuf.c_str(), entry->deny, + if (entry->deny) { + if ((entry->mode & ~(AA_LINK_BITS | AA_CHANGE_PROFILE)) && + !dfarules->add_rule(tbuf.c_str(), entry->deny, entry->mode & ~AA_LINK_BITS, entry->audit & ~AA_LINK_BITS, dfaflags)) return FALSE; diff --git a/parser/tst/Makefile b/parser/tst/Makefile index d254809d3..76609d23e 100644 --- a/parser/tst/Makefile +++ b/parser/tst/Makefile @@ -9,6 +9,8 @@ PROVE_ARG=-f ifeq ($(VERBOSE),1) PROVE_ARG+=-v PYTEST_ARG = -v +else + undefine VERBOSE endif all: tests diff --git a/parser/tst/equality.sh b/parser/tst/equality.sh index 3082dbf59..89a048eb9 100755 --- a/parser/tst/equality.sh +++ b/parser/tst/equality.sh @@ -27,6 +27,7 @@ _SCRIPTDIR=$(dirname "${BASH_SOURCE[0]}" ) APPARMOR_PARSER="${APPARMOR_PARSER:-${_SCRIPTDIR}/../apparmor_parser}" fails=0 errors=0 +verbose="${VERBOSE:-}" hash_binary_policy() { @@ -61,10 +62,11 @@ verify_binary() return $((ret + 1)) fi - printf "Binary %s %s" "$t" "$desc" + if [ -n "$verbose" ] ; then printf "Binary %s %s" "$t" "$desc" ; fi good_hash=$(hash_binary_policy "$good_profile") if [ $? -ne 0 ] then + if [ -z "$verbose" ] ; then printf "Binary %s %s" "$t" "$desc" ; fi printf "\nERROR: Error hashing the following \"known-good\" profile:\n%s\n\n" \ "$good_profile" 1>&2 ((errors++)) @@ -76,12 +78,14 @@ verify_binary() hash=$(hash_binary_policy "$profile") if [ $? -ne 0 ] then + if [ -z "$verbose" ] ; then printf "Binary %s %s" "$t" "$desc" ; fi printf "\nERROR: Error hashing the following profile:\n%s\n\n" \ "$profile" 1>&2 ((errors++)) ((ret++)) elif [ "$t" == "equality" ] && [ "$hash" != "$good_hash" ] then + if [ -z "$verbose" ] ; then printf "Binary %s %s" "$t" "$desc" ; fi printf "\nFAIL: Hash values do not match\n" 2>&1 printf "known-good (%s) != profile-under-test (%s) for the following profile:\n%s\n\n" \ "$good_hash" "$hash" "$profile" 1>&2 @@ -89,6 +93,7 @@ verify_binary() ((ret++)) elif [ "$t" == "inequality" ] && [ "$hash" == "$good_hash" ] then + if [ -z "$verbose" ] ; then printf "Binary %s %s" "$t" "$desc" ; fi printf "\nFAIL: Hash values match\n" 2>&1 printf "known-good (%s) == profile-under-test (%s) for the following profile:\n%s\n\n" \ "$good_hash" "$hash" "$profile" 1>&2 @@ -99,9 +104,13 @@ verify_binary() if [ $ret -eq 0 ] then - printf " ok\n" - fi + if [ -z "$verbose" ] ; then + printf "." + else + printf " ok\n" + fi + fi return $ret } @@ -115,6 +124,8 @@ verify_binary_inequality() verify_binary "inequality" "$@" } +printf "Equality Tests:\n" + verify_binary_equality "dbus send" \ "/t { dbus send, }" \ "/t { dbus write, }" \ @@ -255,6 +266,7 @@ verify_binary_equality "dbus minimization found in dbus abstractions" \ dbus send bus=session, }" # Rules compatible with audit, deny, and audit deny +# note: change_profile does not support audit/allow/deny atm for rule in "capability" "capability mac_admin" \ "network" "network tcp" "network inet6 tcp"\ "mount" "mount /a" "mount /a -> /b" "mount options in (ro) /a -> b" \ @@ -270,41 +282,188 @@ for rule in "capability" "capability mac_admin" \ "unix" "unix (create, listen, accept)" "unix addr=@*" "unix addr=none" \ "unix peer=(label=foo)" \ "/f r" "/f w" "/f rwmlk" "/** r" "/**/ w" \ - "file /f r" "file /f w" "file /f rwmlk" + "file /f r" "file /f w" "file /f rwmlk" \ + "link /a -> /b" "link subset /a -> /b" \ + "l /a -> /b" "l subset /a -> /b" \ + "file l /a -> /b" "l subset /a -> /b" do verify_binary_equality "allow modifier for \"${rule}\"" \ "/t { ${rule}, }" \ "/t { allow ${rule}, }" + verify_binary_equality "audit allow modifier for \"${rule}\"" \ + "/t { audit ${rule}, }" \ + "/t { audit allow ${rule}, }" + verify_binary_inequality "audit, deny, and audit deny modifiers for \"${rule}\"" \ "/t { ${rule}, }" \ "/t { audit ${rule}, }" \ "/t { audit allow ${rule}, }" \ "/t { deny ${rule}, }" \ "/t { audit deny ${rule}, }" + + verify_binary_inequality "audit vs deny and audit deny modifiers for \"${rule}\"" \ + "/t { audit ${rule}, }" \ + "/t { deny ${rule}, }" \ + "/t { audit deny ${rule}, }" + + verify_binary_inequality "deny and audit deny modifiers for \"${rule}\"" \ + "/t { deny ${rule}, }" \ + "/t { audit deny ${rule}, }" done # Rules that need special treatment for the deny modifier -for rule in "/f ux" "/f Ux" "/f px" "/f Px" "/f ix" \ - "file /f ux" "file /f UX" "file /f px" "file /f Px" "file /f ix" +for rule in "/f ux" "/f Ux" "/f px" "/f Px" "/f cx" "/f Cx" "/f ix" \ + "/f pux" "/f Pux" "/f pix" "/f Pix" \ + "/f cux" "/f Cux" "/f cix" "/f Cix" \ + "/* ux" "/* Ux" "/* px" "/* Px" "/* cx" "/* Cx" "/* ix" \ + "/* pux" "/* Pux" "/* pix" "/* Pix" \ + "/* cux" "/* Cux" "/* cix" "/* Cix" \ + "/f px -> b " "/f Px -> b" "/f cx -> b" "/f Cx -> b" \ + "/f pux -> b" "/f Pux -> b" "/f pix -> b" "/f Pix -> b" \ + "/f cux -> b" "/f Cux -> b" "/f cix -> b" "/f Cix -> b" \ + "/* px -> b" "/* Px -> b" "/* cx -> b" "/* Cx -> b" \ + "/* pux -> b" "/* Pux -> b" "/* pix -> b" "/* Pix -> b" \ + "/* cux -> b" "/* Cux -> b" "/* cix -> b" "/* Cix -> b" \ + "file /f ux" "file /f Ux" "file /f px" "file /f Px" \ + "file /f cx" "file /f Cx" "file /f ix" \ + "file /f pux" "file /f Pux" "file /f pix" "file /f Pix" \ + "/f cux" "/f Cux" "/f cix" "/f Cix" \ + "file /* ux" "file /* Ux" "file /* px" "file /* Px" \ + "file /* cx" "file /* Cx" "file /* ix" \ + "file /* pux" "file /* Pux" "file /* pix" "file /* Pix" \ + "file /* cux" "file /* Cux" "file /* cix" "file /* Cix" \ + "file /f px -> b " "file /f Px -> b" "file /f cx -> b" "file /f Cx -> b" \ + "file /f pux -> b" "file /f Pux -> b" "file /f pix -> b" "file /f Pix -> b" \ + "file /f cux -> b" "file /f Cux -> b" "file /f cix -> b" "file /f Cix -> b" \ + "file /* px -> b" "file /* Px -> b" "file /* cx -> b" "file /* Cx -> b" \ + "file /* pux -> b" "file /* Pux -> b" "file /* pix -> b" "file /* Pix -> b" \ + "file /* cux -> b" "file /* Cux -> b" "file /* cix -> b" "file /* Cix -> b" + do verify_binary_equality "allow modifier for \"${rule}\"" \ "/t { ${rule}, }" \ - "/t { allow ${rule}, }" \ + "/t { allow ${rule}, }" + + verify_binary_equality "audit allow modifier for \"${rule}\"" \ + "/t { audit ${rule}, }" \ + "/t { audit allow ${rule}, }" + + # skip rules that don't end with x perm + if [ -n "${rule##*x}" ] ; then continue ; fi verify_binary_inequality "deny, audit deny modifier for \"${rule}\"" \ "/t { ${rule}, }" \ "/t { audit ${rule}, }" \ "/t { audit allow ${rule}, }" \ - "/t { deny /f x, }" \ - "/t { audit deny /f x, }" + "/t { deny ${rule% *} x, }" \ + "/t { audit deny ${rule% *} x, }" + + verify_binary_inequality "audit vs deny and audit deny modifiers for \"${rule}\"" \ + "/t { audit ${rule}, }" \ + "/t { deny ${rule% *} x, }" \ + "/t { audit deny ${rule% *} x, }" + done +# verify deny and audit deny differ for x perms +for prefix in "/f" "/*" "file /f" "file /*" ; do + verify_binary_inequality "deny and audit deny x modifiers for \"${prefix}\"" \ + "/t { deny ${prefix} x, }" \ + "/t { audit deny ${prefix} x, }" +done + +#Test equality of leading and trailing file permissions +for audit in "" "audit" ; do + for allow in "" "allow" "deny" ; do + for owner in "" "owner" ; do + for f in "" "file" ; do + prefix="$audit $allow $owner $f" + for perm in "r" "w" "a" "l" "k" "m" "rw" "ra" \ + "rl" "rk" "rm" "wl" "wk" "wm" \ + "rwl" "rwk" "rwm" "ral" "rak" \ + "ram" "rlk" "rlm" "rkm" "wlk" \ + "wlm" "wkm" "alk" "alm" "akm" \ + "lkm" "rwlk" "rwlm" "rwkm" \ + "ralk" "ralm" "wlkm" "alkm" \ + "rwlkm" "ralkm" ; do + verify_binary_equality "leading and trailing perms for \"${perm}\"" \ + "/t { ${prefix} /f ${perm}, }" \ + "/t { ${prefix} ${perm} /f, }" + done + if [ "$allow" == "deny" ] ; then continue ; fi + for perm in "ux" "Ux" "px" "Px" "cx" "Cx" \ + "ix" "pux" "Pux" "pix" "Pix" \ + "cux" "Cux" "cix" "Cix" + do + verify_binary_equality "leading and trailing perms for \"${perm}\"" \ + "/t { ${prefix} /f ${perm}, }" \ + "/t { ${prefix} ${perm} /f, }" + done + for perm in "px" "Px" "cx" "Cx" \ + "pux" "Pux" "pix" "Pix" \ + "cux" "Cux" "cix" "Cix" + do + verify_binary_equality "leading and trailing perms for x-transition \"${perm}\"" \ + "/t { ${prefix} /f ${perm} -> b, }" \ + "/t { ${prefix} ${perm} /f -> b, }" + done + done + done + done +done + +#Test rule overlap for x most specific match +for perm1 in "ux" "Ux" "px" "Px" "cx" "Cx" "ix" "pux" "Pux" \ + "pix" "Pix" "cux" "Cux" "cix" "Cix" "px -> b" \ + "Px -> b" "cx -> b" "Cx -> b" "pux -> b" "Pux ->b" \ + "pix -> b" "Pix -> b" "cux -> b" "Cux -> b" \ + "cix -> b" "Cix -> b" +do + for perm2 in "ux" "Ux" "px" "Px" "cx" "Cx" "ix" "pux" "Pux" \ + "pix" "Pix" "cux" "Cux" "cix" "Cix" "px -> b" \ + "Px -> b" "cx -> b" "Cx -> b" "pux -> b" "Pux ->b" \ + "pix -> b" "Pix -> b" "cux -> b" "Cux -> b" \ + "cix -> b" "Cix -> b" + do + if [ "$perm1" == "$perm2" ] ; then + verify_binary_equality "Exec perm \"${perm1}\" - most specific match: same as glob" \ + "/t { /* ${perm1}, /f ${perm2}, }" \ + "/t { /* ${perm1}, }" + else + verify_binary_inequality "Exec \"${perm1}\" vs \"${perm2}\" - most specific match: different from glob" \ + "/t { /* ${perm1}, /f ${perm2}, }" \ + "/t { /* ${perm1}, }" + fi + done + verify_binary_inequality "Exec \"${perm1}\" vs deny x - most specific match: different from glob" \ + "/t { /* ${perm1}, audit deny /f x, }" \ + "/t { /* ${perm1}, }" + +done + +#Test deny carves out permission +verify_binary_inequality "Deny removes r perm" \ + "/t { /foo/[abc] r, audit deny /foo/b r, }" \ + "/t { /foo/[abc] r, }" + +verify_binary_equality "Deny removes r perm" \ + "/t { /foo/[abc] r, audit deny /foo/b r, }" \ + "/t { /foo/[ac] r, }" + +#this one may not be true in the future depending on if the compiled profile +#is explicitly including deny permissions for dynamic composition +verify_binary_equality "Deny of ungranted perm" \ + "/t { /foo/[abc] r, audit deny /foo/b w, }" \ + "/t { /foo/[abc] r, }" + + if [ $fails -ne 0 -o $errors -ne 0 ] then printf "ERRORS: %d\nFAILS: %d\n" $errors $fails 2>&1 exit $(($fails + $errors)) fi +[ -z "${verbose}" ] && printf "\n" printf "PASS\n" exit 0 diff --git a/parser/tst/simple_tests/file/ok_audit_deny_link.sd b/parser/tst/simple_tests/file/ok_audit_deny_link.sd new file mode 100644 index 000000000..393f9069d --- /dev/null +++ b/parser/tst/simple_tests/file/ok_audit_deny_link.sd @@ -0,0 +1,9 @@ +# +#=DESCRIPTION simple link access test +#=EXRESULT PASS +# + +profile test { + audit deny link /alpha/beta -> /tmp/**, +} + diff --git a/parser/tst/simple_tests/file/ok_deny_link.sd b/parser/tst/simple_tests/file/ok_deny_link.sd new file mode 100644 index 000000000..fe0684c04 --- /dev/null +++ b/parser/tst/simple_tests/file/ok_deny_link.sd @@ -0,0 +1,9 @@ +# +#=DESCRIPTION simple link access test +#=EXRESULT PASS +# + +profile test { + deny link /alpha/beta -> /tmp/**, +} +