2018-05-22 21:24 GMT+02:00 Paul Moore <p...@paul-moore.com>:
> On Mon, May 21, 2018 at 7:47 AM, Ondrej Mosnacek <omosn...@redhat.com> wrote:
>> 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]).
>
> As you pointed out, we're stuck with the supplementary group checking
> since that change dates back to 2011. With that in mind the only real
> question is do we want to preserve the fsgid checking present in
> in_group_p(), even though it doesn't make sense for gid/egid? It's
> complicated by the fact that in_group_p() was checking fsgid back when
> 37eebe39c973 ("audit: improve GID/EGID comparation logic") was merged
> so it too has been that way for several years.
In fact, the in_group_p function has been checking against fsgid since
the in_group_p function has been split into in_group_p and in_egroup_p
in Linux 2.3.51pre3. You can see this in the historic Linux repository
[1]. At that point, in_group_p was used exclusively in the
filesystem/IPC context (only to check access to shared resources,
where FSGID is more appropriate than EGID/GID):
$ git checkout 2.3.51pre3
$ git grep in_group_p
fs/attr.c: (!in_group_p(attr->ia_gid) && attr->ia_gid !=
inode->i_gid) &&
fs/attr.c: if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
fs/attr.c: if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
fs/dquot.c: return in_group_p(dquot->dq_id) &&
!(dquot->dq_flags & flag);
fs/exec.c: if (!in_group_p(bprm->e_gid))
fs/ext2/balloc.c: !in_group_p (sb->u.ext2_sb.s_resgid)) &&
fs/namei.c: else if (in_group_p(inode->i_gid))
fs/proc/base.c: else if (in_group_p(inode->i_gid))
include/linux/sched.h:extern int in_group_p(gid_t);
ipc/util.c: else if (in_group_p(ipcp->cgid) || in_group_p(ipcp->gid))
kernel/ksyms.c:EXPORT_SYMBOL(in_group_p);
kernel/sys.c:int in_group_p(gid_t grp)
For comparison, in_egroup_p was used only in two special cases where
the actual task identity was relevant:
$ git grep in_egroup_p
fs/dquot.c: (type == GRPQUOTA && in_egroup_p(id))) &&
include/linux/sched.h:extern int in_egroup_p(gid_t);
kernel/sys.c:int in_egroup_p(gid_t grp)
kernel/sysctl.c: else if (in_egroup_p(0))
So from this I would conclude that these two functions are meant to be
used only when checking permissions -- in_group_p for checking access
to resources and in_egroup_p for checking the identity of the task.
Unfortunately, their naming doesn't communicate this very well, so the
author of the problematic commit used them incorrectly. Also note that
except of the very very rare case that FSGID != EGID these functions
do exactly the same thing, so we are effectively comparing both GID
and EGID under AUDIT_GID and only EGID under AUDIT_EGID, which makes
no sense. IMHO anyone who was able to discover this illogical behavior
AND did not report it as a bug AND now relies on it should suffer the
consequences of this fix anyway.
>
> What testing have you done on this change, and what is your motivation
> behind this patch? Was it simply something you noticed, or was it
> causing you problems?
I just ran the audit-testsuite on it (I finally manged to get the
remaining bits working :) and it passes (but that proves nothing,
since AFAIK the tests do not induce a situation where GID/EGID/FSGID
would differ).
I didn't cause any problems to me, but it will be a problem for anyone
who wants to match records for tasks that run with GID == x, but
doesn't want to match tasks with GID != x and EGID == x (there is no
way to do that right now and there is also no mention of this behavior
in the auditctl man page...).
The primary motivation for this patch is to fix the comparison to use
tsk instead of the current task. However, both bugs were caused by the
abuse of the in_[e]group functions, so it makes sense to fix them both
at once.
>
> This is definitely too risky to merge into audit/next at -rc6, it
> needs more time before it hits Linus' tree.
I agree, this has been broken for 6 years, another two months are not
going to make it much worse :)
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/kernel/sys.c?h=0.12&id=268175ce6feb5c4ad5987ee35606e0a2790f6a0c
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
