[PATCH ghak82] audit: Fix extended comparison of GID/EGID

[Paul Moore]

On Mon, May 21, 2018 at 7:47 AM, Ondrej Mosnacek <omosnace at 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.

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?

This is definitely too risky to merge into audit/next at -rc6, it
needs more time before it hits Linus' tree.

-- 
paul moore
www.paul-moore.com