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

[Ondrej Mosnacek]

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 <***@redhat.com>
---
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);
+ 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);

[Richard Guy Briggs]

Post by Ondrej Mosnacek
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.
https://github.com/linux-audit/audit-kernel/issues/82
Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")

Given what you've turned up while investigating this, I'm comfortable
with the solution you've presented.

Post by Ondrej Mosnacek
---
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;
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);
+ else if (f->op == Audit_not_equal)
+ result = result &&
+ !groups_search(cred->group_info, f->gid);
break;
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;
result = audit_gid_comparator(cred->sgid, f->op, f->gid);
--
2.17.0

- RGB

--
Richard Guy Briggs <***@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

[Paul Moore]

Post by Ondrej Mosnacek
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.
https://github.com/linux-audit/audit-kernel/issues/82
Fixes: 37eebe39c973 ("audit: improve GID/EGID comparation logic")
---
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;
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.

Post by Ondrej Mosnacek
+ else if (f->op == Audit_not_equal)
+ result = result &&
+ !groups_search(cred->group_info, f->gid);
break;
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;
result = audit_gid_comparator(cred->sgid, f->op, f->gid);
--
2.17.0