LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

Commit Message

Tetsuo Handa March 24, 2017, 11:46 a.m. UTC

We switched from "struct task_struct"->security to "struct cred"->security
in Linux 2.6.29. But not all LSM modules were happy with that change.
TOMOYO LSM module is an example which want to use per "struct task_struct"
security blob, for TOMOYO's security context is defined based on "struct
task_struct" rather than "struct cred". AppArmor LSM module is another
example which want to use it, for AppArmor is currently abusing the cred
a little bit to store the change_hat and setexeccon info. Although
security_task_free() hook was revived in Linux 3.4 because Yama LSM module
wanted to release per "struct task_struct" security blob,
security_task_alloc() hook and "struct task_struct"->security field were
not revived. Nowadays, we are getting proposals of lightweight LSM modules
which want to use per "struct task_struct" security blob. PTAGS LSM module
and CaitSith LSM module which are currently under proposal for inclusion
also want to use it. Therefore, it will be time to revive
security_task_alloc() hook and "struct task_struct"->security field.

We are already allowing multiple concurrent LSM modules (up to one fully
armored module which uses "struct cred"->security field or exclusive hooks
like security_xfrm_state_pol_flow_match(), plus unlimited number of
lightweight modules which do not use "struct cred"->security nor exclusive
hooks) as long as they are built into the kernel. But this patch does not
implement variable length "struct task_struct"->security field which will
become needed when multiple LSM modules want to use "struct task_struct"->
security field. Although it won't be difficult to implement variable length
"struct task_struct"->security field, let's think about it after we merged
this patch.

Signed-off-by: Tetsuo Handa <[e-mail]>
Acked-by: John Johansen <[e-mail]>
Acked-by: Serge Hallyn <[e-mail]>
Acked-by: Casey Schaufler <[e-mail]>
Tested-by: Djalal Harouni <[e-mail]>
Cc: Paul Moore <[e-mail]>
Cc: Stephen Smalley <[e-mail]>
Cc: Eric Paris <[e-mail]>
Cc: Kees Cook <[e-mail]>
Cc: James Morris <[e-mail]>
Cc: José Bollo <[e-mail]>
---
 include/linux/init_task.h | 7 +++++++
 include/linux/lsm_hooks.h | 9 ++++++++-
 include/linux/sched.h     | 4 ++++
 include/linux/security.h  | 7 +++++++
 kernel/fork.c             | 7 ++++++-
 security/security.c       | 5 +++++
 6 files changed, 37 insertions(+), 2 deletions(-)

Comments

José Bollo March 24, 2017, 12:11 p.m. UTC | #1

On Fri, 24 Mar 2017 20:46:33 +0900
Tetsuo Handa <[e-mail]> wrote:

> We switched from "struct task_struct"->security to "struct
> cred"->security in Linux 2.6.29. But not all LSM modules were happy
> with that change. TOMOYO LSM module is an example which want to use
> per "struct task_struct" security blob, for TOMOYO's security context
> is defined based on "struct task_struct" rather than "struct cred".
> AppArmor LSM module is another example which want to use it, for
> AppArmor is currently abusing the cred a little bit to store the
> change_hat and setexeccon info. Although security_task_free() hook
> was revived in Linux 3.4 because Yama LSM module wanted to release
> per "struct task_struct" security blob, security_task_alloc() hook
> and "struct task_struct"->security field were not revived. Nowadays,
> we are getting proposals of lightweight LSM modules which want to use
> per "struct task_struct" security blob. PTAGS LSM module and CaitSith
> LSM module which are currently under proposal for inclusion also want
> to use it. Therefore, it will be time to revive security_task_alloc()
> hook and "struct task_struct"->security field.
> 
> We are already allowing multiple concurrent LSM modules (up to one
> fully armored module which uses "struct cred"->security field or
> exclusive hooks like security_xfrm_state_pol_flow_match(), plus
> unlimited number of lightweight modules which do not use "struct
> cred"->security nor exclusive hooks) as long as they are built into
> the kernel. But this patch does not implement variable length "struct
> task_struct"->security field which will become needed when multiple
> LSM modules want to use "struct task_struct"-> security field.
> Although it won't be difficult to implement variable length "struct
> task_struct"->security field, let's think about it after we merged
> this patch.
> 
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Acked-by: John Johansen <[e-mail]>
> Acked-by: Serge Hallyn <[e-mail]>
> Acked-by: Casey Schaufler <[e-mail]>

Acked-by: José Bollo <[e-mail]>

(if it cares ;)

Also below, I expect in some future that task_alloc and task_create
will be merged. IMHO not allocating the task is of less importance than
having a simple and unique hook. Statistically the normal is "the
task is allowed" so the cost of creating the task structure for
nothing is only relevant in cases where efficiency is just not expected.


> Tested-by: Djalal Harouni <[e-mail]>
> Cc: Paul Moore <[e-mail]>
> Cc: Stephen Smalley <[e-mail]>
> Cc: Eric Paris <[e-mail]>
> Cc: Kees Cook <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: José Bollo <[e-mail]>
> ---
>  include/linux/init_task.h | 7 +++++++
>  include/linux/lsm_hooks.h | 9 ++++++++-
>  include/linux/sched.h     | 4 ++++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 5 +++++
>  6 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 91d9049..926f2f5 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -210,6 +210,12 @@
>  # define INIT_TASK_TI(tsk)
>  #endif
>  
> +#ifdef CONFIG_SECURITY
> +#define INIT_TASK_SECURITY .security = NULL,
> +#else
> +#define INIT_TASK_SECURITY
> +#endif
> +
>  /*
>   *  INIT_TASK is used to set up the first task table, touch at
>   * your own risk!. Base=0, limit=0x1fffff (=2MB)
> @@ -288,6 +294,7 @@
>  	INIT_VTIME(tsk)
> \ INIT_NUMA_BALANCING(tsk)					\
>  	INIT_KASAN(tsk)
> \
> +
> INIT_TASK_SECURITY						\ }
>  
>  
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 54191cf..865c11d 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -533,8 +533,13 @@
>   *	manual page for definitions of the @clone_flags.
>   *	@clone_flags contains the flags indicating what should be
> shared.
>   *	Return 0 if permission is granted.
> + * @task_alloc:
> + *	@task task being allocated.
> + *	@clone_flags contains the flags indicating what should be
> shared.
> + *	Handle allocation of task-related resources.
> + *	Returns a zero on success, negative values on failure.
>   * @task_free:
> - *	@task task being freed
> + *	@task task about to be freed.
>   *	Handle release of task-related resources. (Note that this
> can be called
>   *	from interrupt context.)
>   * @cred_alloc_blank:
> @@ -1482,6 +1487,7 @@
>  	int (*file_open)(struct file *file, const struct cred *cred);
>  
>  	int (*task_create)(unsigned long clone_flags);
> +	int (*task_alloc)(struct task_struct *task, unsigned long
> clone_flags); void (*task_free)(struct task_struct *task);
>  	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
>  	void (*cred_free)(struct cred *cred);
> @@ -1748,6 +1754,7 @@ struct security_hook_heads {
>  	struct list_head file_receive;
>  	struct list_head file_open;
>  	struct list_head task_create;
> +	struct list_head task_alloc;
>  	struct list_head task_free;
>  	struct list_head cred_alloc_blank;
>  	struct list_head cred_free;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d67eee8..71b8df3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1038,6 +1038,10 @@ struct task_struct {
>  	/* A live task holds one reference: */
>  	atomic_t			stack_refcount;
>  #endif
> +#ifdef CONFIG_SECURITY
> +	/* Used by LSM modules for access restriction: */
> +	void				*security;
> +#endif
>  	/* CPU-specific state of this task: */
>  	struct thread_struct		thread;
>  
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 97df7ba..af675b5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -308,6 +308,7 @@ int security_file_send_sigiotask(struct
> task_struct *tsk, int security_file_receive(struct file *file);
>  int security_file_open(struct file *file, const struct cred *cred);
>  int security_task_create(unsigned long clone_flags);
> +int security_task_alloc(struct task_struct *task, unsigned long
> clone_flags); void security_task_free(struct task_struct *task);
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>  void security_cred_free(struct cred *cred);
> @@ -861,6 +862,12 @@ static inline int security_task_create(unsigned
> long clone_flags) return 0;
>  }
>  
> +static inline int security_task_alloc(struct task_struct *task,
> +				      unsigned long clone_flags)
> +{
> +	return 0;
> +}
> +
>  static inline void security_task_free(struct task_struct *task)
>  { }
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6c463c80..3d32513 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1679,9 +1679,12 @@ static __latent_entropy struct task_struct
> *copy_process( goto bad_fork_cleanup_perf;
>  	/* copy all the process information */
>  	shm_init_task(p);
> -	retval = copy_semundo(clone_flags, p);
> +	retval = security_task_alloc(p, clone_flags);
>  	if (retval)
>  		goto bad_fork_cleanup_audit;
> +	retval = copy_semundo(clone_flags, p);
> +	if (retval)
> +		goto bad_fork_cleanup_security;
>  	retval = copy_files(clone_flags, p);
>  	if (retval)
>  		goto bad_fork_cleanup_semundo;
> @@ -1903,6 +1906,8 @@ static __latent_entropy struct task_struct
> *copy_process( exit_files(p); /* blocking */
>  bad_fork_cleanup_semundo:
>  	exit_sem(p);
> +bad_fork_cleanup_security:
> +	security_task_free(p);
>  bad_fork_cleanup_audit:
>  	audit_free(p);
>  bad_fork_cleanup_perf:
> diff --git a/security/security.c b/security/security.c
> index 45af8fb..8c62fc3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -946,6 +946,11 @@ int security_task_create(unsigned long
> clone_flags) return call_int_hook(task_create, 0, clone_flags);
>  }
>  
> +int security_task_alloc(struct task_struct *task, unsigned long
> clone_flags) +{
> +	return call_int_hook(task_alloc, 0, task, clone_flags);
> +}
> +
>  void security_task_free(struct task_struct *task)
>  {
>  	call_void_hook(task_free, task);

--
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 24, 2017, 12:27 p.m. UTC | #2

Jose Bollo wrote:
> Acked-by: Jose Bollo <[e-mail]>
> 
> (if it cares ;)

Thanks. I reposted this patch in case James missed this patch.

> 
> Also below, I expect in some future that task_alloc and task_create
> will be merged. IMHO not allocating the task is of less importance than
> having a simple and unique hook. Statistically the normal is "the
> task is allowed" so the cost of creating the task structure for
> nothing is only relevant in cases where efficiency is just not expected.
> 
You have below post in your mail box. ;-)

------- Forwarded Message
From: Tetsuo Handa <[e-mail]>
To: [e-mail], [e-mail]
Cc: [e-mail], [e-mail], [e-mail],[e-mail], [e-mail],[e-mail], [e-mail],[e-mail]
Subject: Re: [PATCH] LSM: Revive security_task_alloc() hook.
Date: Fri, 6 Jan 2017 20:35:16 +0900

(...snipped...)
>> @@ -1479,6 +1489,7 @@
>>       int (*file_open)(struct file *file, const struct cred *cred);
>>  
>>       int (*task_create)(unsigned long clone_flags);
>> +     int (*task_alloc)(struct task_struct *task);
> I suggest to add the 'clone_flags' as below
>
>      int (*task_alloc)(
>                     struct task_struct *task,
>                     unsigned long clone_flags);
>
> It would allow to treat CLONE_THREAD and/or CLONE_NEW... in a specific
> way.

OK. I added it. Now, I'm tempted to eliminate security_task_create() call.

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(). I think we can tolerate
these overhead for unlikely situation. What do SELinux people think?

(...snipped...)
--
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 26, 2017, 1:49 a.m. UTC | #3

James, would you teach me why this patch still cannot go to linux-next?
If we don't merge this patch now, we will again fail to land in 4.12.

> On 02/01/2017 08:02 PM, James Morris wrote:
> > On Wed, 1 Feb 2017, John Johansen wrote:
> >
> >> Sorry this took so long, it looks good to me, and I have done
> >> some builds and tests with apparmor using it. The apparmor patch
> >> to make use of this follows as a reply.
> >>
> >> Acked-by: John Johansen <[e-mail]>
> >
> > We're too late in the -rc cycle to take these for 4.11.  Please
> > keep testing them and some more review/acks would also be good.
> >
> Certainly, I didn't expect this to land in 4.11. I would really like
> get some feed back/review/ack from the owner of the task struct.
--
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

James Morris March 26, 2017, 9:09 p.m. UTC | #4

On Sun, 26 Mar 2017, Tetsuo Handa wrote:

> James, would you teach me why this patch still cannot go to linux-next?
> If we don't merge this patch now, we will again fail to land in 4.12.

I'm still reviewing it.


> 
> > On 02/01/2017 08:02 PM, James Morris wrote:
> > > On Wed, 1 Feb 2017, John Johansen wrote:
> > >
> > >> Sorry this took so long, it looks good to me, and I have done
> > >> some builds and tests with apparmor using it. The apparmor patch
> > >> to make use of this follows as a reply.
> > >>
> > >> Acked-by: John Johansen <[e-mail]>
> > >
> > > We're too late in the -rc cycle to take these for 4.11.  Please
> > > keep testing them and some more review/acks would also be good.
> > >
> > Certainly, I didn't expect this to land in 4.11. I would really like
> > get some feed back/review/ack from the owner of the task struct.
>

James Morris March 28, 2017, 12:21 a.m. UTC | #5

On Fri, 24 Mar 2017, Tetsuo Handa wrote:

> Signed-off-by: Tetsuo Handa <[e-mail]>
> Acked-by: John Johansen <[e-mail]>
> Acked-by: Serge Hallyn <[e-mail]>
> Acked-by: Casey Schaufler <[e-mail]>
> Tested-by: Djalal Harouni <[e-mail]>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next

I also synced to a newer -rc (4) as requested.

Please test!

Tetsuo Handa March 28, 2017, 11:02 a.m. UTC | #6

James Morris wrote:
> On Fri, 24 Mar 2017, Tetsuo Handa wrote:
> 
> > Signed-off-by: Tetsuo Handa <[e-mail]>
> > Acked-by: John Johansen <[e-mail]>
> > Acked-by: Serge Hallyn <[e-mail]>
> > Acked-by: Casey Schaufler <[e-mail]>
> > Tested-by: Djalal Harouni <[e-mail]>
> 
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
> 
> I also synced to a newer -rc (4) as requested.
> 
> Please test!

Thank you very much. I confirmed that it passed TOMOYO's all testcases.
A patch which depends on this change follows.
--
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

Patch

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 91d9049..926f2f5 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -210,6 +210,12 @@ 
 # define INIT_TASK_TI(tsk)
 #endif
 
+#ifdef CONFIG_SECURITY
+#define INIT_TASK_SECURITY .security = NULL,
+#else
+#define INIT_TASK_SECURITY
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
@@ -288,6 +294,7 @@ 
 	INIT_VTIME(tsk)							\
 	INIT_NUMA_BALANCING(tsk)					\
 	INIT_KASAN(tsk)							\
+	INIT_TASK_SECURITY						\
 }
 
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 54191cf..865c11d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -533,8 +533,13 @@ 
  *	manual page for definitions of the @clone_flags.
  *	@clone_flags contains the flags indicating what should be shared.
  *	Return 0 if permission is granted.
+ * @task_alloc:
+ *	@task task being allocated.
+ *	@clone_flags contains the flags indicating what should be shared.
+ *	Handle allocation of task-related resources.
+ *	Returns a zero on success, negative values on failure.
  * @task_free:
- *	@task task being freed
+ *	@task task about to be freed.
  *	Handle release of task-related resources. (Note that this can be called
  *	from interrupt context.)
  * @cred_alloc_blank:
@@ -1482,6 +1487,7 @@ 
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1748,6 +1754,7 @@  struct security_hook_heads {
 	struct list_head file_receive;
 	struct list_head file_open;
 	struct list_head task_create;
+	struct list_head task_alloc;
 	struct list_head task_free;
 	struct list_head cred_alloc_blank;
 	struct list_head cred_free;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d67eee8..71b8df3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1038,6 +1038,10 @@  struct task_struct {
 	/* A live task holds one reference: */
 	atomic_t			stack_refcount;
 #endif
+#ifdef CONFIG_SECURITY
+	/* Used by LSM modules for access restriction: */
+	void				*security;
+#endif
 	/* CPU-specific state of this task: */
 	struct thread_struct		thread;
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 97df7ba..af675b5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -308,6 +308,7 @@  int security_file_send_sigiotask(struct task_struct *tsk,
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file, const struct cred *cred);
 int security_task_create(unsigned long clone_flags);
+int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
@@ -861,6 +862,12 @@  static inline int security_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static inline int security_task_alloc(struct task_struct *task,
+				      unsigned long clone_flags)
+{
+	return 0;
+}
+
 static inline void security_task_free(struct task_struct *task)
 { }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 6c463c80..3d32513 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1679,9 +1679,12 @@  static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
-	retval = copy_semundo(clone_flags, p);
+	retval = security_task_alloc(p, clone_flags);
 	if (retval)
 		goto bad_fork_cleanup_audit;
+	retval = copy_semundo(clone_flags, p);
+	if (retval)
+		goto bad_fork_cleanup_security;
 	retval = copy_files(clone_flags, p);
 	if (retval)
 		goto bad_fork_cleanup_semundo;
@@ -1903,6 +1906,8 @@  static __latent_entropy struct task_struct *copy_process(
 	exit_files(p); /* blocking */
 bad_fork_cleanup_semundo:
 	exit_sem(p);
+bad_fork_cleanup_security:
+	security_task_free(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
 bad_fork_cleanup_perf:
diff --git a/security/security.c b/security/security.c
index 45af8fb..8c62fc3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -946,6 +946,11 @@  int security_task_create(unsigned long clone_flags)
 	return call_int_hook(task_create, 0, clone_flags);
 }
 
+int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
+{
+	return call_int_hook(task_alloc, 0, task, clone_flags);
+}
+
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);