Discussion:
Question about audit_filter_rules
Ondrej Mosnacek
2018-05-16 06:57:09 UTC
Permalink
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 <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
Ondrej Mosnacek
2018-05-16 08:43:12 UTC
Permalink
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
Post by Ondrej Mosnacek
Hi,
I noticed this suspicious line in the definition of the
[...]
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 <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
Richard Guy Briggs
2018-05-16 11:46:35 UTC
Permalink
Post by Ondrej Mosnacek
[...]
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;
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.
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...
Post by Ondrej Mosnacek
Post by Ondrej Mosnacek
Hi,
I noticed this suspicious line in the definition of the
[...]
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 <***@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
2018-05-16 12:27:57 UTC
Permalink
Post by Richard Guy Briggs
Post by Ondrej Mosnacek
[...]
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;
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.
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
Post by Richard Guy Briggs
Post by Ondrej Mosnacek
Post by Ondrej Mosnacek
Hi,
I noticed this suspicious line in the definition of the
[...]
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
--
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.
Richard Guy Briggs
2018-05-16 11:37:36 UTC
Permalink
Post by Ondrej Mosnacek
Hi,
I noticed this suspicious line in the definition of the
[...]
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.
Post by Ondrej Mosnacek
Ondrej Mosnacek <omosnace at redhat dot com>
- RGB

--
Richard Guy Briggs <***@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
Loading...