[RFC/PATCH,1/3] security: add the security_task_copy() hook

Commit Message

Djalal Harouni Feb. 2, 2017, 5:04 p.m. UTC

From: Djalal Harouni <[e-mail]>

This hook is needed by Timgad LSM. Timgad uses a sysctl and a
per-process prctl() interface to control which processes are allowed to
load and unload modules. However to achieve that we need new hooks to
save the context of tasks.

Sadly there is no way, if we want to add new LSMs we are faced with two
choices:

1) Either fight and collide we other major LSMs since all of them
   they do save their context on the same shared security structs.

2) If we want our module to be a minor stackable LSM, that can be used
   easily without any pain, we have to find a new way to store our context.

Our use case is more in the seccomp logic, we want to add or fix a gap
that seccomp can not handle. At the same time we want to keep the
interface as easy as possible for userspace sandboxing. After some
years it seems like a good easy security feature for userspace should
apply at the same time when we apply: seccomp filters and the
no_new_privs flag.

To achieve the above we add the security_task_copy() hook that allows us
to clone the Timgad context of parent into child task_struct.

The security hook can also be used by new LSMs after the child task has
done some initialization, this way they won't clash with the major LSMs.
The situation is not really well, this hook allows us to introduce a
stackable LSM that can be easily used with all other LSMs.

Cc: Kees Cook <[e-mail]>
Signed-off-by: Djalal Harouni <[e-mail]>
---
 include/linux/lsm_hooks.h | 2 ++
 include/linux/security.h  | 6 ++++++
 kernel/fork.c             | 4 ++++
 security/security.c       | 6 ++++++
 4 files changed, 18 insertions(+)

Comments

Tetsuo Handa Feb. 6, 2017, 10:49 a.m. UTC | #1

Djalal Harouni wrote:
> To achieve the above we add the security_task_copy() hook that allows us
> to clone the Timgad context of parent into child task_struct.
> 
> The security hook can also be used by new LSMs after the child task has
> done some initialization, this way they won't clash with the major LSMs.
> The situation is not really well, this hook allows us to introduce a
> stackable LSM that can be easily used with all other LSMs.

We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
( news://news.gmane.org:119/[e-mail] ).
Is security_task_alloc() called too early for your case?

(Well, we want to configure http archive like marc.info ?)

Djalal Harouni Feb. 6, 2017, 12:40 p.m. UTC | #2

Hi Tetsuo,

On Mon, Feb 6, 2017 at 11:49 AM, Tetsuo Handa
<[e-mail]> wrote:
> Djalal Harouni wrote:
>> To achieve the above we add the security_task_copy() hook that allows us
>> to clone the Timgad context of parent into child task_struct.
>>
>> The security hook can also be used by new LSMs after the child task has
>> done some initialization, this way they won't clash with the major LSMs.
>> The situation is not really well, this hook allows us to introduce a
>> stackable LSM that can be easily used with all other LSMs.
>
> We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
> ( news://news.gmane.org:119/[e-mail] ).
> Is security_task_alloc() called too early for your case?

Hmm, didn't know about it, thank you!

Yes that seems the same, to have a per-task vars or context.

I'm reading http://www.spinics.net/linux/fedora/linux-security-module/msg17004.html

For my specific use case I'm still not sure. I was thinking that since
this is LSM maybe it would be useful to only add one hook after all
the copy_*() functions that also copy the context, maybe another LSM
will find it useful and may want to check the context of namespaces,
fs (copy_namespaces(), copy_fs() ...). I mean this already happened to
me with security_task_create() which is too early, so maybe try to do
it right.

The other thing is cgroups cgroup_can_fork() better do it after which
avoids me to clean up. IMHO the best place is near or after
copy_seccomp() where we hold the lock, especially if there is an LSM
that wants to copy/apply the context on all other threads not only
current. Ultimately if we are copying stuff then maybe it should
happen as late as possible, and if we have to sleep I guess we can use
security_task_create() to allocate ?

What do you think ?

Thanks!

> (Well, we want to configure http archive like marc.info ?)

Djalal Harouni Feb. 6, 2017, 1:10 p.m. UTC | #3

On Mon, Feb 6, 2017 at 11:49 AM, Tetsuo Handa
<[e-mail]> wrote:
> Djalal Harouni wrote:
>> To achieve the above we add the security_task_copy() hook that allows us
>> to clone the Timgad context of parent into child task_struct.
>>
>> The security hook can also be used by new LSMs after the child task has
>> done some initialization, this way they won't clash with the major LSMs.
>> The situation is not really well, this hook allows us to introduce a
>> stackable LSM that can be easily used with all other LSMs.
>
> We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
> ( news://news.gmane.org:119/[e-mail] ).
> Is security_task_alloc() called too early for your case?
>
> (Well, we want to configure http archive like marc.info ?)

I found this marc.info
http://marc.info/?l=linux-security-module&m=129584883703846

Oups from 2011! your email pretty sum up all the approach that I took
:-) , and yes it was not that obvious... so I did it that way where
the data is kept in an external table. Also yes beside TOMOYO I also
need that hook for the various reasons that are listed in that thread.

Thanks!

Patch

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 558adfa..b37e35e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1479,6 +1479,7 @@  union security_list_options {
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_copy)(struct task_struct *task);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1745,6 +1746,7 @@  struct security_hook_heads {
 	struct list_head file_receive;
 	struct list_head file_open;
 	struct list_head task_create;
+	struct list_head task_copy;
 	struct list_head task_free;
 	struct list_head cred_alloc_blank;
 	struct list_head cred_free;
diff --git a/include/linux/security.h b/include/linux/security.h
index c2125e9..748058e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -305,6 +305,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_copy(struct task_struct *task);
 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);
@@ -857,6 +858,11 @@  static inline int security_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static inline int security_task_copy(struct task_struct *task)
+{
+	return 0;
+}
+
 static inline void security_task_free(struct task_struct *task)
 { }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 11c5c8a..81c29d8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1746,6 +1746,10 @@  static __latent_entropy struct task_struct *copy_process(
 	if (retval)
 		goto bad_fork_free_pid;
 
+	retval = security_task_copy(p);
+	if (retval)
+		goto bad_fork_cancel_cgroup;
+
 	/*
 	 * Make it visible to the rest of the system, but dont wake it up yet.
 	 * Need tasklist lock for parent etc handling!
diff --git a/security/security.c b/security/security.c
index f825304..5c699c8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -892,6 +892,11 @@  int security_task_create(unsigned long clone_flags)
 	return call_int_hook(task_create, 0, clone_flags);
 }
 
+int security_task_copy(struct task_struct *task)
+{
+	return call_int_hook(task_copy, 0, task);
+}
+
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);
@@ -1731,6 +1736,7 @@  struct security_hook_heads security_hook_heads = {
 	.file_receive =	LIST_HEAD_INIT(security_hook_heads.file_receive),
 	.file_open =	LIST_HEAD_INIT(security_hook_heads.file_open),
 	.task_create =	LIST_HEAD_INIT(security_hook_heads.task_create),
+	.task_copy =    LIST_HEAD_INIT(security_hook_heads.task_copy),
 	.task_free =	LIST_HEAD_INIT(security_hook_heads.task_free),
 	.cred_alloc_blank =
 		LIST_HEAD_INIT(security_hook_heads.cred_alloc_blank),