| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231130203549.5549-1-kamatam@amazon.com>
Date: Thu, 30 Nov 2023 20:35:49 +0000
From: Munehisa Kamata <kamatam@...zon.com>
To: <casey@...aufler-ca.com>
CC: <akpm@...ux-foundation.org>, <kamatam@...zon.com>,
<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] proc: Update inode upon changing task security attribute
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@...10-0-32-198 ~]$ uname -r
6.7.0-rc3-proc-fix+
[ec2-user@...10-0-32-198 ~]$ sudo capsh --drop=cap_mac_override --
[root@...10-0-32-198 ec2-user]# mount --bind /proc/$$ dir
[root@...10-0-32-198 ec2-user]# echo AAA > /proc/$$/attr/current
[root@...10-0-32-198 ec2-user]# cat /proc/$$/attr/current; echo
AAA
[root@...10-0-32-198 ec2-user]# echo BBB > dir/attr/current
[root@...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@...zon.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:
>
Powered by blists - more mailing lists