Re: Question about audit_filter_rules

[Ondrej Mosnacek <omosnace@redhat.com>]

2018-05-16 13:46 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-05-16 10:43, Ondrej Mosnacek wrote:
>> I found more inconsistencies:
>> [...]
>> case AUDIT_GID:
>>         result = audit_gid_comparator(cred->gid, f->op, f->gid);
>>         if (f->op == Audit_equal) {
>>                if (!result)
>>                        result = in_group_p(f->gid);
>>         } else if (f->op == Audit_not_equal) {
>>                 if (result)
>>                         result = !in_group_p(f->gid);
>>         }
>>         break;
>> case AUDIT_EGID:
>>         result = audit_gid_comparator(cred->egid, f->op, f->gid);
>>         if (f->op == Audit_equal) {
>>                 if (!result)
>>                         result = in_egroup_p(f->gid);
>>         } else if (f->op == Audit_not_equal) {
>>                if (result)
>>                         result = !in_egroup_p(f->gid);
>>         }
>>         break;
>> [...]
>>
>> The in_[e]group_p functions match the current task's group list.
>> Unfortunately there don't seem to be functions in the kernel that
>> would do the same for arbitrary struct cred pointers, we may need to
>> add these to fix this.
>>
>> See the definition of in_group_p for reference:
>> https://elixir.bootlin.com/linux/latest/source/kernel/groups.c#L219
>
> Interesting.  Nice catch.  I'll need to look at these and the previous
> one again.  File github audit kernel issues...

Done, created at:
https://github.com/linux-audit/audit-kernel/issues/82

>> 2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek <omosnace@redhat.com>:
>> > Hi,
>> >
>> > I noticed this suspicious line in the definition of the
>> > audit_filter_rules function in auditsc.c:
>> >
>> > [...]
>> > case AUDIT_SESSIONID:
>> >         sessionid = audit_get_sessionid(current);     // <--- HERE
>> >         result = audit_comparator(sessionid, f->op, f->val);
>> >         break;
>> > [...]
>> >
>> > Here, the sessionid is retrieved from the current task pointer, while
>> > all the other code in this function compares against the tsk task
>> > pointer. It seems that it is not always guaranteed that tsk ==
>> > current, so my question is: Is it intentional for some reason or
>> > should it be tsk instead of current?
>> >
>> > Ondrej Mosnacek <omosnace at redhat dot com>
>>
>> Ondrej Mosnacek <omosnace at redhat dot com>
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635



-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.