Re: Question about audit_filter_rules

2018-05-16 Thread Ondrej Mosnacek
2018-05-16 13:46 GMT+02:00 Richard Guy Briggs :
> 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 :
>> > 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 
>>
>> Ondrej Mosnacek 
>
> - RGB
>
> --
> Richard Guy Briggs 
> 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 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


Re: Question about audit_filter_rules

2018-05-16 Thread Richard Guy Briggs
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...

> 2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek :
> > 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 
> 
> Ondrej Mosnacek 

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


Re: Question about audit_filter_rules

2018-05-16 Thread Richard Guy Briggs
On 2018-05-16 08:57, Ondrej Mosnacek wrote:
> 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?

I'd agree you've found a bug.  I can trace it to my 2016-11-20
commit 8fae47705685fcaa75a1fe4c8c3e18300a702979
("audit: add support for session ID user filter")

It appears it should in fact be tsk rather than current.

> Ondrej Mosnacek 

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


Re: Question about audit_filter_rules

2018-05-16 Thread Ondrej Mosnacek
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

2018-05-16 8:57 GMT+02:00 Ondrej Mosnacek :
> 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?
>
> Thanks,
> --
> Ondrej Mosnacek 
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.



-- 
Ondrej Mosnacek 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit