Prevent a task from opening, in "write" mode, any /proc/*/mem
file that operates on the task's mm.
A process could use it to overwrite read-only memory, bypassing
S.A.R.A. restrictions.
Signed-off-by: Salvatore Mesoraca <[email protected]>
---
security/sara/include/sara_data.h | 18 ++++++++++++++++-
security/sara/sara_data.c | 8 ++++++++
security/sara/wxprot.c | 41 +++++++++++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/security/sara/include/sara_data.h b/security/sara/include/sara_data.h
index 9216c47..ee95f74 100644
--- a/security/sara/include/sara_data.h
+++ b/security/sara/include/sara_data.h
@@ -15,6 +15,7 @@
#define __SARA_DATA_H
#include <linux/cred.h>
+#include <linux/fs.h>
#include <linux/init.h>
#include <linux/lsm_hooks.h>
#include <linux/spinlock.h>
@@ -40,6 +41,10 @@ struct sara_shm_data {
spinlock_t lock;
};
+struct sara_inode_data {
+ struct task_struct *task;
+};
+
static inline struct sara_data *get_sara_data(const struct cred *cred)
{
@@ -79,6 +84,17 @@ static inline struct sara_shm_data *get_sara_shm_data(
#define lock_sara_shm(X) (spin_lock(&get_sara_shm_data((X))->lock))
#define unlock_sara_shm(X) (spin_unlock(&get_sara_shm_data((X))->lock))
-#endif
+
+static inline struct sara_inode_data *get_sara_inode_data(
+ const struct inode *inode)
+{
+ if (unlikely(!inode->i_security))
+ return NULL;
+ return inode->i_security + sara_blob_sizes.lbs_inode;
+}
+
+#define get_sara_inode_task(X) (get_sara_inode_data((X))->task)
+
+#endif /* CONFIG_SECURITY_SARA_WXPROT */
#endif /* __SARA_H */
diff --git a/security/sara/sara_data.c b/security/sara/sara_data.c
index 9afca37..e875cf0 100644
--- a/security/sara/sara_data.c
+++ b/security/sara/sara_data.c
@@ -17,6 +17,7 @@
#include <linux/cred.h>
#include <linux/lsm_hooks.h>
#include <linux/mm.h>
+#include <linux/slab.h>
#include <linux/spinlock.h>
static int sara_cred_prepare(struct cred *new, const struct cred *old,
@@ -40,15 +41,22 @@ static int sara_shm_alloc_security(struct kern_ipc_perm *shp)
return 0;
}
+static void sara_task_to_inode(struct task_struct *t, struct inode *i)
+{
+ get_sara_inode_task(i) = t;
+}
+
static struct security_hook_list data_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(cred_prepare, sara_cred_prepare),
LSM_HOOK_INIT(cred_transfer, sara_cred_transfer),
LSM_HOOK_INIT(shm_alloc_security, sara_shm_alloc_security),
+ LSM_HOOK_INIT(task_to_inode, sara_task_to_inode),
};
struct lsm_blob_sizes sara_blob_sizes __lsm_ro_after_init = {
.lbs_cred = sizeof(struct sara_data),
.lbs_ipc = sizeof(struct sara_shm_data),
+ .lbs_inode = sizeof(struct sara_inode_data),
};
int __init sara_data_init(void)
diff --git a/security/sara/wxprot.c b/security/sara/wxprot.c
index 773d1fd..1a8d132 100644
--- a/security/sara/wxprot.c
+++ b/security/sara/wxprot.c
@@ -22,8 +22,11 @@
#include <linux/mm.h>
#include <linux/mman.h>
#include <linux/module.h>
+#include <linux/mount.h>
#include <linux/printk.h>
#include <linux/ratelimit.h>
+#include <linux/sched/mm.h>
+#include <linux/sched/task.h>
#include <linux/spinlock.h>
#include <linux/xattr.h>
@@ -615,6 +618,43 @@ static int sara_file_mprotect(struct vm_area_struct *vma,
return 0;
}
+static int sara_file_open(struct file *file)
+{
+ struct task_struct *t;
+ struct mm_struct *mm;
+ u16 sara_wxp_flags = get_current_sara_wxp_flags();
+
+ /*
+ * Prevent write access to /proc/.../mem
+ * if it operates on the mm_struct of the
+ * current process: it could be used to
+ * bypass W^X.
+ */
+
+ if (!sara_enabled ||
+ !wxprot_enabled ||
+ !(sara_wxp_flags & SARA_WXP_WXORX) ||
+ !(file->f_mode & FMODE_WRITE))
+ return 0;
+
+ t = get_sara_inode_task(file_inode(file));
+ if (unlikely(t != NULL &&
+ strcmp(file->f_path.dentry->d_name.name,
+ "mem") == 0)) {
+ get_task_struct(t);
+ mm = get_task_mm(t);
+ put_task_struct(t);
+ if (unlikely(mm == current->mm))
+ sara_warn_or_goto(error,
+ "write access to /proc/*/mem");
+ mmput(mm);
+ }
+ return 0;
+error:
+ mmput(mm);
+ return -EACCES;
+}
+
#ifdef CONFIG_SECURITY_SARA_WXPROT_EMUTRAMP
static int sara_pagefault_handler(struct pt_regs *regs,
unsigned long error_code,
@@ -778,6 +818,7 @@ static int sara_setprocattr(const char *name, void *value, size_t size)
LSM_HOOK_INIT(check_vmflags, sara_check_vmflags),
LSM_HOOK_INIT(shm_shmat, sara_shm_shmat),
LSM_HOOK_INIT(file_mprotect, sara_file_mprotect),
+ LSM_HOOK_INIT(file_open, sara_file_open),
#ifdef CONFIG_SECURITY_SARA_WXPROT_EMUTRAMP
LSM_HOOK_INIT(pagefault_handler, sara_pagefault_handler),
#endif
--
1.9.1
On Sat, Jul 6, 2019 at 12:55 PM Salvatore Mesoraca
<[email protected]> wrote:
> Prevent a task from opening, in "write" mode, any /proc/*/mem
> file that operates on the task's mm.
> A process could use it to overwrite read-only memory, bypassing
> S.A.R.A. restrictions.
[...]
> +static void sara_task_to_inode(struct task_struct *t, struct inode *i)
> +{
> + get_sara_inode_task(i) = t;
This looks bogus. Nothing is actually holding a reference to `t` here, right?
> +}
> +
> static struct security_hook_list data_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(cred_prepare, sara_cred_prepare),
> LSM_HOOK_INIT(cred_transfer, sara_cred_transfer),
> LSM_HOOK_INIT(shm_alloc_security, sara_shm_alloc_security),
> + LSM_HOOK_INIT(task_to_inode, sara_task_to_inode),
> };
[...]
> +static int sara_file_open(struct file *file)
> +{
> + struct task_struct *t;
> + struct mm_struct *mm;
> + u16 sara_wxp_flags = get_current_sara_wxp_flags();
> +
> + /*
> + * Prevent write access to /proc/.../mem
> + * if it operates on the mm_struct of the
> + * current process: it could be used to
> + * bypass W^X.
> + */
> +
> + if (!sara_enabled ||
> + !wxprot_enabled ||
> + !(sara_wxp_flags & SARA_WXP_WXORX) ||
> + !(file->f_mode & FMODE_WRITE))
> + return 0;
> +
> + t = get_sara_inode_task(file_inode(file));
> + if (unlikely(t != NULL &&
> + strcmp(file->f_path.dentry->d_name.name,
> + "mem") == 0)) {
This should probably at least have a READ_ONCE() somewhere in case the
file concurrently gets renamed?
> + get_task_struct(t);
> + mm = get_task_mm(t);
> + put_task_struct(t);
Getting and dropping a reference to the task_struct here is completely
useless. Either you have a reference, in which case you don't need to
take another one, or you don't have a reference, in which case you
also can't take one.
> + if (unlikely(mm == current->mm))
> + sara_warn_or_goto(error,
> + "write access to /proc/*/mem");
Why is the current process so special that it must be protected more
than other processes? Is the idea here to rely on other protections to
protect all other tasks? This should probably come with a comment that
explains this choice.
> + mmput(mm);
> + }
> + return 0;
> +error:
> + mmput(mm);
> + return -EACCES;
> +}
Jann Horn <[email protected]> wrote:
>
> On Sat, Jul 6, 2019 at 12:55 PM Salvatore Mesoraca
> <[email protected]> wrote:
> > Prevent a task from opening, in "write" mode, any /proc/*/mem
> > file that operates on the task's mm.
> > A process could use it to overwrite read-only memory, bypassing
> > S.A.R.A. restrictions.
> [...]
> > +static void sara_task_to_inode(struct task_struct *t, struct inode *i)
> > +{
> > + get_sara_inode_task(i) = t;
>
> This looks bogus. Nothing is actually holding a reference to `t` here, right?
I think you are right, I should probably store the PID here.
> > +}
> > +
> > static struct security_hook_list data_hooks[] __lsm_ro_after_init = {
> > LSM_HOOK_INIT(cred_prepare, sara_cred_prepare),
> > LSM_HOOK_INIT(cred_transfer, sara_cred_transfer),
> > LSM_HOOK_INIT(shm_alloc_security, sara_shm_alloc_security),
> > + LSM_HOOK_INIT(task_to_inode, sara_task_to_inode),
> > };
> [...]
> > +static int sara_file_open(struct file *file)
> > +{
> > + struct task_struct *t;
> > + struct mm_struct *mm;
> > + u16 sara_wxp_flags = get_current_sara_wxp_flags();
> > +
> > + /*
> > + * Prevent write access to /proc/.../mem
> > + * if it operates on the mm_struct of the
> > + * current process: it could be used to
> > + * bypass W^X.
> > + */
> > +
> > + if (!sara_enabled ||
> > + !wxprot_enabled ||
> > + !(sara_wxp_flags & SARA_WXP_WXORX) ||
> > + !(file->f_mode & FMODE_WRITE))
> > + return 0;
> > +
> > + t = get_sara_inode_task(file_inode(file));
> > + if (unlikely(t != NULL &&
> > + strcmp(file->f_path.dentry->d_name.name,
> > + "mem") == 0)) {
>
> This should probably at least have a READ_ONCE() somewhere in case the
> file concurrently gets renamed?
My understanding here is that /proc/$pid/mem files cannot be renamed.
t != NULL implies we are in procfs.
Under these assumptions I think that that code is fine.
Am I wrong?
> > + get_task_struct(t);
> > + mm = get_task_mm(t);
> > + put_task_struct(t);
>
> Getting and dropping a reference to the task_struct here is completely
> useless. Either you have a reference, in which case you don't need to
> take another one, or you don't have a reference, in which case you
> also can't take one.
Absolutely agree.
> > + if (unlikely(mm == current->mm))
> > + sara_warn_or_goto(error,
> > + "write access to /proc/*/mem");
>
> Why is the current process so special that it must be protected more
> than other processes? Is the idea here to rely on other protections to
> protect all other tasks? This should probably come with a comment that
> explains this choice.
Yes, I should have spent some more words here.
Access to /proc/$pid/mem from other processes is already regulated by
security_ptrace_access_check (i.e. Yama).
Unfortunately, that hook is ignored when "mm == current->mm".
It seems that there is some user-space software that relies on /proc/self/mem
being writable (cfr. commit f511c0b17b08).
Thank you for your suggestions.