lkml.org 
[lkml]   [2022]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RFC v2 0/4] coredump: mitigate privilege escalation of process coredump
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(-)
>

\
 
 \ /
  Last update: 2022-01-05 13:31    [W:0.070 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site