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

[Richard Guy Briggs]

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.

> > 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.
> 
> >
> >
> >> ---
> >>  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