[apparmor] [PATCH] Fix: make sure overlapping safe and unsafe exec rules conflict

[Christian Boltz]

Hello,

it's nice to see that my question didn't need a terribly big patch ;-)

Some notes:

Am Dienstag, 31. Mai 2016, 23:07:58 CEST schrieb John Johansen:
> --- a/parser/tst/equality.sh
> +++ b/parser/tst/equality.sh
> @@ -461,9 +461,23 @@ verify_binary_equality "Deny of ungranted perm" \
> verify_binary_equality "change_profile == change_profile -> **" \ "/t
> { change_profile, }" \
>  		       "/t { change_profile -> **, }" \
> +
> +verify_binary_equality "change_profile /** == change_profile /** ->
> **" \ "/t { change_profile /**, }" \
>  		       "/t { change_profile /** -> **, }"

You are splitting the test (with 4 cases) into two tests with 2 cases 
each, but this also means that 1 and 2 aren't checked against 3 and 4 
anymore.

Is this intentional? If not, please add another test comparing [1 or 2] 
with [3 or 4].

> --- /dev/null
> +++ b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe1.sd
> @@ -0,0 +1,8 @@
> +#
> +#=DESCRIPTION test for conflict safe and unsafe exec condition
> +#=EXRESULT FAIL
> +#
> +/usr/bin/foo {
> +   change_profile /onexec -> /bin/foo,
> +   change_profile unsafe /onexec -> /bin/foo,
> +}
> diff --git
> a/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
> b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd new
> file mode 100644
> index 0000000..a6c583e
> --- /dev/null
> +++ b/parser/tst/simple_tests/change_profile/onx_conflict_unsafe2.sd
> @@ -0,0 +1,8 @@
> +#
> +#=DESCRIPTION test for conflict safe and unsafe exec condition
> +#=EXRESULT FAIL
> +#
> +/usr/bin/foo {
> +   change_profile safe /onexec -> /bin/foo,
> +   change_profile unsafe /onexec -> /bin/foo,
> +}

So you check for none vs. unsafe and safe vs. unsafe.

Can you add another testcase to check none vs. safe (EXRESULT PASS)?
I know this repeats a test from equality.sh, but adding it in 
simple_tests will make it visible for the utils tests too. (This isn't
a promise to add conflict handling to the tools, but having a failing
testcase definitively helps ;-)

--- /dev/null
+++ b/parser/tst/simple_tests/change_profile/onx_no_conflict_safe1.sd
@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION 'safe' and unspecified exec condition shouldn't conflict because 'safe' is the default
+#=EXRESULT PASS
+#
+/usr/bin/foo {
+   change_profile safe /onexec -> /bin/foo,
+   change_profile /onexec -> /bin/foo,
+}


Maybe another interesting test would be to have the same exec condition 
with two different targets, one safe, one unsafe:

--- /dev/null
+++ b/parser/tst/simple_tests/change_profile/onx_no_conflict_safe2.sd
@@ -0,0 +1,8 @@
+#
+#=DESCRIPTION 'safe' and 'unsafe' for the same exec condition, but different exec targets
+#=EXRESULT PASS
+#
+/usr/bin/foo {
+   change_profile safe /onexec -> /bin/foo,
+   change_profile unsafe /onexec -> /bin/bar,
+}

I'm not sure about the expected behaviour, but I'd say since 
change_profile allows to specify two different exec targets, it should 
also allow to specify one as safe and one as unsafe.

Feel free to commit these two patches together with your patch.


Regards,

Christian Boltz
-- 
> Wrong ;-)
Ah, diplomacy at its finest.
[> Christian Boltz and Steve Beattie in apparmor]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160601/06d8f0fd/attachment.pgp>