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

[PATCH ghak82] 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 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@vger.kernel.org
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 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

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

[Richard Guy Briggs]

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@vger.kernel.org
> Signed-off-by: Ondrej Mosnacek <omosnace@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?

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

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

[Ondrej Mosnacek]

2018-05-17 19:07 GMT+02:00 Richard Guy Briggs <rgb@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@vger.kernel.org
>> Signed-off-by: Ondrej Mosnacek <omosnace@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.

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