Message-ID: <20250718.091233-bored.chainsaw.organic.pose-0SJBoWYaT8s@cyphar.com>
Date: Fri, 18 Jul 2025 19:36:37 +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, 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.

>  }
>  
>  
> 
> ---
> 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