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

[John Johansen]

On 06/01/2016 09:50 AM, Christian Boltz wrote:
> 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].
> 
yes, that test was wrong
  change_profile, == change_profile -> **,
but it does not equal
  change_profile /**, which is the same as change_profile /** -> **,

the 2nd set is a subset of the first

>> --- /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 ;-)
> 
sure

> --- /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:
>
yep
> --- /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.
> 
> 
ok