On 11/30/2023 12:35 PM, Munehisa Kamata wrote: > On Thu, 2023-11-30 18:00:13 +0000, Casey Schaufler wrote: >> 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. > I verified the behavior with 6.7-rc3. > > Here is more "raw" log from my machine: > > [ec2-user@ip-10-0-32-198 ~]$ uname -r > 6.7.0-rc3-proc-fix+ > [ec2-user@ip-10-0-32-198 ~]$ sudo capsh --drop=cap_mac_override -- > [root@ip-10-0-32-198 ec2-user]# mount --bind /proc/$$ dir > [root@ip-10-0-32-198 ec2-user]# echo AAA > /proc/$$/attr/current > [root@ip-10-0-32-198 ec2-user]# cat /proc/$$/attr/current; echo > AAA > [root@ip-10-0-32-198 ec2-user]# echo BBB > dir/attr/current > [root@ip-10-0-32-198 ec2-user]# cat dir/attr/current > cat: dir/attr/current: Permission denied > > If something frequently scans /proc, such as ps, top or whatever, on your > machine, the inode may get updated quickly (i.e. revalidated during path > lookup) and then you may only have a short window to observe the behavior. I was able to reproduce the issue with a 6.5 kernel. The window seems to be really short. Would it be completely unreasonable for your sandboxing application to call syncfs(2) after writing to current? > >>>>> # 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:
