Re: [PATCH] proc: Update inode upon changing task security attribute

From: Casey Schaufler
Date: Wed Nov 29 2023 - 21:29:14 EST


On 11/29/2023 4:37 PM, Munehisa Kamata wrote:
> I'm not clear whether VFS is a better (or worse) place[1] to fix the
> problem described below and would like to hear opinion.

Please To: or at least Cc: me on all Smack related issues.

>
> If the /proc/[pid] directory is bind-mounted on a system with Smack
> enabled, and if the task updates its current security attribute, the task
> may lose access to files in its own /proc/[pid] through the mountpoint.
>
> $ sudo capsh --drop=cap_mac_override --
> # mkdir -p dir
> # mount --bind /proc/$$ dir
> # echo AAA > /proc/$$/task/current # assuming built-in echo

I don't see "current" in /proc/$$/task. Did you mean /proc/$$/attr?

> # cat /proc/$$/task/current # revalidate
> AAA
> # echo BBB > dir/attr/current
> # cat dir/attr/current
> cat: dir/attr/current: Permission denied
> # ls dir/
> ls: cannot access dir/: Permission denied
> # cat /proc/$$/attr/current # revalidate
> BBB
> # cat dir/attr/current
> BBB
> # echo CCC > /proc/$$/attr/current
> # cat dir/attr/current
> cat: dir/attr/current: Permission denied
>
> This happens because path lookup doesn't revalidate the dentry of the
> /proc/[pid] when traversing the filesystem boundary, so the inode security
> blob of the /proc/[pid] doesn't get updated with the new task security
> attribute. Then, this may lead security modules to deny an access to the
> directory. Looking at the code[2] and the /proc/pid/attr/current entry in
> proc man page, seems like the same could happen with SELinux. Though, I
> didn't find relevant reports.
>
> The steps above are quite artificial. I actually encountered such an
> unexpected denial of access with an in-house application sandbox
> framework; each app has its own dedicated filesystem tree where the
> process's /proc/[pid] is bind-mounted to and the app enters into via
> chroot.
>
> With this patch, writing to /proc/[pid]/attr/current (and its per-security
> module variant) updates the inode security blob of /proc/[pid] or
> /proc/[pid]/task/[tid] (when pid != tid) with the new attribute.
>
> [1] https://lkml.kernel.org/linux-fsdevel/4A2D15AF.8090000@xxxxxxx/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/selinux/hooks.c#n4220
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Munehisa Kamata <kamatam@xxxxxxxxxx>
> ---
> fs/proc/base.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index dd31e3b6bf77..bdb7bea53475 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2741,6 +2741,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> {
> struct inode * inode = file_inode(file);
> struct task_struct *task;
> + const char *name = file->f_path.dentry->d_name.name;
> void *page;
> int rv;
>
> @@ -2784,10 +2785,26 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
> if (rv < 0)
> goto out_free;
>
> - rv = security_setprocattr(PROC_I(inode)->op.lsm,
> - file->f_path.dentry->d_name.name, page,
> - count);
> + rv = security_setprocattr(PROC_I(inode)->op.lsm, name, page, count);
> mutex_unlock(&current->signal->cred_guard_mutex);
> +
> + /*
> + * Update the inode security blob in advance if the task's security
> + * attribute was updated
> + */
> + if (rv > 0 && !strcmp(name, "current")) {
> + struct pid *pid;
> + struct proc_inode *cur, *ei;
> +
> + rcu_read_lock();
> + pid = get_task_pid(current, PIDTYPE_PID);
> + hlist_for_each_entry(cur, &pid->inodes, sibling_inodes)
> + ei = cur;
> + put_pid(pid);
> + pid_update_inode(current, &ei->vfs_inode);
> + rcu_read_unlock();
> + }
> +
> out_free:
> kfree(page);
> out: