netdev - Re: [PATCH v7 7/9] coredump: validate socket name as it is written

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: <20250516-planen-radar-2131a4b7d9b1@brauner>
Date: Fri, 16 May 2025 11:54:31 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jann Horn <jannh@...gle.com>
Cc: linux-fsdevel@...r.kernel.org, Daniel Borkmann <daniel@...earbox.net>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, Eric Dumazet <edumazet@...gle.com>, 
	Oleg Nesterov <oleg@...hat.com>, "David S. Miller" <davem@...emloft.net>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Daan De Meyer <daan.j.demeyer@...il.com>, 
	David Rheinsberg <david@...dahead.eu>, Jakub Kicinski <kuba@...nel.org>, Jan Kara <jack@...e.cz>, 
	Lennart Poettering <lennart@...ttering.net>, Luca Boccassi <bluca@...ian.org>, Mike Yuan <me@...dnzj.com>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-security-module@...r.kernel.org, Alexander Mikhalitsyn <alexander@...alicyn.com>
Subject: Re: [PATCH v7 7/9] coredump: validate socket name as it is written

On Thu, May 15, 2025 at 10:56:51PM +0200, Jann Horn wrote:
> On Thu, May 15, 2025 at 12:04 AM Christian Brauner <brauner@...nel.org> wrote:
> > In contrast to other parameters written into
> > /proc/sys/kernel/core_pattern that never fail we can validate enabling
> > the new AF_UNIX support. This is obviously racy as hell but it's always
> > been that way.
> >
> > Signed-off-by: Christian Brauner <brauner@...nel.org>
> 
> Reviewed-by: Jann Horn <jannh@...gle.com>
> 
> > ---
> >  fs/coredump.c | 37 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 6ee38e3da108..d4ff08ef03e5 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -1228,13 +1228,44 @@ void validate_coredump_safety(void)
> >         }
> >  }
> >
> > +static inline bool check_coredump_socket(void)
> > +{
> > +       if (core_pattern[0] != '@')
> > +               return true;
> > +
> > +       /*
> > +        * Coredump socket must be located in the initial mount
> > +        * namespace. Don't give the that impression anything else is
> > +        * supported right now.
> > +        */
> > +       if (current->nsproxy->mnt_ns != init_task.nsproxy->mnt_ns)
> > +               return false;
> 
> (Ah, dereferencing init_task.nsproxy without locks is safe because
> init_task is actually the boot cpu's swapper/idle task, which never
> switches namespaces, right?)

I would be very worried if it did. It would fsck everyone over that
relies on copying its credentials and assumes that the set of namespaces
is stable.

> 
> > +       /* Must be an absolute path. */
> > +       if (*(core_pattern + 1) != '/')
> > +               return false;
> > +
> > +       return true;
> > +}
> > +
> >  static int proc_dostring_coredump(const struct ctl_table *table, int write,
> >                   void *buffer, size_t *lenp, loff_t *ppos)
> >  {
> > -       int error = proc_dostring(table, write, buffer, lenp, ppos);
> > +       int error;
> > +       ssize_t retval;
> > +       char old_core_pattern[CORENAME_MAX_SIZE];
> > +
> > +       retval = strscpy(old_core_pattern, core_pattern, CORENAME_MAX_SIZE);
> > +
> > +       error = proc_dostring(table, write, buffer, lenp, ppos);
> > +       if (error)
> > +               return error;
> > +       if (!check_coredump_socket()) {
> 
> (non-actionable note: This is kiiinda dodgy under
> SYSCTL_WRITES_LEGACY, but I guess we can assume that new users of the
> new coredump socket feature aren't actually going to write the
> coredump path one byte at a time, so I guess it's fine.)

So this is all kinds of broken already imho. Because there's not really
mutual exclusion between multiple writers to such sysctls from what I
remember. Which means that this buffer can be trampled in all kinds of
ways if multiple tasks decide to update it at the same time. That's
super unlikely of course but whatever.

> 
> > +               strscpy(core_pattern, old_core_pattern, retval + 1);
> 
> The third strscpy() argument is semantically supposed to be the
> destination buffer size, not the amount of data to copy. For trivial
> invocations like here, strscpy() actually allows you to leave out the
> third argument.

Eeeeewww, that's really implicit behavior. I can use the destination
buffer size but given that retval will always be smaller than that I
didn't bother but ok. I'll fix that in-tree.