On Thu, May 31, 2018 at 3:19 AM, Ondrej Mosnacek <[email protected]> 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 even more incorrect: it compares to
> cred->fsgid (which is usually equal to cred->egid) and not cred->gid.
>
> GitHub issue:
> https://github.com/linux-audit/audit-kernel/issues/82
>
> Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
> kernel/auditsc.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c4596c51..3a324ca2fd20 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -492,23 +492,21 @@ static int audit_filter_rules(struct task_struct *tsk,
> break;
> case AUDIT_GID:
> result = audit_gid_comparator(cred->gid, f->op,
> f->gid);
> - if (f->op == Audit_equal) {
> - if (!result)
> - result = in_group_p(f->gid);
> - } else if (f->op == Audit_not_equal) {
> - if (result)
> - result = !in_group_p(f->gid);
> - }
> + if (f->op == Audit_equal)
> + result = result ||
> + groups_search(cred->group_info,
> f->gid);
Okay, so I get you're being clever, and everybody likes to be clever,
but to my eyes this is not an improvement in readability. From my
perspective the following pattern:
if (x)
x = y;
... is so much easier to read than the following pattern:
x = x || y;
Since we've got time during the merge window, please restore the
original if-conditional style. The same comment applies to the other
instances as well.
Other than that, everything else looks okay. I worry we might
surprise some users with this, but the current behavior is not
correct.
> + else if (f->op == Audit_not_equal)
> + result = 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);
> - } else if (f->op == Audit_not_equal) {
> - if (result)
> - result = !in_egroup_p(f->gid);
> - }
> + if (f->op == Audit_equal)
> + result = result ||
> + groups_search(cred->group_info,
> f->gid);
> + else if (f->op == Audit_not_equal)
> + result = result &&
> + !groups_search(cred->group_info,
> f->gid);
> break;
> case AUDIT_SGID:
> result = audit_gid_comparator(cred->sgid, f->op,
> f->gid);
> --
> 2.17.0
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
