selinux: Use task_alloc hook rather than task_create hook

Commit Message

Tetsuo Handa March 28, 2017, 2:08 p.m. UTC

Stephen Smalley wrote:
> On Tue, 2017-03-28 at 22:12 +0900, Tetsuo Handa wrote:
> > This patch is a preparation for getting rid of task_create hook
> > because
> > task_create hook
> 
> task_alloc hook?

Oops, copy&paste error. Yes, I meant task_alloc hook.

> 
> >  which can do what task_create hook can do was revived.
> > 
> > Creating a new thread is unlikely prohibited by security policy, for
> > fork()/execve()/exit() is fundamental of how processes are managed in
> > Unix. If a program is known to create a new thread, it is likely that
> > permission to create a new thread is given to that program.
> > Therefore,
> > a situation where security_task_create() returns an error is likely
> > that
> > the program was exploited and lost control. Even if SELinux failed to
> > check permission to create a thread at security_task_create(),
> > SELinux
> > can later check it at security_task_alloc(). Since the new thread is
> > not
> > yet visible from the rest of the system, nobody can do bad things
> > using
> > the new thread. What we waste will be limited to some initialization
> > steps such as dup_task_struct(), copy_creds() and audit_alloc() in
> > copy_process(). We can tolerate these overhead for unlikely
> > situation.
> > 
> > Therefore, this patch changes SELinux to use task_alloc hook rather
> > than
> > task_create hook so that we can remove task_create hook.
> 
> Aside from the nit on the patch description above,
> 
> Acked-by: Stephen Smalley <[e-mail]>

Thank you.

>From b43bd0fc0cc267b91f51ad118f6fabd13efb921e Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Tue, 28 Mar 2017 22:09:38 +0900
Subject: [PATCH v2] selinux: Use task_alloc hook rather than task_create hook

This patch is a preparation for getting rid of task_create hook because
task_alloc hook which can do what task_create hook can do was revived.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in
Unix. If a program is known to create a new thread, it is likely that
permission to create a new thread is given to that program. Therefore,
a situation where security_task_create() returns an error is likely that
the program was exploited and lost control. Even if SELinux failed to
check permission to create a thread at security_task_create(), SELinux
can later check it at security_task_alloc(). Since the new thread is not
yet visible from the rest of the system, nobody can do bad things using
the new thread. What we waste will be limited to some initialization
steps such as dup_task_struct(), copy_creds() and audit_alloc() in
copy_process(). We can tolerate these overhead for unlikely situation.

Therefore, this patch changes SELinux to use task_alloc hook rather than
task_create hook so that we can remove task_create hook.

Signed-off-by: Tetsuo Handa <[e-mail]>
Acked-by: Stephen Smalley <[e-mail]>
---
 security/selinux/hooks.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Paul Moore March 29, 2017, 10:08 p.m. UTC | #1

On Tue, Mar 28, 2017 at 10:08 AM, Tetsuo Handa
<[e-mail]> wrote:
> >From b43bd0fc0cc267b91f51ad118f6fabd13efb921e Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Tue, 28 Mar 2017 22:09:38 +0900
> Subject: [PATCH v2] selinux: Use task_alloc hook rather than task_create hook
>
> This patch is a preparation for getting rid of task_create hook because
> task_alloc hook which can do what task_create hook can do was revived.
>
> Creating a new thread is unlikely prohibited by security policy, for
> fork()/execve()/exit() is fundamental of how processes are managed in
> Unix. If a program is known to create a new thread, it is likely that
> permission to create a new thread is given to that program. Therefore,
> a situation where security_task_create() returns an error is likely that
> the program was exploited and lost control. Even if SELinux failed to
> check permission to create a thread at security_task_create(), SELinux
> can later check it at security_task_alloc(). Since the new thread is not
> yet visible from the rest of the system, nobody can do bad things using
> the new thread. What we waste will be limited to some initialization
> steps such as dup_task_struct(), copy_creds() and audit_alloc() in
> copy_process(). We can tolerate these overhead for unlikely situation.
>
> Therefore, this patch changes SELinux to use task_alloc hook rather than
> task_create hook so that we can remove task_create hook.
>
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Acked-by: Stephen Smalley <[e-mail]>
> ---
>  security/selinux/hooks.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

When are you planning to remove the task_create() hook?

I have no objection to this patch, and I plan to merge it, but merging
it now would require rebasing the selinux/next and I try to keep from
rebasing during the development cycle unless absolutely necessary.  I
think this can wait until after the next merge window, what do you
think?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d37a723..d850b7f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3710,7 +3710,8 @@ static int selinux_file_open(struct file *file, const struct cred *cred)
>
>  /* task security operations */
>
> -static int selinux_task_create(unsigned long clone_flags)
> +static int selinux_task_alloc(struct task_struct *task,
> +                             unsigned long clone_flags)
>  {
>         u32 sid = current_sid();
>
> @@ -6205,7 +6206,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>
>         LSM_HOOK_INIT(file_open, selinux_file_open),
>
> -       LSM_HOOK_INIT(task_create, selinux_task_create),
> +       LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
>         LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
>         LSM_HOOK_INIT(cred_free, selinux_cred_free),
>         LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),
> --
> 1.8.3.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tetsuo Handa March 30, 2017, 11:13 a.m. UTC | #2

Paul Moore wrote:
> > Signed-off-by: Tetsuo Handa <[e-mail]>
> > Acked-by: Stephen Smalley <[e-mail]>
> > ---
> >  security/selinux/hooks.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> When are you planning to remove the task_create() hook?
> 
> I have no objection to this patch, and I plan to merge it, but merging
> it now would require rebasing the selinux/next and I try to keep from
> rebasing during the development cycle unless absolutely necessary.  I
> think this can wait until after the next merge window, what do you
> think?

Nothing to hurry. SELinux is the only user. We can wait as much as you want.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paul Moore March 31, 2017, 7:20 p.m. UTC | #3

On Thu, Mar 30, 2017 at 7:13 AM, Tetsuo Handa
<[e-mail]> wrote:
> Paul Moore wrote:
>> > Signed-off-by: Tetsuo Handa <[e-mail]>
>> > Acked-by: Stephen Smalley <[e-mail]>
>> > ---
>> >  security/selinux/hooks.c | 5 +++--
>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> When are you planning to remove the task_create() hook?
>>
>> I have no objection to this patch, and I plan to merge it, but merging
>> it now would require rebasing the selinux/next and I try to keep from
>> rebasing during the development cycle unless absolutely necessary.  I
>> think this can wait until after the next merge window, what do you
>> think?
>
> Nothing to hurry. SELinux is the only user. We can wait as much as you want.

Okay, I'll leave this in the queue and I'll merge it after the next
merge window.

Thanks.

Paul Moore May 16, 2017, 6:21 p.m. UTC | #4

On Fri, Mar 31, 2017 at 3:20 PM, Paul Moore <[e-mail]> wrote:
> On Thu, Mar 30, 2017 at 7:13 AM, Tetsuo Handa
> <[e-mail]> wrote:
>> Paul Moore wrote:
>>> > Signed-off-by: Tetsuo Handa <[e-mail]>
>>> > Acked-by: Stephen Smalley <[e-mail]>
>>> > ---
>>> >  security/selinux/hooks.c | 5 +++--
>>> >  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> When are you planning to remove the task_create() hook?
>>>
>>> I have no objection to this patch, and I plan to merge it, but merging
>>> it now would require rebasing the selinux/next and I try to keep from
>>> rebasing during the development cycle unless absolutely necessary.  I
>>> think this can wait until after the next merge window, what do you
>>> think?
>>
>> Nothing to hurry. SELinux is the only user. We can wait as much as you want.
>
> Okay, I'll leave this in the queue and I'll merge it after the next
> merge window.
>
> Thanks.

Merged, thanks again.

Patch

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d37a723..d850b7f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3710,7 +3710,8 @@  static int selinux_file_open(struct file *file, const struct cred *cred)
 
 /* task security operations */
 
-static int selinux_task_create(unsigned long clone_flags)
+static int selinux_task_alloc(struct task_struct *task,
+			      unsigned long clone_flags)
 {
 	u32 sid = current_sid();
 
@@ -6205,7 +6206,7 @@  static int selinux_key_getsecurity(struct key *key, char **_buffer)
 
 	LSM_HOOK_INIT(file_open, selinux_file_open),
 
-	LSM_HOOK_INIT(task_create, selinux_task_create),
+	LSM_HOOK_INIT(task_alloc, selinux_task_alloc),
 	LSM_HOOK_INIT(cred_alloc_blank, selinux_cred_alloc_blank),
 	LSM_HOOK_INIT(cred_free, selinux_cred_free),
 	LSM_HOOK_INIT(cred_prepare, selinux_cred_prepare),