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

[Casey Schaufler <casey@schaufler-ca.com>]

On 12/5/2023 2:21 PM, Paul Moore wrote:
> On Fri, Dec 1, 2023 at 4:00 PM Munehisa Kamata <kamatam@amazon.com> wrote:
>> 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@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@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?
>>
>>>> +           put_pid(pid);
>>>> +           pid_update_inode(current, &ei->vfs_inode);
>>>> +           rcu_read_unlock();
>>>> +   }
> I think my thoughts are neatly summarized by Andrew's "yuk!" comment
> at the top.  However, before we go too much further on this, can we
> get clarification that Casey was able to reproduce this on a stock
> upstream kernel?  Last I read in the other thread Casey wasn't seeing
> this problem on Linux v6.5.

I was able to recreate the problem once given corrected instructions,
which have to be followed exactly. 

> However, for the moment I'm going to assume this is a real problem, is
> there some reason why the existing pid_revalidate() code is not being
> called in the bind mount case?  From what I can see in the original
> problem report, the path walk seems to work okay when the file is
> accessed directly from /proc, but fails when done on the bind mount.
> Is there some problem with revalidating dentrys on bind mounts?
>