[PATCH ghak82] audit: Fix wrong task in comparison of session ID

[PATCH ghak82] audit: Fix wrong task in comparison of session ID

[Ondrej Mosnacek]

The audit_filter_rules() function in auditsc.c compared the session ID
with the credentials of the current task, while it should use the
credentials of the task given to audit_filter_rules() as a parameter
(tsk).

GitHub issue:
https://github.com/linux-audit/audit-kernel/issues/82

Fixes: 8fae47705685 ("audit: add support for session ID user filter")
Cc: stable@vger.kernel.org
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/auditsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ec38e4d97c23..6d577a34b16b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -513,7 +513,7 @@ static int audit_filter_rules(struct task_struct *tsk,
 			result = audit_gid_comparator(cred->fsgid, f->op, f->gid);
 			break;
 		case AUDIT_SESSIONID:
-			sessionid = audit_get_sessionid(current);
+			sessionid = audit_get_sessionid(tsk);
 			result = audit_comparator(sessionid, f->op, f->val);
 			break;
 		case AUDIT_PERS:
-- 
2.17.0

Re: [PATCH ghak82] audit: Fix wrong task in comparison of session ID

[Richard Guy Briggs]

On 2018-05-17 17:31, Ondrej Mosnacek wrote:
> The audit_filter_rules() function in auditsc.c compared the session ID
> with the credentials of the current task, while it should use the
> credentials of the task given to audit_filter_rules() as a parameter
> (tsk).
> 
> GitHub issue:
> https://github.com/linux-audit/audit-kernel/issues/82
> 
> Fixes: 8fae47705685 ("audit: add support for session ID user filter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  kernel/auditsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ec38e4d97c23..6d577a34b16b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -513,7 +513,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>  			result = audit_gid_comparator(cred->fsgid, f->op, f->gid);
>  			break;
>  		case AUDIT_SESSIONID:
> -			sessionid = audit_get_sessionid(current);
> +			sessionid = audit_get_sessionid(tsk);
>  			result = audit_comparator(sessionid, f->op, f->val);
>  			break;
>  		case AUDIT_PERS:
> -- 
> 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 wrong task in comparison of session ID

[Paul Moore]

On Thu, May 17, 2018 at 11:31 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> The audit_filter_rules() function in auditsc.c compared the session ID
> with the credentials of the current task, while it should use the
> credentials of the task given to audit_filter_rules() as a parameter
> (tsk).
>
> GitHub issue:
> https://github.com/linux-audit/audit-kernel/issues/82
>
> Fixes: 8fae47705685 ("audit: add support for session ID user filter")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/auditsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Good catch.  However, I'm not completely convinced this is stable material.

This bug really only affects "task" filters which I believe to be
fairly limited in the wild, due to the limited number of fields that
one can filter on.  Every other filter, including the "exit" filter,
work as expected (which is why the audit-testsuite didn't catch this
bug).  Further, even in the case of the task filter, the *only* time
where the current session ID would be different from the tsk session
ID is in the case of a login event, but even in this case the two
values should be equal during the "task" filtering as you can't change
the login-ID/session-ID until after you have successfully
fork()'d/clone()'d the new task.

I'll hold off on merging this in case I'm missing some even more
subtle point that you've found.  From what I can tell this is a good
fix, but not something an actual user would ever really encounter and
therefore not something that warrants inclusion in stable.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ec38e4d97c23..6d577a34b16b 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -513,7 +513,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>                         result = audit_gid_comparator(cred->fsgid, f->op, f->gid);
>                         break;
>                 case AUDIT_SESSIONID:
> -                       sessionid = audit_get_sessionid(current);
> +                       sessionid = audit_get_sessionid(tsk);
>                         result = audit_comparator(sessionid, f->op, f->val);
>                         break;
>                 case AUDIT_PERS:
> --
> 2.17.0

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak82] audit: Fix wrong task in comparison of session ID

[Ondrej Mosnacek]

2018-05-18 21:23 GMT+02:00 Paul Moore <paul@paul-moore.com>:
> On Thu, May 17, 2018 at 11:31 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> The audit_filter_rules() function in auditsc.c compared the session ID
>> with the credentials of the current task, while it should use the
>> credentials of the task given to audit_filter_rules() as a parameter
>> (tsk).
>>
>> GitHub issue:
>> https://github.com/linux-audit/audit-kernel/issues/82
>>
>> Fixes: 8fae47705685 ("audit: add support for session ID user filter")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>> ---
>>  kernel/auditsc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Good catch.  However, I'm not completely convinced this is stable material.
>
> This bug really only affects "task" filters which I believe to be
> fairly limited in the wild, due to the limited number of fields that
> one can filter on.  Every other filter, including the "exit" filter,
> work as expected (which is why the audit-testsuite didn't catch this
> bug).  Further, even in the case of the task filter, the *only* time
> where the current session ID would be different from the tsk session
> ID is in the case of a login event, but even in this case the two
> values should be equal during the "task" filtering as you can't change
> the login-ID/session-ID until after you have successfully
> fork()'d/clone()'d the new task.
>
> I'll hold off on merging this in case I'm missing some even more
> subtle point that you've found.  From what I can tell this is a good
> fix, but not something an actual user would ever really encounter and
> therefore not something that warrants inclusion in stable.

Fair enough, I don't have any objections.

>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index ec38e4d97c23..6d577a34b16b 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -513,7 +513,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>>                         result = audit_gid_comparator(cred->fsgid, f->op, f->gid);
>>                         break;
>>                 case AUDIT_SESSIONID:
>> -                       sessionid = audit_get_sessionid(current);
>> +                       sessionid = audit_get_sessionid(tsk);
>>                         result = audit_comparator(sessionid, f->op, f->val);
>>                         break;
>>                 case AUDIT_PERS:
>> --
>> 2.17.0
>
> --
> paul moore
> www.paul-moore.com

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH ghak82] audit: Fix wrong task in comparison of session ID

[Paul Moore]

On Mon, May 21, 2018 at 3:50 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
> 2018-05-18 21:23 GMT+02:00 Paul Moore <paul@paul-moore.com>:
>> On Thu, May 17, 2018 at 11:31 AM, Ondrej Mosnacek <omosnace@redhat.com> wrote:
>>> The audit_filter_rules() function in auditsc.c compared the session ID
>>> with the credentials of the current task, while it should use the
>>> credentials of the task given to audit_filter_rules() as a parameter
>>> (tsk).
>>>
>>> GitHub issue:
>>> https://github.com/linux-audit/audit-kernel/issues/82
>>>
>>> Fixes: 8fae47705685 ("audit: add support for session ID user filter")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>> ---
>>>  kernel/auditsc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Good catch.  However, I'm not completely convinced this is stable material.
>>
>> This bug really only affects "task" filters which I believe to be
>> fairly limited in the wild, due to the limited number of fields that
>> one can filter on.  Every other filter, including the "exit" filter,
>> work as expected (which is why the audit-testsuite didn't catch this
>> bug).  Further, even in the case of the task filter, the *only* time
>> where the current session ID would be different from the tsk session
>> ID is in the case of a login event, but even in this case the two
>> values should be equal during the "task" filtering as you can't change
>> the login-ID/session-ID until after you have successfully
>> fork()'d/clone()'d the new task.
>>
>> I'll hold off on merging this in case I'm missing some even more
>> subtle point that you've found.  From what I can tell this is a good
>> fix, but not something an actual user would ever really encounter and
>> therefore not something that warrants inclusion in stable.
>
> Fair enough, I don't have any objections.

Merged into audit/next.  In the future you don't have to worry about
adding the stable tag to patches, if it warrants sending to stable I
can always add it when I merge the patch.

-- 
paul moore
www.paul-moore.com