| Message ID | 20231130003704.31928-1-kamatam@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | proc: Update inode upon changing task security attribute | expand |
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@sun.com/ > [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@amazon.com> > --- > 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:
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 > > # 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@sun.com/ > > [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@amazon.com> > > --- > > 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: >
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@sun.com/ >>> [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@amazon.com> >>> --- >>> 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:
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. > >>> # 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@sun.com/ > >>> [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@amazon.com> > >>> --- > >>> 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: >
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@sun.com/ >>>>> [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@amazon.com> >>>>> --- >>>>> 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:
On Thu, 2023-11-30 16:31:11 -0800, Casey Schaufler wrote: > > 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. Creating a PID namespace before the bind-mount may make the window lasts longer (or forever). $ sudo unshare -pf --mount-proc > Would it be completely unreasonable for your sandboxing application to > call syncfs(2) after writing to current? It doesn't help. It won't revalidate dentries. > > > >>>>> # 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@sun.com/ > >>>>> [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@amazon.com> > >>>>> --- > >>>>> 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: >
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:
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. 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 # 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@sun.com/ [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@amazon.com> --- fs/proc/base.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)