On 11/29/2023 7:07 PM, Munehisa Kamata wrote: > Hi Casey, > > On Wed, 2023-11-29 18:28:55 -0800, Casey Schaufler wrote: >> 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. > Will do that next. > >>> 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? > Ahh, yes, I meant /proc/$$/attr/current. Sorry about that... > >>> # 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 I don't see this behavior. What kernel version are you using? I have a 6.5 kernel. >>> # 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(¤t->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:
