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

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

[Munehisa Kamata]

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(-)

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:
-- 
2.40.1

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

[Casey Schaufler]

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(&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:

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

[Munehisa Kamata]

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(&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:
> 

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

[Casey Schaufler]

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(&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: