WhiteEgret is an LSM to simply provide a whitelisting-type
execution control.
An execution-whitelist, simply called whitelist, is a list
of executable components (e.g., applications, libraries)
that are approved to run on a host. The whitelist is used
to decide whether executable components are permitted to
execute or not. This mechanism can stop an execution of
unknown software, so it helps stop the execution of
malicious code and other unauthorized software.
It is important to maintain a whitelist properly according to
the execution environments. Managing whitelists for information
systems is a difficult task because their environments are
changed frequently. On the other hand, for such devices that
continue to do the same tasks for a certain period of time,
we can use the same whitelist for the period once the whitelist
is established. Thus the whitelisting-type execution control
works best in such execution environments. Examples of the above
execution environments include control devices in industrial
control systems.
Although the number of changing whitelists is not so large,
it is necessary to change them according to a system life cycle
or each phase of system operations. There is a requirement to
change whitelists with the system operations continued because
they often cannot easily be stopped. For example, such cases
include temporarily allowing maintenance programs for maintenance
or troubleshooting purposes while running the systems.
WhiteEgret is aiming at satisfying the above requirement.
WhiteEgret adopts a model that a whitelist is managed in user space.
Namely, WhiteEgret assumes that a privileged user manages a whitelist
in user space. This makes it possible to change the whitelist while
running the systems.
Mechanism of WhiteEgret
WhiteEgret requires a user application called WhiteEgret User
Application (WEUA, for short). WhiteEgret utilizes the
bprm_check_security hook and the mmap_file hook.
WhiteEgret asks WEUA whether an executable component hooked
by the above hooks is permitted to execute or not.
If the response from the WEUA is "permit", then WhiteEgret
continues to process the executable component. If the response
is "not permit", then WhiteEgret returns an error and blocks
the execution of the executable component.
The bprm_check_security hook is triggered by execve system
call, so execution by almost all executable components are
hooked by the hook. However, because shared objects do not
invoke execve system call, WhiteEgret utilizes the mmap_file
hook to hook the memory mapping by a shared object.
Thus WhiteEgret ignores the mmap_file hook caused by
non-executable and by executable which calls execve system call.
To ask the permission to a WEUA, WhiteEgret sends the
absolute path, the inode number and the device number of the
executable component to the WEUA.
Then the WEUA is expected to work as follows.
The WEUA sees if the tuple of the absolute path and/or the inode
number and/or the device number is contained in the whitelist.
If it exists, the WEUA compares a hash value of the executable
component indicated by the absolute path (and/or the inode number
and/or device number) with that in the whitelist to see whether
the executable component is changed or not after the whitelist is
made. The WEUA returns "permit" if both tests are passed,
otherwise returns "not permit".
WhiteEgret v4 is also able to control for script execution. Some
LSM hooks (file_open/file_permission/task_alloc/task_free) are
added. Kernel configs are required to enable the hooks.
Most of interpreters open script files to execute. Therefore
WhiteEgret hooks for reading or opening a file. Then WhiteEgret
asks the WEUA whether an execution of the script is permitted to
execute or not. WhiteEgret is able to choose a hook entry for
execution control between file_open or file_permission.
Hook entries of task_alloc and task_free are used to control
exections of script effectively. Some interpreters forks child
processes to execte script files, so the WEUA managed a process
family of an interpter by bprm_check_security, task_alloc and
task_free.
To use WhiteEgret
Users have to prepare a whitelist and a WEUA to use WhiteEgret.
A sample WEUA is involved in samples/whiteegret/.
To enable WhiteEgret, you are required to build the kernel using
normal procedures with CONFIG_SECURITY_WHITEEGRET=y.
Additionally, SECURITY_WHITEEGRET_INTERPRETER=y option is
required to enable to control script executions.
And SECURITY_WHITEEGRET_WRITE=y option is also required to
detect of writing script files.
The patchset is also available in our github repo:
https://github.com/whiteegret/whiteegret
---
Changes in v4:
- Add LSM hooks (file_open/file_permission/task_alloc/task_free)
to control for script execution. Kernel configs are required
to enable the hooks.
- Modify the data struct for kernel space and user space
communication.
Changes in v3:
- Change to a minor LSM module.
Changes in v2:
- Change communication method between kernel space and user space
from netlink to device driver, and device driver utilizes not LKM
but securityfs.
- Add inode number and device number to information of executable
component sent from kernel space to user space.
- Fix bugs regarding to locks during kernel space and user space
communication.
- Change return value from -EPERM to -EACCES when the execution of
an executable component is denied.
Shinya Takumi (2):
WhiteEgret: Add WhiteEgret core functions.
WhiteEgret: Add an example of user application.
samples/Kconfig | 6 +
samples/Makefile | 2 +-
samples/whiteegret/Makefile | 14 ++
samples/whiteegret/checkwl.c | 215 +++++++++++++++++
samples/whiteegret/checkwl.h | 33 +++
samples/whiteegret/main.c | 119 ++++++++++
security/Kconfig | 1 +
security/Makefile | 2 +
security/whiteegret/Kconfig | 82 +++++++
security/whiteegret/Makefile | 2 +
security/whiteegret/init.c | 148 ++++++++++++
security/whiteegret/main.c | 466 +++++++++++++++++++++++++++++++++++++
security/whiteegret/request.c | 98 ++++++++
security/whiteegret/request.h | 47 ++++
security/whiteegret/we.h | 72 ++++++
security/whiteegret/we_fs.c | 269 +++++++++++++++++++++
security/whiteegret/we_fs.h | 23 ++
security/whiteegret/we_fs_common.h | 60 +++++
18 files changed, 1658 insertions(+), 1 deletion(-)
create mode 100644 samples/whiteegret/Makefile
create mode 100644 samples/whiteegret/checkwl.c
create mode 100644 samples/whiteegret/checkwl.h
create mode 100644 samples/whiteegret/main.c
create mode 100644 security/whiteegret/Kconfig
create mode 100644 security/whiteegret/Makefile
create mode 100644 security/whiteegret/init.c
create mode 100644 security/whiteegret/main.c
create mode 100644 security/whiteegret/request.c
create mode 100644 security/whiteegret/request.h
create mode 100644 security/whiteegret/we.h
create mode 100644 security/whiteegret/we_fs.c
create mode 100644 security/whiteegret/we_fs.h
create mode 100644 security/whiteegret/we_fs_common.h
--
2.7.4
This is an interesting idea, and an evolution since the initial
approach which was submitted based upon xattr attributes. I still
find the idea of using attributes simpler to manage though, since
they're easy to add, and audit for.
I suspect the biggest objection to this module is that maintaining a
whitelist at all is often impractical.
My (trivial/toy/silly) module went the attribute-reading way:
https://github.com/skx/linux-security-modules/tree/master/security/whitelist
For a completely crazy take upon the idea of userspace decisions
though you can laugh at my attempt here:
https://github.com/skx/linux-security-modules/tree/master/security/can-exec
I callback to userspace for every decision, via
call_usermodehelper_setup. The crazy part is that it actually works
at all!
Steve
On Fri, Oct 19, 2018 at 8:37 AM Shinya Takumi
<[email protected]> wrote:
>
> WhiteEgret is an LSM to simply provide a whitelisting-type
> execution control.
>
> An execution-whitelist, simply called whitelist, is a list
> of executable components (e.g., applications, libraries)
> that are approved to run on a host. The whitelist is used
> to decide whether executable components are permitted to
> execute or not. This mechanism can stop an execution of
> unknown software, so it helps stop the execution of
> malicious code and other unauthorized software.
>
> It is important to maintain a whitelist properly according to
> the execution environments. Managing whitelists for information
> systems is a difficult task because their environments are
> changed frequently. On the other hand, for such devices that
> continue to do the same tasks for a certain period of time,
> we can use the same whitelist for the period once the whitelist
> is established. Thus the whitelisting-type execution control
> works best in such execution environments. Examples of the above
> execution environments include control devices in industrial
> control systems.
>
> Although the number of changing whitelists is not so large,
> it is necessary to change them according to a system life cycle
> or each phase of system operations. There is a requirement to
> change whitelists with the system operations continued because
> they often cannot easily be stopped. For example, such cases
> include temporarily allowing maintenance programs for maintenance
> or troubleshooting purposes while running the systems.
>
> WhiteEgret is aiming at satisfying the above requirement.
> WhiteEgret adopts a model that a whitelist is managed in user space.
> Namely, WhiteEgret assumes that a privileged user manages a whitelist
> in user space. This makes it possible to change the whitelist while
> running the systems.
>
> Mechanism of WhiteEgret
>
> WhiteEgret requires a user application called WhiteEgret User
> Application (WEUA, for short). WhiteEgret utilizes the
> bprm_check_security hook and the mmap_file hook.
> WhiteEgret asks WEUA whether an executable component hooked
> by the above hooks is permitted to execute or not.
> If the response from the WEUA is "permit", then WhiteEgret
> continues to process the executable component. If the response
> is "not permit", then WhiteEgret returns an error and blocks
> the execution of the executable component.
> The bprm_check_security hook is triggered by execve system
> call, so execution by almost all executable components are
> hooked by the hook. However, because shared objects do not
> invoke execve system call, WhiteEgret utilizes the mmap_file
> hook to hook the memory mapping by a shared object.
> Thus WhiteEgret ignores the mmap_file hook caused by
> non-executable and by executable which calls execve system call.
>
> To ask the permission to a WEUA, WhiteEgret sends the
> absolute path, the inode number and the device number of the
> executable component to the WEUA.
> Then the WEUA is expected to work as follows.
> The WEUA sees if the tuple of the absolute path and/or the inode
> number and/or the device number is contained in the whitelist.
> If it exists, the WEUA compares a hash value of the executable
> component indicated by the absolute path (and/or the inode number
> and/or device number) with that in the whitelist to see whether
> the executable component is changed or not after the whitelist is
> made. The WEUA returns "permit" if both tests are passed,
> otherwise returns "not permit".
>
> WhiteEgret v4 is also able to control for script execution. Some
> LSM hooks (file_open/file_permission/task_alloc/task_free) are
> added. Kernel configs are required to enable the hooks.
>
> Most of interpreters open script files to execute. Therefore
> WhiteEgret hooks for reading or opening a file. Then WhiteEgret
> asks the WEUA whether an execution of the script is permitted to
> execute or not. WhiteEgret is able to choose a hook entry for
> execution control between file_open or file_permission.
>
> Hook entries of task_alloc and task_free are used to control
> exections of script effectively. Some interpreters forks child
> processes to execte script files, so the WEUA managed a process
> family of an interpter by bprm_check_security, task_alloc and
> task_free.
>
> To use WhiteEgret
>
> Users have to prepare a whitelist and a WEUA to use WhiteEgret.
> A sample WEUA is involved in samples/whiteegret/.
>
> To enable WhiteEgret, you are required to build the kernel using
> normal procedures with CONFIG_SECURITY_WHITEEGRET=y.
>
> Additionally, SECURITY_WHITEEGRET_INTERPRETER=y option is
> required to enable to control script executions.
> And SECURITY_WHITEEGRET_WRITE=y option is also required to
> detect of writing script files.
>
> The patchset is also available in our github repo:
> https://github.com/whiteegret/whiteegret
>
> ---
> Changes in v4:
> - Add LSM hooks (file_open/file_permission/task_alloc/task_free)
> to control for script execution. Kernel configs are required
> to enable the hooks.
> - Modify the data struct for kernel space and user space
> communication.
>
> Changes in v3:
> - Change to a minor LSM module.
>
> Changes in v2:
> - Change communication method between kernel space and user space
> from netlink to device driver, and device driver utilizes not LKM
> but securityfs.
> - Add inode number and device number to information of executable
> component sent from kernel space to user space.
> - Fix bugs regarding to locks during kernel space and user space
> communication.
> - Change return value from -EPERM to -EACCES when the execution of
> an executable component is denied.
>
> Shinya Takumi (2):
> WhiteEgret: Add WhiteEgret core functions.
> WhiteEgret: Add an example of user application.
>
> samples/Kconfig | 6 +
> samples/Makefile | 2 +-
> samples/whiteegret/Makefile | 14 ++
> samples/whiteegret/checkwl.c | 215 +++++++++++++++++
> samples/whiteegret/checkwl.h | 33 +++
> samples/whiteegret/main.c | 119 ++++++++++
> security/Kconfig | 1 +
> security/Makefile | 2 +
> security/whiteegret/Kconfig | 82 +++++++
> security/whiteegret/Makefile | 2 +
> security/whiteegret/init.c | 148 ++++++++++++
> security/whiteegret/main.c | 466 +++++++++++++++++++++++++++++++++++++
> security/whiteegret/request.c | 98 ++++++++
> security/whiteegret/request.h | 47 ++++
> security/whiteegret/we.h | 72 ++++++
> security/whiteegret/we_fs.c | 269 +++++++++++++++++++++
> security/whiteegret/we_fs.h | 23 ++
> security/whiteegret/we_fs_common.h | 60 +++++
> 18 files changed, 1658 insertions(+), 1 deletion(-)
> create mode 100644 samples/whiteegret/Makefile
> create mode 100644 samples/whiteegret/checkwl.c
> create mode 100644 samples/whiteegret/checkwl.h
> create mode 100644 samples/whiteegret/main.c
> create mode 100644 security/whiteegret/Kconfig
> create mode 100644 security/whiteegret/Makefile
> create mode 100644 security/whiteegret/init.c
> create mode 100644 security/whiteegret/main.c
> create mode 100644 security/whiteegret/request.c
> create mode 100644 security/whiteegret/request.h
> create mode 100644 security/whiteegret/we.h
> create mode 100644 security/whiteegret/we_fs.c
> create mode 100644 security/whiteegret/we_fs.h
> create mode 100644 security/whiteegret/we_fs_common.h
>
> --
> 2.7.4
>
Steve Kemp wrote:
> This is an interesting idea, and an evolution since the initial
> approach which was submitted based upon xattr attributes. I still
> find the idea of using attributes simpler to manage though, since
> they're easy to add, and audit for.
>
> I suspect the biggest objection to this module is that maintaining a
> whitelist at all is often impractical.
If existing implementations were perfect enough for everyone, we would not
have done a lot of trial and error. ;-)
>
> My (trivial/toy/silly) module went the attribute-reading way:
>
> https://github.com/skx/linux-security-modules/tree/master/security/whitelist
>
> For a completely crazy take upon the idea of userspace decisions
> though you can laugh at my attempt here:
>
> https://github.com/skx/linux-security-modules/tree/master/security/can-exec
>
> I callback to userspace for every decision, via
> call_usermodehelper_setup. The crazy part is that it actually works
> at all!
>
> Steve
Oh, checking only execve() requests? I have implemented it (using AKARI as a
codebase) which does on-access ClamAV scan, without using call_usermodehelper().
The tutorial is (written in Japanese language) available at
http://I-love.SAKURA.ne.jp/tomoyo/scamp2017-kumaneko.html . Since WhiteEgret
is using a daemon rather than one-time agent, I'm sure that this example
implementation helps how to make WhiteEgret interface robust.
Here begins the feedback.
(1) Please drop module exit functions, for currently it is impossible to unload
"loadable kernel module based LSM modules".
(2) Please add __init to all init functions, for it helps finding redundant code.
(3) Please test with CONFIG_PROVE_LOCKING=y, for there is a deadlock bug which
lockdep can find.
(4) Please test with CONFIG_KASAN=y, for there is a use-after-free bug.
(5) Please test with CONFIG_DEBUG_ATOMIC_SLEEP=y, for there is a sleep-in-atomic bug.
(6) Please avoid redundant NULL checks.
(7) Please annotate __user to userspace pointers.
(8) Please do check size when copying kernel <=> user memory.
(9) Setting "struct file_operations"->owner is pointless, for currently it is
impossible to unload "loadable kernel module based LSM modules".
(10) Please check the compiler output, for there is an incompatible argument warning.
(11) Please don't try to defer pending signals. If a process got a fatal signal,
the process should be able to terminate as soon as possible.
(12) Please understand when "struct file_operations"->open/release hooks are called,
for current "from_task" approach is not robust (and hence the tutorial above).
A delta diff is shown below, but you don't want to apply it as-is.
diff --git a/security/whiteegret/init.c b/security/whiteegret/init.c
index b78f581..c447655 100644
--- a/security/whiteegret/init.c
+++ b/security/whiteegret/init.c
@@ -23,9 +23,6 @@
static int we_security_bprm_check(struct linux_binprm *bprm)
{
- if (!bprm)
- return 0;
-
if (we_security_bprm_check_main(bprm) == -EACCES)
return -EACCES;
@@ -48,8 +45,6 @@ static int we_security_mmap_check(struct file *file, unsigned long reqprot,
defined(CONFIG_SECURITY_WHITEEGRET_HOOK_FILE_WRITE)
static int we_security_access_check(struct file *file, int mask)
{
- if (!file)
- return 0;
return we_security_access_check_main(file, mask);
}
#endif
@@ -58,23 +53,18 @@ static int we_security_access_check(struct file *file, int mask)
defined(CONFIG_SECURITY_WHITEEGRET_HOOK_WRITE_OPEN)
static int we_security_open_check(struct file *file)
{
- if (!file)
- return 0;
return we_security_open_check_main(file);
}
#endif
#ifdef CONFIG_SECURITY_WHITEEGRET_HOOK_WRITE
-static int we_security_rename_check(struct path *old_dir,
+static int we_security_rename_check(const struct path *old_dir,
struct dentry *old_dentry,
- struct path *new_dir,
+ const struct path *new_dir,
struct dentry *new_dentry)
{
struct path new_path;
- if (!new_dir)
- return 0;
-
new_path.mnt = new_dir->mnt;
new_path.dentry = new_dentry;
return we_security_rename_check_main(&new_path);
@@ -85,17 +75,11 @@ static int we_security_rename_check(struct path *old_dir,
static int we_task_alloc_check(struct task_struct *task,
unsigned long clone_flag)
{
- if (!task)
- return 0;
-
return we_security_task_alloc_check_main(task, clone_flag);
}
static void we_task_free_check(struct task_struct *task)
{
- if (!task)
- return;
-
we_security_task_free_check_main(task);
}
#endif
@@ -137,12 +121,4 @@ static int __init we_init(void)
return 0;
}
-static void __exit we_exit(void)
-{
- we_specific_exit();
-
- pr_warn("WhiteEgret (LSM) exited.\n");
-}
-
module_init(we_init);
-module_exit(we_exit);
diff --git a/security/whiteegret/main.c b/security/whiteegret/main.c
index cc531d0..8e895b9 100644
--- a/security/whiteegret/main.c
+++ b/security/whiteegret/main.c
@@ -42,7 +42,7 @@ static int send_receive_we_obj_info(
*
* Returns 0.
*/
-int we_specific_init(void)
+int __init we_specific_init(void)
{
int rc = 0;
@@ -53,20 +53,6 @@ int we_specific_init(void)
}
we_req_q_head_init();
-#ifdef CONFIG_SECURITY_WHITEEGRET_CHECK_LIVING_TASK
- root_we_obj_info = NULL;
-#endif
-
- return 0;
-}
-
-/**
- * we_specific_exit - Nothing to do in the implementation.
- *
- * Returns 0.
- */
-int we_specific_exit(void)
-{
return 0;
}
@@ -93,12 +79,9 @@ static int we_get_path(struct path *path,
char **ret_pathname, char **ret_pathnamebuf)
{
char *pathname = NULL, *pathnamebuf = NULL;
- int pathsize = PAGE_SIZE;
+ const int pathsize = PAGE_SIZE;
int rc = 0;
- if (!path || !path->dentry)
- goto failure;
-
pathnamebuf = kmalloc(pathsize, GFP_KERNEL);
if (unlikely(!pathnamebuf)) {
rc = -ENOMEM;
@@ -395,17 +378,17 @@ int we_security_task_alloc_check_main(struct task_struct *task,
* the WhiteEgret User Application.
*/
while (1) {
- write_lock(&root_we_obj_info_lock);
+ write_lock_irq(&root_we_obj_info_lock);
if (root_we_obj_info) {
node = root_we_obj_info;
root_we_obj_info = root_we_obj_info->next;
- write_unlock(&root_we_obj_info_lock);
+ write_unlock_irq(&root_we_obj_info_lock);
if (likely(from_task))
rc = send_receive_we_obj_info
(&node->we_obj_info, &checkresult);
kfree(node);
} else {
- write_unlock(&root_we_obj_info_lock);
+ write_unlock_irq(&root_we_obj_info_lock);
break;
}
}
@@ -435,11 +418,12 @@ int we_security_task_alloc_check_main(struct task_struct *task,
void we_security_task_free_check_main(struct task_struct *task)
{
struct we_obj_info_stack *node;
+ unsigned long flags;
if (unlikely(!from_task) || from_task == task)
return;
- if (get_nr_threads(task) > 1)
+ if (get_nr_threads(task) > 1) // Bad access; use-after-free.
return;
node = kzalloc(sizeof(*node), GFP_ATOMIC);
@@ -458,9 +442,9 @@ void we_security_task_free_check_main(struct task_struct *task)
* Notifying infomation is exit process infomation, not include
* file information.
*/
- write_lock(&root_we_obj_info_lock);
+ write_lock_irqsave(&root_we_obj_info_lock, flags);
node->next = root_we_obj_info;
root_we_obj_info = node;
- write_unlock(&root_we_obj_info_lock);
+ write_unlock_irqrestore(&root_we_obj_info_lock, flags);
}
#endif
diff --git a/security/whiteegret/request.c b/security/whiteegret/request.c
index 8d230cb..0b6f48b 100644
--- a/security/whiteegret/request.c
+++ b/security/whiteegret/request.c
@@ -19,7 +19,7 @@
*
* Returns 0.
*/
-int we_req_q_head_init(void)
+int __init we_req_q_head_init(void)
{
rwlock_init(&(we_q_head.lock));
INIT_LIST_HEAD(&(we_q_head.head));
diff --git a/security/whiteegret/request.h b/security/whiteegret/request.h
index c205c4c..273a19f 100644
--- a/security/whiteegret/request.h
+++ b/security/whiteegret/request.h
@@ -40,7 +40,7 @@ struct we_req_q {
int we_req_q_pop(struct we_req_q *queue);
int we_req_q_cleanup(void);
-int we_req_q_head_init(void);
+int __init we_req_q_head_init(void);
int we_req_q_init(struct we_req_q *req, struct we_obj_info *info);
int we_req_q_push(struct we_req_q *queue);
diff --git a/security/whiteegret/we.h b/security/whiteegret/we.h
index e8f067c..7812242 100644
--- a/security/whiteegret/we.h
+++ b/security/whiteegret/we.h
@@ -66,7 +66,6 @@ int we_security_task_alloc_check_main(struct task_struct *task,
unsigned long clone_flags);
void we_security_task_free_check_main(struct task_struct *task);
-int we_specific_init(void);
-int we_specific_exit(void);
+int __init we_specific_init(void);
#endif /* _WE_H */
diff --git a/security/whiteegret/we_fs.c b/security/whiteegret/we_fs.c
index 57f77aa..af7c3f6 100644
--- a/security/whiteegret/we_fs.c
+++ b/security/whiteegret/we_fs.c
@@ -16,6 +16,7 @@
#include <linux/wait.h>
#include <linux/sched.h>
#include <linux/uaccess.h>
+#include <linux/sched/task.h>
#include "we_fs.h"
#include "we.h"
@@ -31,7 +32,7 @@ static int check_we_pathsize(struct we_req_q *we_req, int size)
return -1;
}
-static int set_we_req_info(struct we_req_user *user,
+static int set_we_req_info(struct we_req_user __user *user,
struct we_obj_info *info)
{
unsigned long ret;
@@ -49,7 +50,7 @@ static int set_we_req_info(struct we_req_user *user,
return 0;
}
-static int set_we_ack(struct we_ack *to, struct we_ack *from)
+static int set_we_ack(struct we_ack *to, const void __user *from)
{
unsigned long ret;
@@ -60,12 +61,12 @@ static int set_we_ack(struct we_ack *to, struct we_ack *from)
return 0;
}
-static struct we_req_user *get_alive_we_req(void *buf, int size)
+static struct we_req_user __user *get_alive_we_req(void __user *buf, int size)
{
int pathsize;
struct list_head *p;
struct we_req_q *req;
- struct we_req_user *user = NULL;
+ struct we_req_user __user *user = NULL;
write_lock(&we_q_head.lock);
list_for_each(p, &we_q_head.head) {
@@ -73,8 +74,8 @@ static struct we_req_user *get_alive_we_req(void *buf, int size)
if (req->finish_flag == STOP_EXEC) {
if (unlikely(check_we_pathsize(req, size)))
goto SIZE_ERROR;
- user = (struct we_req_user *)buf;
- set_we_req_info(user, req->we_obj_info);
+ user = (struct we_req_user __user *)buf;
+ set_we_req_info(user, req->we_obj_info); // Bad access; sleep-in-atomic.
break;
}
}
@@ -117,7 +118,7 @@ static ssize_t send_ack(struct we_ack *ack)
return sizeof(*ack);
}
-static ssize_t we_driver_read(struct file *file, char *buf,
+static ssize_t we_driver_read(struct file *file, char __user *buf,
size_t size, loff_t *off)
{
int ret;
@@ -137,14 +138,16 @@ static ssize_t we_driver_read(struct file *file, char *buf,
return 1;
}
-static ssize_t we_driver_write(struct file *file, const char *buf,
+static ssize_t we_driver_write(struct file *file, const char __user *buf,
size_t size, loff_t *off)
{
int rc;
ssize_t ret;
struct we_ack ack;
- rc = set_we_ack(&ack, (struct we_ack *)((void *)buf));
+ if (size != sizeof(ack))
+ return -EINVAL;
+ rc = set_we_ack(&ack, buf);
if (rc < 0)
return (ssize_t)rc;
ret = send_ack(&ack);
@@ -186,6 +189,7 @@ static long we_driver_ioctl(struct file *file,
static int we_driver_release(struct inode *inode, struct file *filp)
{
int ret = 0;
+ struct task_struct *task = NULL;
write_lock(&from_task_lock);
if (!from_task) {
@@ -193,21 +197,26 @@ static int we_driver_release(struct inode *inode, struct file *filp)
ret = -EACCES;
goto END;
}
+ // Bad assumption; fork() etc. can make this condition false.
if (from_task != current) {
pr_warn("This task is not registered to WhiteEgret.\n");
ret = -EACCES;
goto END;
}
+ task = from_task;
from_task = NULL;
we_req_q_cleanup();
END:
write_unlock(&from_task_lock);
+ if (task)
+ put_task_struct(task);
return ret;
}
static int we_driver_open(struct inode *inode, struct file *filp)
{
write_lock(&from_task_lock);
+ // Bad assumption; fork() etc. does not hit this condition.
if (from_task) {
write_unlock(&(from_task_lock));
pr_warn("WhiteEgret has already started.\n");
@@ -215,13 +224,13 @@ static int we_driver_open(struct inode *inode, struct file *filp)
}
from_task = current;
+ get_task_struct(from_task);
write_unlock(&from_task_lock);
return 0;
}
static const struct file_operations we_driver_fops = {
- .owner = THIS_MODULE,
.read = we_driver_read,
.write = we_driver_write,
.unlocked_ioctl = we_driver_ioctl,
@@ -229,7 +238,7 @@ static int we_driver_open(struct inode *inode, struct file *filp)
.release = we_driver_release,
};
-int we_fs_init(void)
+int __init we_fs_init(void)
{
struct dentry *we_dir;
struct dentry *wecom;
diff --git a/security/whiteegret/we_fs.h b/security/whiteegret/we_fs.h
index ffb7775..f3d69bc 100644
--- a/security/whiteegret/we_fs.h
+++ b/security/whiteegret/we_fs.h
@@ -17,7 +17,7 @@
extern struct task_struct *from_task;
-int we_fs_init(void);
+int __init we_fs_init(void);
int send_we_obj_info(struct we_req_q *req);
#endif /* _WE_FS_H */
Regarding (4), you can comfirm that get_nr_threads(task) returns 0x6b6b6b6b, using
below demo patch and reproducer. When copy_process() failed after copy_signal()
succeeded, security_task_free() is called after free_signal_struct() is called.
diff --git a/kernel/fork.c b/kernel/fork.c
index f0b5847..11714dd 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1939,6 +1939,12 @@ static __latent_entropy struct task_struct *copy_process(
p->task_works = NULL;
cgroup_threadgroup_change_begin(current);
+
+ if (!strcmp(current->comm, "a.out")) {
+ retval = -ENOMEM;
+ goto bad_fork_free_pid;
+ }
+
/*
* Ensure that the cgroup subsystem policies allow the new process to be
* forked. It should be noted the the new process's css_set can be changed
---------- source code of a.out ----------
#include <unistd.h>
int main(int argc, char *argv[])
{
if (fork() == 0)
_exit(0);
return 0;
}
---------- source code of a.out ----------
Regarding (12), you can comfirm that "from_task" logic is broken, using below
reproducer. Since "struct file_operations"->release hook is called when refcount
becomes 0, it is not always the thread who called "struct file_operations"->open
hook. The thread which "from_task" remembers can terminate before "from_task" is
reset to NULL. The kernel side has to be prepared for such situation. You can
hold a refcount using get_task_struct()/put_task_struct(), but "from_task" logic
is still fragile. You need to reconsider how to distinguish requests from
WhiteEgret User Application.
---------- source code of b.out ----------
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
int main(int argc, char *argv[])
{
if (open("/sys/kernel/security/whiteegret/wecom", O_RDWR) < 0)
return 1;
if (fork() == 0) {
sleep(1);
_exit(0);
}
return 0;
}
---------- source code of b.out ----------
Steve Kemp wrote:
> This is an interesting idea, and an evolution since the initial
> approach which was submitted based upon xattr attributes. I still
> find the idea of using attributes simpler to manage though, since
> they're easy to add, and audit for.
>
> I suspect the biggest objection to this module is that maintaining a
> whitelist at all is often impractical.
We also consider that it is an important point of view.
We seem that it is easy to control of executions by file path rather than xattr attributes.
Because the WEUA can easily get file information by file path in user space.
For example, in industrial control systems (ICS), the frequency of software updates are rarer than information systems.
The maintenance cost of whitelist is low in such systems.
Following the NIST guideline describes importance of the whitelist execution control technology for ICS.
https://nvlpubs.nist.gov/nistpubs/specialpublications/nist.sp.800-167.pdf
Moreover, there are various requirements depending on ICS.
WhiteEgret users can implement the WEUA which is specialized for their own ICS.
I guess that maintenance cost of whitelist can be decreased in ICS.
>
> My (trivial/toy/silly) module went the attribute-reading way:
>
> https://github.com/skx/linux-security-modules/tree/master/security/whi
> telist
>
> For a completely crazy take upon the idea of userspace decisions
> though you can laugh at my attempt here:
>
> https://github.com/skx/linux-security-modules/tree/master/security/can
> -exec
>
> I callback to userspace for every decision, via
> call_usermodehelper_setup. The crazy part is that it actually works
> at all!
>
> Steve
Takumi Shinya
We appreciate your comments.
We refine source code according to your comments.
>> This is an interesting idea, and an evolution since the initial
>> approach which was submitted based upon xattr attributes. I still
>> find the idea of using attributes simpler to manage though, since
>> they're easy to add, and audit for.
>>
>> I suspect the biggest objection to this module is that maintaining a
>> whitelist at all is often impractical.
>
>If existing implementations were perfect enough for everyone, we would not
>have done a lot of trial and error. ;-)
>
>>
>> My (trivial/toy/silly) module went the attribute-reading way:
>>
>> https://github.com/skx/linux-security-modules/tree/master/security/whi
>> telist
>>
>> For a completely crazy take upon the idea of userspace decisions
>> though you can laugh at my attempt here:
>>
>> https://github.com/skx/linux-security-modules/tree/master/security/can
>> -exec
>>
>> I callback to userspace for every decision, via
>> call_usermodehelper_setup. The crazy part is that it actually works
>> at all!
>>
>> Steve
>
>Oh, checking only execve() requests? I have implemented it (using AKARI as a
>codebase) which does on-access ClamAV scan, without using
>call_usermodehelper().
>The tutorial is (written in Japanese language) available at
>http://I-love.SAKURA.ne.jp/tomoyo/scamp2017-kumaneko.html . Since
>WhiteEgret is using a daemon rather than one-time agent, I'm sure that this
>example implementation helps how to make WhiteEgret interface robust.
Thank you for introducing useful information.
>
>
>
>Here begins the feedback.
>
>(1) Please drop module exit functions, for currently it is impossible to unload
> "loadable kernel module based LSM modules".
>
>(2) Please add __init to all init functions, for it helps finding redundant code.
>
>(3) Please test with CONFIG_PROVE_LOCKING=y, for there is a deadlock bug
>which
> lockdep can find.
>
>(4) Please test with CONFIG_KASAN=y, for there is a use-after-free bug.
>
>(5) Please test with CONFIG_DEBUG_ATOMIC_SLEEP=y, for there is a
>sleep-in-atomic bug.
We use to fix bugs with these kernel options.
>
>(6) Please avoid redundant NULL checks.
>
>(7) Please annotate __user to userspace pointers.
>
>(8) Please do check size when copying kernel <=> user memory.
>
>(9) Setting "struct file_operations"->owner is pointless, for currently it is
> impossible to unload "loadable kernel module based LSM modules".
>
>(10) Please check the compiler output, for there is an incompatible argument
>warning.
>
>(11) Please don't try to defer pending signals. If a process got a fatal signal,
> the process should be able to terminate as soon as possible.
We concern whether the LSM can restart system calls in any situation.
We study behavior of restarting system calls, and we reconsider way of handling signals.
>
>(12) Please understand when "struct file_operations"->open/release hooks are
>called,
> for current "from_task" approach is not robust (and hence the tutorial
>above).
We improve our LSM implementation to be compatible with various WEUA implementations.
>
>
>
>A delta diff is shown below, but you don't want to apply it as-is.
>
>diff --git a/security/whiteegret/init.c b/security/whiteegret/init.c index
>b78f581..c447655 100644
>--- a/security/whiteegret/init.c
>+++ b/security/whiteegret/init.c
>@@ -23,9 +23,6 @@
>
> static int we_security_bprm_check(struct linux_binprm *bprm) {
>- if (!bprm)
>- return 0;
>-
> if (we_security_bprm_check_main(bprm) == -EACCES)
> return -EACCES;
>
>@@ -48,8 +45,6 @@ static int we_security_mmap_check(struct file *file,
>unsigned long reqprot,
> defined(CONFIG_SECURITY_WHITEEGRET_HOOK_FILE_WRITE)
> static int we_security_access_check(struct file *file, int mask) {
>- if (!file)
>- return 0;
> return we_security_access_check_main(file, mask); } #endif @@
>-58,23 +53,18 @@ static int we_security_access_check(struct file *file, int
>mask)
> defined(CONFIG_SECURITY_WHITEEGRET_HOOK_WRITE_OPEN)
> static int we_security_open_check(struct file *file) {
>- if (!file)
>- return 0;
> return we_security_open_check_main(file);
> }
> #endif
>
> #ifdef CONFIG_SECURITY_WHITEEGRET_HOOK_WRITE
>-static int we_security_rename_check(struct path *old_dir,
>+static int we_security_rename_check(const struct path *old_dir,
> struct dentry *old_dentry,
>- struct path *new_dir,
>+ const struct path *new_dir,
> struct dentry *new_dentry)
> {
> struct path new_path;
>
>- if (!new_dir)
>- return 0;
>-
> new_path.mnt = new_dir->mnt;
> new_path.dentry = new_dentry;
> return we_security_rename_check_main(&new_path);
>@@ -85,17 +75,11 @@ static int we_security_rename_check(struct path
>*old_dir, static int we_task_alloc_check(struct task_struct *task,
> unsigned long clone_flag)
> {
>- if (!task)
>- return 0;
>-
> return we_security_task_alloc_check_main(task, clone_flag); }
>
> static void we_task_free_check(struct task_struct *task) {
>- if (!task)
>- return;
>-
> we_security_task_free_check_main(task);
> }
> #endif
>@@ -137,12 +121,4 @@ static int __init we_init(void)
> return 0;
> }
>
>-static void __exit we_exit(void)
>-{
>- we_specific_exit();
>-
>- pr_warn("WhiteEgret (LSM) exited.\n");
>-}
>-
> module_init(we_init);
>-module_exit(we_exit);
>diff --git a/security/whiteegret/main.c b/security/whiteegret/main.c index
>cc531d0..8e895b9 100644
>--- a/security/whiteegret/main.c
>+++ b/security/whiteegret/main.c
>@@ -42,7 +42,7 @@ static int send_receive_we_obj_info(
> *
> * Returns 0.
> */
>-int we_specific_init(void)
>+int __init we_specific_init(void)
> {
> int rc = 0;
>
>@@ -53,20 +53,6 @@ int we_specific_init(void)
> }
> we_req_q_head_init();
>
>-#ifdef CONFIG_SECURITY_WHITEEGRET_CHECK_LIVING_TASK
>- root_we_obj_info = NULL;
>-#endif
>-
>- return 0;
>-}
>-
>-/**
>- * we_specific_exit - Nothing to do in the implementation.
>- *
>- * Returns 0.
>- */
>-int we_specific_exit(void)
>-{
> return 0;
> }
>
>@@ -93,12 +79,9 @@ static int we_get_path(struct path *path,
> char **ret_pathname, char **ret_pathnamebuf) {
> char *pathname = NULL, *pathnamebuf = NULL;
>- int pathsize = PAGE_SIZE;
>+ const int pathsize = PAGE_SIZE;
> int rc = 0;
>
>- if (!path || !path->dentry)
>- goto failure;
>-
> pathnamebuf = kmalloc(pathsize, GFP_KERNEL);
> if (unlikely(!pathnamebuf)) {
> rc = -ENOMEM;
>@@ -395,17 +378,17 @@ int we_security_task_alloc_check_main(struct
>task_struct *task,
> * the WhiteEgret User Application.
> */
> while (1) {
>- write_lock(&root_we_obj_info_lock);
>+ write_lock_irq(&root_we_obj_info_lock);
> if (root_we_obj_info) {
> node = root_we_obj_info;
> root_we_obj_info = root_we_obj_info->next;
>- write_unlock(&root_we_obj_info_lock);
>+ write_unlock_irq(&root_we_obj_info_lock);
> if (likely(from_task))
> rc = send_receive_we_obj_info
> (&node->we_obj_info,
>&checkresult);
> kfree(node);
> } else {
>- write_unlock(&root_we_obj_info_lock);
>+ write_unlock_irq(&root_we_obj_info_lock);
> break;
> }
> }
>@@ -435,11 +418,12 @@ int we_security_task_alloc_check_main(struct
>task_struct *task, void we_security_task_free_check_main(struct task_struct
>*task) {
> struct we_obj_info_stack *node;
>+ unsigned long flags;
>
> if (unlikely(!from_task) || from_task == task)
> return;
>
>- if (get_nr_threads(task) > 1)
>+ if (get_nr_threads(task) > 1) // Bad access; use-after-free.
> return;
>
> node = kzalloc(sizeof(*node), GFP_ATOMIC); @@ -458,9 +442,9 @@
>void we_security_task_free_check_main(struct task_struct *task)
> * Notifying infomation is exit process infomation, not include
> * file information.
> */
>- write_lock(&root_we_obj_info_lock);
>+ write_lock_irqsave(&root_we_obj_info_lock, flags);
> node->next = root_we_obj_info;
> root_we_obj_info = node;
>- write_unlock(&root_we_obj_info_lock);
>+ write_unlock_irqrestore(&root_we_obj_info_lock, flags);
> }
> #endif
>diff --git a/security/whiteegret/request.c b/security/whiteegret/request.c
>index 8d230cb..0b6f48b 100644
>--- a/security/whiteegret/request.c
>+++ b/security/whiteegret/request.c
>@@ -19,7 +19,7 @@
> *
> * Returns 0.
> */
>-int we_req_q_head_init(void)
>+int __init we_req_q_head_init(void)
> {
> rwlock_init(&(we_q_head.lock));
> INIT_LIST_HEAD(&(we_q_head.head));
>diff --git a/security/whiteegret/request.h b/security/whiteegret/request.h
>index c205c4c..273a19f 100644
>--- a/security/whiteegret/request.h
>+++ b/security/whiteegret/request.h
>@@ -40,7 +40,7 @@ struct we_req_q {
> int we_req_q_pop(struct we_req_q *queue); int we_req_q_cleanup(void);
>
>-int we_req_q_head_init(void);
>+int __init we_req_q_head_init(void);
> int we_req_q_init(struct we_req_q *req, struct we_obj_info *info); int
>we_req_q_push(struct we_req_q *queue);
>
>diff --git a/security/whiteegret/we.h b/security/whiteegret/we.h index
>e8f067c..7812242 100644
>--- a/security/whiteegret/we.h
>+++ b/security/whiteegret/we.h
>@@ -66,7 +66,6 @@ int we_security_task_alloc_check_main(struct task_struct
>*task,
> unsigned long clone_flags);
> void we_security_task_free_check_main(struct task_struct *task);
>
>-int we_specific_init(void);
>-int we_specific_exit(void);
>+int __init we_specific_init(void);
>
> #endif /* _WE_H */
>diff --git a/security/whiteegret/we_fs.c b/security/whiteegret/we_fs.c index
>57f77aa..af7c3f6 100644
>--- a/security/whiteegret/we_fs.c
>+++ b/security/whiteegret/we_fs.c
>@@ -16,6 +16,7 @@
> #include <linux/wait.h>
> #include <linux/sched.h>
> #include <linux/uaccess.h>
>+#include <linux/sched/task.h>
> #include "we_fs.h"
> #include "we.h"
>
>@@ -31,7 +32,7 @@ static int check_we_pathsize(struct we_req_q *we_req, int
>size)
> return -1;
> }
>
>-static int set_we_req_info(struct we_req_user *user,
>+static int set_we_req_info(struct we_req_user __user *user,
> struct we_obj_info *info)
> {
> unsigned long ret;
>@@ -49,7 +50,7 @@ static int set_we_req_info(struct we_req_user *user,
> return 0;
> }
>
>-static int set_we_ack(struct we_ack *to, struct we_ack *from)
>+static int set_we_ack(struct we_ack *to, const void __user *from)
> {
> unsigned long ret;
>
>@@ -60,12 +61,12 @@ static int set_we_ack(struct we_ack *to, struct we_ack
>*from)
> return 0;
> }
>
>-static struct we_req_user *get_alive_we_req(void *buf, int size)
>+static struct we_req_user __user *get_alive_we_req(void __user *buf,
>+int size)
> {
> int pathsize;
> struct list_head *p;
> struct we_req_q *req;
>- struct we_req_user *user = NULL;
>+ struct we_req_user __user *user = NULL;
>
> write_lock(&we_q_head.lock);
> list_for_each(p, &we_q_head.head) {
>@@ -73,8 +74,8 @@ static struct we_req_user *get_alive_we_req(void *buf, int
>size)
> if (req->finish_flag == STOP_EXEC) {
> if (unlikely(check_we_pathsize(req, size)))
> goto SIZE_ERROR;
>- user = (struct we_req_user *)buf;
>- set_we_req_info(user, req->we_obj_info);
>+ user = (struct we_req_user __user *)buf;
>+ set_we_req_info(user, req->we_obj_info); // Bad
>access; sleep-in-atomic.
> break;
> }
> }
>@@ -117,7 +118,7 @@ static ssize_t send_ack(struct we_ack *ack)
> return sizeof(*ack);
> }
>
>-static ssize_t we_driver_read(struct file *file, char *buf,
>+static ssize_t we_driver_read(struct file *file, char __user *buf,
> size_t size, loff_t *off)
> {
> int ret;
>@@ -137,14 +138,16 @@ static ssize_t we_driver_read(struct file *file, char
>*buf,
> return 1;
> }
>
>-static ssize_t we_driver_write(struct file *file, const char *buf,
>+static ssize_t we_driver_write(struct file *file, const char __user
>+*buf,
> size_t size, loff_t *off)
> {
> int rc;
> ssize_t ret;
> struct we_ack ack;
>
>- rc = set_we_ack(&ack, (struct we_ack *)((void *)buf));
>+ if (size != sizeof(ack))
>+ return -EINVAL;
>+ rc = set_we_ack(&ack, buf);
> if (rc < 0)
> return (ssize_t)rc;
> ret = send_ack(&ack);
>@@ -186,6 +189,7 @@ static long we_driver_ioctl(struct file *file, static int
>we_driver_release(struct inode *inode, struct file *filp) {
> int ret = 0;
>+ struct task_struct *task = NULL;
>
> write_lock(&from_task_lock);
> if (!from_task) {
>@@ -193,21 +197,26 @@ static int we_driver_release(struct inode *inode, struct
>file *filp)
> ret = -EACCES;
> goto END;
> }
>+ // Bad assumption; fork() etc. can make this condition false.
> if (from_task != current) {
> pr_warn("This task is not registered to WhiteEgret.\n");
> ret = -EACCES;
> goto END;
> }
>+ task = from_task;
> from_task = NULL;
> we_req_q_cleanup();
> END:
> write_unlock(&from_task_lock);
>+ if (task)
>+ put_task_struct(task);
> return ret;
> }
>
> static int we_driver_open(struct inode *inode, struct file *filp) {
> write_lock(&from_task_lock);
>+ // Bad assumption; fork() etc. does not hit this condition.
> if (from_task) {
> write_unlock(&(from_task_lock));
> pr_warn("WhiteEgret has already started.\n"); @@ -215,13
>+224,13 @@ static int we_driver_open(struct inode *inode, struct file *filp)
> }
>
> from_task = current;
>+ get_task_struct(from_task);
> write_unlock(&from_task_lock);
>
> return 0;
> }
>
> static const struct file_operations we_driver_fops = {
>- .owner = THIS_MODULE,
> .read = we_driver_read,
> .write = we_driver_write,
> .unlocked_ioctl = we_driver_ioctl,
>@@ -229,7 +238,7 @@ static int we_driver_open(struct inode *inode, struct file
>*filp)
> .release = we_driver_release,
> };
>
>-int we_fs_init(void)
>+int __init we_fs_init(void)
> {
> struct dentry *we_dir;
> struct dentry *wecom;
>diff --git a/security/whiteegret/we_fs.h b/security/whiteegret/we_fs.h index
>ffb7775..f3d69bc 100644
>--- a/security/whiteegret/we_fs.h
>+++ b/security/whiteegret/we_fs.h
>@@ -17,7 +17,7 @@
>
> extern struct task_struct *from_task;
>
>-int we_fs_init(void);
>+int __init we_fs_init(void);
> int send_we_obj_info(struct we_req_q *req);
>
> #endif /* _WE_FS_H */
>
>
>
>
>
>
>
>Regarding (4), you can comfirm that get_nr_threads(task) returns 0x6b6b6b6b,
>using below demo patch and reproducer. When copy_process() failed after
>copy_signal() succeeded, security_task_free() is called after
>free_signal_struct() is called.
We implement another method to detect a process termination by our LSM.
>
>diff --git a/kernel/fork.c b/kernel/fork.c index f0b5847..11714dd 100644
>--- a/kernel/fork.c
>+++ b/kernel/fork.c
>@@ -1939,6 +1939,12 @@ static __latent_entropy struct task_struct
>*copy_process(
> p->task_works = NULL;
>
> cgroup_threadgroup_change_begin(current);
>+
>+ if (!strcmp(current->comm, "a.out")) {
>+ retval = -ENOMEM;
>+ goto bad_fork_free_pid;
>+ }
>+
> /*
> * Ensure that the cgroup subsystem policies allow the new process to
>be
> * forked. It should be noted the the new process's css_set can be
>changed
>
>---------- source code of a.out ---------- #include <unistd.h>
>
>int main(int argc, char *argv[])
>{
> if (fork() == 0)
> _exit(0);
> return 0;
>}
>---------- source code of a.out ----------
>
>
>
>Regarding (12), you can comfirm that "from_task" logic is broken, using below
>reproducer. Since "struct file_operations"->release hook is called when
>refcount becomes 0, it is not always the thread who called "struct
>file_operations"->open hook. The thread which "from_task" remembers can
>terminate before "from_task" is reset to NULL. The kernel side has to be
>prepared for such situation. You can hold a refcount using
>get_task_struct()/put_task_struct(), but "from_task" logic is still fragile. You
>need to reconsider how to distinguish requests from WhiteEgret User
>Application.
>
>---------- source code of b.out ---------- #include <sys/types.h> #include
><sys/stat.h> #include <fcntl.h> #include <unistd.h>
>
>int main(int argc, char *argv[])
>{
> if (open("/sys/kernel/security/whiteegret/wecom", O_RDWR) < 0)
> return 1;
> if (fork() == 0) {
> sleep(1);
> _exit(0);
> }
> return 0;
>}
>---------- source code of b.out ----------