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/[email protected]/
[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 <[email protected]>
---
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:
--
2.40.1
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/[email protected]/
> [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 <[email protected]>
> ---
> 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/[email protected]/
> > [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 <[email protected]>
> > ---
> > 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/[email protected]/
>>> [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 <[email protected]>
>>> ---
>>> 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/[email protected]/
> >>> [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 <[email protected]>
> >>> ---
> >>> 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/[email protected]/
>>>>> [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 <[email protected]>
>>>>> ---
>>>>> 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/[email protected]/
> >>>>> [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 <[email protected]>
> >>>>> ---
> >>>>> 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:
>