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

2018-05-17 19:07 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: > 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(a)vger.kernel.org >> Signed-off-by: Ondrej Mosnacek <omosnace(a)redhat.com&gt; > > 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.

> 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(a)redhat.com&gt; > 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.