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

[Casey Schaufler]

On 12/1/2023 12:59 PM, Munehisa Kamata wrote:
> Hi Alexey,
>
> On Fri, 2023-12-01 09:30:00 +0000, Alexey Dobriyan wrote:
>> On Wed, Nov 29, 2023 at 05:11:22PM -0800, Andrew Morton wrote:
>>> fyi...
>>>
>>> (yuk!)
>>>
>>>
>>>
>>> Begin forwarded message:
>>>
>>> Date: Thu, 30 Nov 2023 00:37:04 +0000
>>> From: Munehisa Kamata <kamatam at amazon.com>
>>> To: <linux-fsdevel at vger.kernel.org>, <linux-security-module at vger.kernel.org>
>>> Cc: <linux-kernel at vger.kernel.org>, <akpm at linux-foundation.org>, "Munehisa Kamata" <kamatam at amazon.com>
>>> Subject: [PATCH] proc: Update inode upon changing task security attribute
>>>
>>>
>>> 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 at 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;
>> Should this "break;"? Why is only the last inode in the list updated?
>> Should it be the first? All of them?
> If it picks up the first node, it may end up updating /proc/[pid]/task/[tid]
> rather than /proc/[pid] (when pid == tid) and the task may be denied access
> to its own /proc/[pid] afterward.
>
> I think updating all of them won't hurt. But, as long as /proc/[pid] is
> accessible, the rest of the inodes should be updated upon path lookup via
> revalidation as usual.
>
> When pid != tid, it only updates /proc/[pid]/task/[tid] and the thread may
> lose an access to /proc/[pid], but I think it's okay as it's a matter of
> security policy enforced by security modules. Casey, do you have any
> comments here?

I do not.

>
>
> Regards,
> Munehisa
>
>  
>>> +		put_pid(pid);
>>> +		pid_update_inode(current, &ei->vfs_inode);
>>> +		rcu_read_unlock();
>>> +	}