[PATCH ghak82] audit: Fix extended comparison of GID/EGID
Ondrej Mosnacek
omosnace at redhat.com
Mon May 21 11:47:09 UTC 2018
2018-05-18 16:28 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
> On 2018-05-18 09:44, Ondrej Mosnacek wrote:
>> 2018-05-17 19:07 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
>> > On 2018-05-17 17:31, Ondrej Mosnacek wrote:
>> >> The audit_filter_rules() function in auditsc.c used the in_[e]group_p()
>> >> functions to check GID/EGID match, but these functions use the current
>> >> task's credentials, while the comparison should use the credentials of
>> >> the task given to audit_filter_rules() as a parameter (tsk).
>> >>
>> >> Note that we can use group_search(cred->group_info, ...) as a
>> >> replacement for both in_group_p and in_egroup_p as these functions only
>> >> compare the parameter to cred->fsgid/egid and then call group_search.
>> >>
>> >> In fact, the usage of in_group_p was incorrect also because it compared
>> >> to cred->fsgid and not cred->gid.
>> >>
>> >> GitHub issue:
>> >> https://github.com/linux-audit/audit-kernel/issues/82
>> >>
>> >> Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")
>> >> Cc: stable at vger.kernel.org
>> >> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
>> >
>> > Nice. You found a function that let you not have to roll a new global
>> > one. Is the comparision with cred->fsgid important?
>>
>> To be honest, I don't really understand the exact purpose/meaning of
>> all the different *GIDs, but since we have a separate field for
>> comparing FSGID, I don't think it should be checked under GID.
>>
>> What concerns me a bit though is that the check for 'extra' groups is
>> now the same for GID and EGID... depending on the intended semantics
>> of these credential fields (or of the corresponding audit fields),
>> this may or may not be what we want. Maybe we should drop this
>> extended checking altogether, or maybe do the same check for FSGID, or
>> for a different subset of *GIDs... I will try to investigate this and
>> figure out what is the right thing to do here.
>
> fsgid may already be covered by another comparision function, but should
> it be included in this one to avoid changing the potentially intended
> coverage, or was it too broad to start with? I don't know the answer.
>
> I'd just add a check for fsgid too to be safe (or lazy) to avoid
> changing the behaviour, if not doing the background research to find out
> the intent. There could be users depending on existing behaviour.
OK, so after doing a bit of research I think this is how it is:
1. (Real) GID, effective GID, saved set-group-ID, FSGID and the list
of supplementary groups are each a separate concept and IMHO should be
each compared with a distinct field.
2. When checking access to a file, the FSGID and supplementary groups
are what matters.
3. There is a hierarchy GID -> EGID -> FSGID, in that changing one of
them changes the ones to right, but keeps the ones to the left (see
setgid(2) and setfsgid(2)).
4. The saved set-group-ID is Linux-specific and is only checked when a
process tries to change its real GID (so that it can regain the
effective group ID established at the last exec call [1]).
5. The FSGID is also Linux-specific and was only necessary in Linux
pre-2.0 in applications such as NFS where changing EGID would lead to
security issues. [2]
In an ideal world we should have fields: GID, EGID, FSGID, SGID, and
possibly something like SUPPLGID, which would be only used when
filtering events in the kernel and would not be emitted in records (or
it would contain the list of all supplementary groups of the process).
But, since we are not in and ideal world, we are now in a situation
where:
1. Due to commit 37eebe39c973, we filter also by supplementary groups
in both GID and EGID.
2. Due to the confusing naming/implementation/intent of the in_group_p
function we also check for FSGID under GID. This is IMHO completely
pointless (if anything, EGID is more related to FSGID than GID).
Anyway, it is very rare for a process to change its FSGID so this bug
(or its fix) should have very little to no impact in real life.
The supplementary groups checking OTOH, since this is a feature that
may easily significantly affect filtering (and has been there for over
6 years already), should be preserved for backwards compatibility.
All things said, I still believe the patch should be applied, but of
course it's up to Paul to decide. Either way it probably shouldn't go
to stable for the reasons Paul described in the other patch ([3]).
>> > If you run ./scripts/checkpatch.pl on the patch file it will complain on
>> > those lines longer than 80 chars. I don't have a problem with it.
>>
>> Yes, unfortunately it doesn't seem that splitting the lines would help
>> much here... I'll see if I can rewrite the conditions in a simpler
>> way.
I managed to find a bit nicer way to write it, so I will send an
updated v2 once Paul decides about the preferred solution.
[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/setuid.html
[2] http://man7.org/linux/man-pages/man2/setfsuid.2.html
[3] https://www.redhat.com/archives/linux-audit/2018-May/msg00095.html
>>
>> >
>> >
>> >> ---
>> >> kernel/auditsc.c | 8 ++++----
>> >> 1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> index cbab0da86d15..ec38e4d97c23 100644
>> >> --- a/kernel/auditsc.c
>> >> +++ b/kernel/auditsc.c
>> >> @@ -490,20 +490,20 @@ static int audit_filter_rules(struct task_struct *tsk,
>> >> result = audit_gid_comparator(cred->gid, f->op, f->gid);
>> >> if (f->op == Audit_equal) {
>> >> if (!result)
>> >> - result = in_group_p(f->gid);
>> >> + result = groups_search(cred->group_info, f->gid);
>> >> } else if (f->op == Audit_not_equal) {
>> >> if (result)
>> >> - result = !in_group_p(f->gid);
>> >> + result = !groups_search(cred->group_info, 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);
>> >> + result = groups_search(cred->group_info, f->gid);
>> >> } else if (f->op == Audit_not_equal) {
>> >> if (result)
>> >> - result = !in_egroup_p(f->gid);
>> >> + result = !groups_search(cred->group_info, f->gid);
>> >> }
>> >> break;
>> >> case AUDIT_SGID:
>> >> --
>> >> 2.17.0
>> >>
>> >
>> > - RGB
>> >
>> > --
>> > Richard Guy Briggs <rgb at 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.
>
> - RGB
>
> --
> Richard Guy Briggs <rgb at 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.
More information about the Linux-audit
mailing list