linux-kernel - Re: [RFC] exec: add a flag for "reasonable" execveat() comm

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] [day] [month] [year] [list]

Message-ID: <Zv1aA4I6r4py-8yW@kawka3.in.waw.pl>
Date: Wed, 2 Oct 2024 14:34:43 +0000
From: Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Tycho Andersen <tycho@...ho.pizza>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Kees Cook <kees@...nel.org>, Jeff Layton <jlayton@...nel.org>,
	Chuck Lever <chuck.lever@...cle.com>,
	Alexander Aring <alex.aring@...il.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Tycho Andersen <tandersen@...flix.com>,
	Aleksa Sarai <cyphar@...har.com>
Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm

On Tue, Sep 24, 2024 at 12:39:35PM -0500, Eric W. Biederman wrote:
> Tycho Andersen <tycho@...ho.pizza> writes:
> 
> > From: Tycho Andersen <tandersen@...flix.com>
> >
> > Zbigniew mentioned at Linux Plumber's that systemd is interested in
> > switching to execveat() for service execution, but can't, because the
> > contents of /proc/pid/comm are the file descriptor which was used,
> > instead of the path to the binary. This makes the output of tools like
> > top and ps useless, especially in a world where most fds are opened
> > CLOEXEC so the number is truly meaningless.
> >
> > This patch adds an AT_ flag to fix up /proc/pid/comm to instead be the
> > contents of argv[0], instead of the fdno.

I tried this version (with a local modification to drop the flag and
enable the new codepath if get_user_arg_ptr(argv, 0) returns nonnull
as suggested later in the thread), and it seems to work as expected.
In particular, 'pgrep' finds for the original name in case of
symlinks.

> All of that said I am not a fan of the implementation below as it has
> the side effect of replacing /dev/fd/N with a filename that is not
> usable by #! interpreters.  So I suggest an implementation that affects
> task->comm and not brpm->filename.

Hmm, I don't understand this. /dev/fd/ would not generally contain an
open fd for the original binary. It only would if the caller uses
fexecve with an fd opened without O_CLOEXEC, but then it'd be
something like /dev/fd/3 or /dev/fd/4 and the callee would be confused
by having an extra fd, so except for some specialed cases, the caller
should always use O_CLOEXEC.

With this patch:
$ sudo ln -sv /bin/sleep /usr/local/bin/sleep-link
$ sudo systemd-run sleep-link 10000

$ sudo strace -f -e execve,execveat -p 1
...
[pid  1200] execve("/proc/self/fd/9", ["/usr/lib/systemd/systemd-executo"..., "--deserialize", "150", "--log-level", "info", "--log-target", "journal-or-kmsg"], 0x7ffe97b98178 /* 3 vars */) = 0
[pid  1200] execveat(4, "", ["/usr/local/bin/sleep-link", "10000"], 0xd8edf70 /* 9 vars */, AT_EMPTY_PATH) = 0
^C

$ pgrep sleep-link
1200

$ sudo ls -l /proc/1200/fd
total 0
lr-x------ 1 root root 64 Oct  2 17:13 0 -> /dev/null
lrwx------ 1 root root 64 Oct  2 17:13 1 -> 'socket:[8585]'
lrwx------ 1 root root 64 Oct  2 17:13 2 -> 'socket:[8585]'

$ head -n1 /proc/1200/{comm,status,stat}
==> /proc/1200/comm <==
sleep-link
==> /proc/1200/status <==
Name:   sleep-link
==> /proc/1200/stat <==
1200 (sleep-link) ...

This all looks good.

Zbyszek