Re: [PATCH RFC v2 0/4] coredump: mitigate privilege escalation of process coredump

[Wander Costa <wcosta@redhat.com>]

On Mon, Jan 3, 2022 at 7:54 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Wander Lairson Costa <wander@redhat.com> writes:
>
> Have you seen the discussion at:
> https://lkml.kernel.org/r/20211221021744.864115-1-longman@redhat.com
> ?
>

No, I wasn't aware of this, thanks.

> Adding a few people from that conversation.
>
> > v2
> > ==
> >
> > Patch 02 conflicted with commit 92307383082d("coredump:  Don't perform
> > any cleanups before dumping core") which I didn't have in my tree. V2
> > just changes the hunk
> >
> > +#define PF_SUID   0x00000008
> >
> > To
> >
> > +#define PF_SUID   0x01000000
> >
> > To merge cleanly. Other than that, it is the same patch as v1.
> >
> > v1
> > ==
> >
> > A set-uid executable might be a vector to privilege escalation if the
> > system configures the coredump file name pattern as a relative
> > directory destiny. The full description of the vulnerability and
> > a demonstration of how we can exploit it can be found at [1].
> >
> > This patch series adds a PF_SUID flag to the process in execve if it is
> > set-[ug]id binary and elevates the new image's privileges.
> >
> > In the do_coredump function, we check if:
> >
> > 1) We have the SUID_FLAG set
> > 2) We have CAP_SYS_ADMIN (the process might have decreased its
> >    privileges)
> > 3) The current directory is owned by root (the current code already
> >    checks for core_pattern being a relative path).
> > 4) non-privileged users don't have permission to write to the current
> >    directory.
>
> Which is a slightly different approach than we have discussed
> previously.
>
> Something persistent to mark the processes descended from the suid exec
> is something commonly agreed upon.
>
> How we can dump core after that (with the least disruption is the
> remaining question).
>
> You would always allow coredumps unless the target directory is
> problematic.  I remember it being suggested that even dumping to a pipe
> might not be safe in practice.  Can someone remember why?
>
> The other approach we have discussed is simply not allowing coredumps
> unless the target process takes appropriate action to reenable them.
>
> Willy posted a patch to that effect.
>
>
> From a proof of concept perspective PF_SUID and your directory checks look
> like fine.  From a production implementation standpoint I think we would
> want to make them a bit more general.  PF_SUID because it is more than
> uid changes that can grant privilege during exec.  We especially want to
> watch out for setcap executables.  The directory checks similarly look
> very inflexible.  I think what we want is to test if the process before
> the privilege change of the exec could write to the directory.
>
> Even with your directory test approach you are going to run into
> the semi-common idio of becomming root and then starting a daemon
> in debugging mode so you can get a core dump.
>
> > If all four conditions match, we set the need_suid_safe flag.
> >
> > An alternative implementation (and more elegant IMO) would be saving
> > the fsuid and fsgid of the process in the task_struct before loading the
> > new image to the memory. But this approach would add eight bytes to all
> > task_struct instances where only a tiny fraction of the processes need
> > it and under a configuration that not all (most?) distributions don't
> > adopt by default.
>
> One possibility is to save a struct cred on the mm_struct.  If done
> carefully I think that would allow commit_creds to avoid the need
> for dumpability changes (there would always be enough information to
> directly compute it).
>
> I can see that working for detecting dropped privileges.  I don't know
> if that would work for raised privileges.
>
> Definitely focusing in on the mm_struct for where to save the needed
> information seems preferable, as in general it is an mm property not a
> per task property.
>

After reading the other thread and your comments, I came up with the
following idea:

- Create fields coredump_uid and coredump_gid in the mm_struct
- At fork time, copy the euid and egid to coredump_uid and coredump_gid.
- Only change coredump_uid/coredump_gid when the process changes its euid/egid.
- The do_coredump function already creates a local creds struct.
Change the code to set
its fsuid and fsgid to coredump_uid and coredump_gid.

This solution still has the inconvenience that a suid daemon probably
won't have the permission to
core dump by default. We can fix this by changing the setsid system
call to assign coredump_uid and coredump_gid
to euid and egid.

If it sounds reasonable, I can give this idea a try.


> Eric
>
>
>
> > Wander Lairson Costa (4):
> >   exec: add a flag indicating if an exec file is a suid/sgid
> >   process: add the PF_SUID flag
> >   coredump: mitigate privilege escalation of process coredump
> >   exec: only set the suid flag if the current proc isn't root
> >
> >  fs/coredump.c           | 15 +++++++++++++++
> >  fs/exec.c               | 10 ++++++++++
> >  include/linux/binfmts.h |  6 +++++-
> >  include/linux/sched.h   |  1 +
> >  kernel/fork.c           |  2 ++
> >  5 files changed, 33 insertions(+), 1 deletion(-)
>