[apparmor] [PATCH 04/11] Fix the x intersection consistency test
John Johansen
john.johansen at canonical.com
Thu Mar 8 18:30:30 UTC 2012
On 03/08/2012 09:41 AM, Steve Beattie wrote:
> On Wed, Mar 07, 2012 at 11:52:32AM -0800, John Johansen wrote:
>> On 03/07/2012 10:27 AM, Steve Beattie wrote:
>>> On Wed, Mar 07, 2012 at 06:17:23AM -0800, John Johansen wrote:
>>>> The in x intersection consistency test for minimization was failing because
>>>> it was screening off the AA_MAY_EXEC permission before passing the exec
>>>> information to the consistency test fn. This resulted in the consistency
>>>> test fn not testing the consistency because it treated the permission set
>>>> as not having x permissions.
>>>>
>>>> Signed-off-by: John Johansen <john.johansen at canonical.com>
>>>
>>> It *looks* sensible, but applying this patch plus the first 2 (but
>>> not the third, which doesn't apply for some reason) results in the
>>> parser testsuite having 11 new failures; without this patch, there's
>>> no failures. I have yet to look into why.
>>>
>> Yes it will, because this particular bug is part of the reason the next one
>> wasn't caught sooner. Fixing it turns up a lot of errors happening
>
> Hrm, okay.
>
> Acked-By: Steve Beattie <sbeattie at ubuntu.com>
>
> Sorry for the delay, but I got confused by the added testcase in
> this patch, in that it doesn't actually demonstrate the issue this
> patch is supposed to address. (There is no change in behavior of the
> parser for the testcase with or without the parser change in this patch
> applied. It's a fine testcase to have, assuming it's not overlapping
> with another testcase we have.) Do you have such a testcase?
>
>> In fact you won't get the complete test suite passing until the very last
>> patch.
>>
No, nor will I because once the partitioning is done, the is_mered_x_consistent
test in minimization should never fail. I left it in for now, as its worth
having the extra checking but some time in the future it will go away.
So where the is_merged_x_consistent test is needed is in the DFA construction
specifically in new State where it calls accept_perms. This is where all
the permissions for a given state get accumulated, and any consistency problems
should show up, because at the end of DFA construction we have a valid DFA
In minimization we are trying to eliminate states, by throwing away unused
information, mostly by merging down alternate paths that split early but have
compatible tails etc. In traditional DFA minimization you only have one accept
state so you start with two partitions (accepting and non-accepting) and then
do an iterative splitting of those partitions as you find states that are not
the same (eg. both have transitions on x but one is to an none accepting state
and the other to an accepting, basically if they go to different partitions).
The mistake I made with full minimization was starting with just two partitions,
as in traditional minimization. We don't want to do this as we really have
more than 1 accept state.
Eg.
/a r,
/b w,
In our dfa this would result in 4 states, with the final transition state for 'a'
being the 'r' accept state and the final transition for 'b' being the 'w' accept
state.
We could minimize this down by combining the 'r' and 'w' accept states into a
single one merging their permissions into an 'rw' set but that would be wrong
as now matching /a would have 'rw' permissions, and /b would also have 'rw'
permissions.
We can avoid this happening by starting out with more partitions. One for each
permission set. With one difference we do allow accumulation of deny information
within a partion, and we don't setup partitions based on deny info.
Any initial overlap for deny (say /** /a) is taken care of in DFA construction
Anyways, basically once we are done with with fixing minimization there is
no permission accumulation done in minimization. So we can't create a test
case for this patch, or at least if we did here the next patch would have to
remove it.
>>>> ---
>>>> parser/immunix.h | 3 +++
>>>> parser/libapparmor_re/hfa.h | 8 ++++----
>>>> parser/tst/simple_tests/xtrans/x-conflict.sd | 11 +++++++++++
>>>> 3 files changed, 18 insertions(+), 4 deletions(-)
>>>> create mode 100644 parser/tst/simple_tests/xtrans/x-conflict.sd
>>>>
>>>> diff --git a/parser/immunix.h b/parser/immunix.h
>>>> index 72446fc..8dc157a 100644
>>>> --- a/parser/immunix.h
>>>> +++ b/parser/immunix.h
>>>> @@ -96,6 +96,9 @@
>>>>
>>>> #define ALL_AA_EXEC_TYPE (AA_USER_EXEC_TYPE | AA_OTHER_EXEC_TYPE)
>>>>
>>>> +#define ALL_USER_EXEC (AA_USER_EXEC | AA_USER_EXEC_TYPE)
>>>> +#define ALL_OTHER_EXEC (AA_OTHER_EXEC | AA_OTHER_EXEC_TYPE)
>>>> +
>>>> #define AA_LINK_BITS ((AA_MAY_LINK << AA_USER_SHIFT) | \
>>>> (AA_MAY_LINK << AA_OTHER_SHIFT))
>>>>
>>>> diff --git a/parser/libapparmor_re/hfa.h b/parser/libapparmor_re/hfa.h
>>>> index c77905d..a35becb 100644
>>>> --- a/parser/libapparmor_re/hfa.h
>>>> +++ b/parser/libapparmor_re/hfa.h
>>>> @@ -50,8 +50,8 @@ public:
>>>> {
>>>> deny |= rhs.deny;
>>>>
>>>> - if (!is_merged_x_consistent(allow & AA_USER_EXEC_TYPE,
>>>> - rhs.allow & AA_USER_EXEC_TYPE)) {
>>>> + if (!is_merged_x_consistent(allow & ALL_USER_EXEC,
>>>> + rhs.allow & ALL_USER_EXEC)) {
>>>> if ((exact & AA_USER_EXEC_TYPE) &&
>>>> !(rhs.exact & AA_USER_EXEC_TYPE)) {
>>>> /* do nothing */
>>>> @@ -64,8 +64,8 @@ public:
>>>> } else
>>>> allow |= rhs.allow & AA_USER_EXEC_TYPE;
>>>>
>>>> - if (!is_merged_x_consistent(allow & AA_OTHER_EXEC_TYPE,
>>>> - rhs.allow & AA_OTHER_EXEC_TYPE)) {
>>>> + if (!is_merged_x_consistent(allow & ALL_OTHER_EXEC,
>>>> + rhs.allow & ALL_OTHER_EXEC)) {
>>>> if ((exact & AA_OTHER_EXEC_TYPE) &&
>>>> !(rhs.exact & AA_OTHER_EXEC_TYPE)) {
>>>> /* do nothing */
>>>> diff --git a/parser/tst/simple_tests/xtrans/x-conflict.sd b/parser/tst/simple_tests/xtrans/x-conflict.sd
>>>> new file mode 100644
>>>> index 0000000..92215f9
>>>> --- /dev/null
>>>> +++ b/parser/tst/simple_tests/xtrans/x-conflict.sd
>>>> @@ -0,0 +1,11 @@
>>>> +#
>>>> +#=DESCRIPTION test for conflict resolution in minimization phase of dfa gen
>>>> +#=EXRESULT FAIL
>>>> +#=TODO
>>>> +#
>>>> +/usr/bin/foo {
>>>> + /b* px,
>>>> + /* Pixr,
>>>> + /a* Cx -> foo,
>>>> +}
>>>> +
>>>> --
>>>> 1.7.9
>>>>
>>>>
>>>> --
>>>> AppArmor mailing list
>>>> AppArmor at lists.ubuntu.com
>>>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>>>
>>>
>>>
>>
>>
>> --
>> AppArmor mailing list
>> AppArmor at lists.ubuntu.com
>> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>
>
>
More information about the AppArmor
mailing list