Message-ID: <20250718.113751-obedient.unloader.usable.clocks-2vPOsgY9lE5m@cyphar.com>
Date: Fri, 18 Jul 2025 21:40:29 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: nicolas.bouchinet@....cyber.gouv.fr
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Vasiliy Kulikov <segooon@...il.com>, Christian Brauner <brauner@...nel.org>, 
	Olivier Bal-Petre <olivier.bal-petre@....cyber.gouv.fr>, Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
Subject: Re: [PATCH] fs: hidepid: Fixes hidepid non dumpable behavior

On 2025-07-18, Aleksa Sarai <cyphar@...har.com> wrote:
> On 2025-07-18, nicolas.bouchinet@....cyber.gouv.fr <nicolas.bouchinet@....cyber.gouv.fr> wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
> > 
> > The hidepid mount option documentation defines the following modes:
> > 
> > - "noaccess": user may not access any `/proc/<pid>/ directories but
> >   their own.
> > - "invisible": all `/proc/<pid>/` will be fully invisible to other users.
> > - "ptraceable": means that procfs should only contain `/proc/<pid>/`
> >   directories that the caller can ptrace.
> > 
> > We thus expect that with "noaccess" and "invisible" users would be able to
> > see their own processes in `/proc/<pid>/`.
> > 
> > The implementation of hidepid however control accesses using the
> > `ptrace_may_access()` function in any cases. Thus, if a process set
> > itself as non-dumpable using the `prctl(PR_SET_DUMPABLE,
> > SUID_DUMP_DISABLE)` it becomes invisible to the user.
> 
> In my view, the documentation is wrong here. This behaviour has remained
> effectively unchanged since it was introduced in 0499680a4214 ("procfs:
> add hidepid= and gid= mount options"), and the documentation was written
> by the same author (added to Cc, though they appear to be inactive since
> 2013). hidepid=ptraceable was added many years later, and so the current
> documentation seeming somewhat contradictory is probably more a result
> of a new feature being documented without rewriting the old
> documentation, rather than an incorrect implementation.
> 
> A process marking itself with SUID_DUMP_DISABLE is a *very* strong
> signal that other processes (even processes owned by the same user) must
> have very restricted access to it. Given how many times they have been
> instrumental for protecting against attacks, I am quite hesitant about
> making changes to loosen these restrictions.
> 
> For instance, container runtimes need to set SUID_DUMP_DISABLE to avoid
> all sorts of breakout attacks (CVE-2016-9962 and CVE-2019-5736 being the
> most famous examples, but there are plenty of others). If a container
> has been configured with a restrictive hidepid, I would expect the
> kernel to block most attempts to interact with such a process to
> non-privileged users. But this patch would loosen said restrictions.
> 
> Now, many of the bits in /proc/self/* are additionally gated behind
> ptrace_may_access() checks, so this kind of change might be less
> catastrophic than at first glance, but the original concerns that
> motivated hidepid= were about /proc/self/cmdline and the uid/euid of
> processes being discoverable, and AFAICS this patch still undoes those
> protections for the cases we care about with SUID_DUMP_DISABLE.
> 
> What motivated you to want to change this behaviour?
> 
> > This patch fixes the `has_pid_permissions()` function in order to make
> > its behavior to match the documentation.
> > 
> > Note that since `ptrace_may_access()` is not called anymore with
> > "noaccess" and "invisible", the `security_ptrace_access_check()` will no
> > longer be called either.
> > 
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
> > ---
> >  fs/proc/base.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index c667702dc69b8ca2531e88e12ed7a18533f294dd..fb128cb5f95fe65016fce96c75aee18c762a30f2 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -746,9 +746,12 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> >  				 struct task_struct *task,
> >  				 enum proc_hidepid hide_pid_min)
> >  {
> > +	const struct cred *cred = current_cred(), *tcred;
> > +	kuid_t caller_uid;
> > +	kgid_t caller_gid;
> >  	/*
> > -	 * If 'hidpid' mount option is set force a ptrace check,
> > -	 * we indicate that we are using a filesystem syscall
> > +	 * If 'hidepid=ptraceable' mount option is set, force a ptrace check.
> > +	 * We indicate that we are using a filesystem syscall
> >  	 * by passing PTRACE_MODE_READ_FSCREDS
> >  	 */
> >  	if (fs_info->hide_pid == HIDEPID_NOT_PTRACEABLE)
> > @@ -758,7 +761,25 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
> >  		return true;
> >  	if (in_group_p(fs_info->pid_gid))
> >  		return true;
> > -	return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
> > +
> > +	task_lock(task);
> > +	rcu_read_lock();
> > +	caller_uid = cred->fsuid;
> > +	caller_gid = cred->fsgid;
> > +	tcred = __task_cred(task);
> > +	if (uid_eq(caller_uid, tcred->euid) &&
> > +	    uid_eq(caller_uid, tcred->suid) &&
> > +	    uid_eq(caller_uid, tcred->uid)  &&
> > +	    gid_eq(caller_gid, tcred->egid) &&
> > +	    gid_eq(caller_gid, tcred->sgid) &&
> > +	    gid_eq(caller_gid, tcred->gid)) {
> > +		rcu_read_unlock();
> > +		task_unlock(task);
> > +		return true;
> > +	}
> > +	rcu_read_unlock();
> > +	task_unlock(task);
> > +	return false;
> 
> At the very least, this check needs to be gated based on
> ns_capable(get_task_mm(task)->user_ns, CAP_SYS_PTRACE), to avoid
> containers from being able to introspect SUID_DUMP_DISABLE processes
> (such as container runtimes) in the process of joining a user namespaced
> container.

Actually get_task_mm(test)->user_ns == current_user_ns() is probably
want you want? ns_capable(..., CAP_SYS_PTRACE) is basically an
equivalent of ptrace_may_access() here. But we very rarely do permission
checks based just on the userns -- we almost always use ns_capable()
since that actually handles the hierarchical relationship between user
namespaces regarding privileges.

Either way, I'm not a fan of this change, to be honest.

> 
> >  }
> >  
> >  
> > 
> > ---
> > base-commit: 884a80cc9208ce75831b2376f2b0464018d7f2c4
> > change-id: 20250718-hidepid_fix-d0743d0540e7
> > 
> > Best regards,
> > -- 
> > Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
> > 
> > 

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists