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

[Richard Guy Briggs]

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

Nice. You found a function that let you not have to roll a new global
one. Is the comparision with cred->fsgid important?

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.

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

[Ondrej Mosnacek]

Post by Richard Guy Briggs

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

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.

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

Post by Richard Guy Briggs
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.

I managed to find a bit nicer way to write it, so I will send an
updated v2 once Paul decides about the preferred solution.

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/setuid.html
[2] http://man7.org/linux/man-pages/man2/setfsuid.2.html
[3] https://www.redhat.com/archives/linux-audit/2018-May/msg00095.html

Post by Richard Guy Briggs

---
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;
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;
--
2.17.0

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

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek
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
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.

In fact, the in_group_p function has been checking against fsgid since
the in_group_p function has been split into in_group_p and in_egroup_p
in Linux 2.3.51pre3. You can see this in the historic Linux repository
[1]. At that point, in_group_p was used exclusively in the
filesystem/IPC context (only to check access to shared resources,
where FSGID is more appropriate than EGID/GID):

$ git checkout 2.3.51pre3
$ git grep in_group_p
fs/attr.c: (!in_group_p(attr->ia_gid) && attr->ia_gid !=
inode->i_gid) &&
fs/attr.c: if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
fs/attr.c: if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
fs/dquot.c: return in_group_p(dquot->dq_id) &&
!(dquot->dq_flags & flag);
fs/exec.c: if (!in_group_p(bprm->e_gid))
fs/ext2/balloc.c: !in_group_p (sb->u.ext2_sb.s_resgid)) &&
fs/namei.c: else if (in_group_p(inode->i_gid))
fs/proc/base.c: else if (in_group_p(inode->i_gid))
include/linux/sched.h:extern int in_group_p(gid_t);
ipc/util.c: else if (in_group_p(ipcp->cgid) || in_group_p(ipcp->gid))
kernel/ksyms.c:EXPORT_SYMBOL(in_group_p);
kernel/sys.c:int in_group_p(gid_t grp)

For comparison, in_egroup_p was used only in two special cases where
the actual task identity was relevant:

$ git grep in_egroup_p
fs/dquot.c: (type == GRPQUOTA && in_egroup_p(id))) &&
include/linux/sched.h:extern int in_egroup_p(gid_t);
kernel/sys.c:int in_egroup_p(gid_t grp)
kernel/sysctl.c: else if (in_egroup_p(0))

So from this I would conclude that these two functions are meant to be
used only when checking permissions -- in_group_p for checking access
to resources and in_egroup_p for checking the identity of the task.
Unfortunately, their naming doesn't communicate this very well, so the
author of the problematic commit used them incorrectly. Also note that
except of the very very rare case that FSGID != EGID these functions
do exactly the same thing, so we are effectively comparing both GID
and EGID under AUDIT_GID and only EGID under AUDIT_EGID, which makes
no sense. IMHO anyone who was able to discover this illogical behavior
AND did not report it as a bug AND now relies on it should suffer the
consequences of this fix anyway.

Post by Paul Moore
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?

I just ran the audit-testsuite on it (I finally manged to get the
remaining bits working :) and it passes (but that proves nothing,
since AFAIK the tests do not induce a situation where GID/EGID/FSGID
would differ).

I didn't cause any problems to me, but it will be a problem for anyone
who wants to match records for tasks that run with GID == x, but
doesn't want to match tasks with GID != x and EGID == x (there is no
way to do that right now and there is also no mention of this behavior
in the auditctl man page...).

The primary motivation for this patch is to fix the comparison to use
tsk instead of the current task. However, both bugs were caused by the
abuse of the in_[e]group functions, so it makes sense to fix them both
at once.

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

I agree, this has been broken for 6 years, another two months are not
going to make it much worse :)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/diff/kernel/sys.c?h=0.12&id=268175ce6feb5c4ad5987ee35606e0a2790f6a0c

Post by Paul Moore
--
paul moore
www.paul-moore.com