linux-kernel - Re: [PATCH] fs: hidepid: Fixes hidepid non dumpable behavior

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez3u09TK=Ju3xdEKzKuM_-sO_y9150NBx3Drs8T1G-V9AQ@mail.gmail.com>
Date: Fri, 18 Jul 2025 16:45:15 +0200
From: Jann Horn <jannh@...gle.com>
To: nicolas.bouchinet@....cyber.gouv.fr
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.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 Fri, Jul 18, 2025 at 10:47 AM <nicolas.bouchinet@....cyber.gouv.fr> wrote:
> 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>/`.

"their own" is very fuzzy and could be interpreted many ways.

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

As Aleksa said, a non-dumpable processes is essentially like a setuid
process (even if its UIDs match yours, it may have some remaining
special privileges you don't have), so it's not really "your own".

> This patch fixes the `has_pid_permissions()` function in order to make
> its behavior to match the documentation.

I don't think "it doesn't match the documentation" is good enough
reason to change how the kernel works.

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

I think this is a bad idea for several reasons:

1. It duplicates existing logic.
2. I think it prevents a process with euid!=ruid from introspecting
itself through procfs.
3. I think it prevents root from viewing all processes through procfs.
4. It allows processes to view metadata about each other when that was
previously blocked by the combination of hidepid and an LSM such as
Landlock or SELinux.
5. It ignores capabilities held by the introspected process but not
the process doing the introspection (which is currently checked by
cap_ptrace_access_check()).

What's the background here - do you have a specific usecase that
motivated this patch?