Jann Horn [off-list ref] wrote:

On Sat, Jul 6, 2019 at 12:55 PM Salvatore Mesoraca
[off-list ref] wrote:

quoted

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.

[...]

quoted

+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.

quoted

+}
+
 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),
 };

[...]

quoted

+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?

quoted

+               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.

quoted

+               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.