Message-ID: <CAG48ez1ERkkwd+cJPmLmVj4JKpj5Uq=LaUEpb6_TgC4PRXosUw@mail.gmail.com>
Date: Fri, 18 Jul 2025 18:48:54 +0200
From: Jann Horn <jannh@...gle.com>
To: Nicolas Bouchinet <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 5:47 PM Nicolas Bouchinet
<nicolas.bouchinet@....cyber.gouv.fr> wrote:
> Hi Jann, thanks for your review !
>
> On Fri, Jul 18, 2025 at 04:45:15PM +0200, Jann Horn wrote:
> > 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".
> >
>
> Also replying to  :
>
> > What's the background here - do you have a specific usecase that
> > motivated this patch?
>
> The case I encountered is using the zathura-sandbox pdf viewer which
> sandboxes itself with Landlock and set itself as non-dumpable.

It kind of sounds like an issue with your PDF viewer if that just sets
the non-dumpable flag for no reason...

> If my PDF viewer freezes and I want to kill it as an unprivileged user,
> I'm not able to get its PID from `/proc` since its fully invisible to my
> user.
>
> > > 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.
> I open to work on that.
>
> > 2. I think it prevents a process with euid!=ruid from introspecting
> > itself through procfs.
> Great question, I'll test that and write some hidepid tests to check that.
>
> > 3. I think it prevents root from viewing all processes through procfs.
> Yes only if combined with yama="no attach", and IMHO, that would make sense.

Why only if combined with yama? Doesn't your code always "return
false" on a UID/GID mismatch?

> > 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.
> Arf, you're absolutely right about this, my bad.
>
> > 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()).
> As suggested by Aleksa, I can add some capabilities checks here.
>
> >
> > What's the background here - do you have a specific usecase that
> > motivated this patch?
>
> The second motivation is that the "ptraceable" mode didn't worked with
> the yama LSM, which doesn't care about `PTRACE_MODE_READ_FSCREDS` trace
> mode. Thus, using hidepid "ptraceable" mode with yama "restricted",
> "admin-only" or "no attach" modes doesn't do much.
>
> As you have seen, I also have submited a fix to yama in order to make it
> take into account `PTRACE_MODE_READ_FSCREDS` traces.

I don't think that's really a fix - that's more of a new feature
you're proposing. Yama currently explicitly only restricts ATTACH-mode
ptrace access (which can read all process memory or modify the state
of a process), and it doesn't restrict less invasive introspection
that uses READ-mode ptrace checks.

> I have to admit I'm not really found of the fact that those two patch
> are so tightly linked.
>
> Thanks again for your review,
>
> Nicolas

Powered by blists - more mailing lists