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

Message-ID: <ZvR+k3D1KGALOIWt@tycho.pizza>
Date: Wed, 25 Sep 2024 15:20:19 -0600
From: Tycho Andersen <tycho@...ho.pizza>
To: Aleksa Sarai <cyphar@...har.com>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>,
	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>,
	Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>
Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm

On Wed, Sep 25, 2024 at 05:50:10PM +0200, Aleksa Sarai wrote:
> On 2024-09-24, Eric W. Biederman <ebiederm@...ssion.com> 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.
> > 
> > The kernel allows prctl(PR_SET_NAME, ...)  without any permission
> > checks so adding an AT_ flat to use argv[0] instead of the execed
> > filename seems reasonable.
> > 
> > Maybe the flag should be called AT_NAME_ARGV0.
> > 
> > 
> > That said I am trying to remember why we picked /dev/fd/N, as the
> > filename.
> > 
> > My memory is that we couldn't think of anything more reasonable to use.
> > Looking at commit 51f39a1f0cea ("syscalls: implement execveat() system
> > call") unfortunately doesn't clarify anything for me, except that
> > /dev/fd/N was a reasonable choice.
> > 
> > I am thinking the code could reasonably try:
> > 	get_fs_root_rcu(current->fs, &root);
> > 	path = __d_path(file->f_path, root, buf, buflen);
> > 
> > To see if a path to the file from the current root directory can be
> > found.  For files that are not reachable from the current root the code
> > still need to fallback to /dev/fd/N.
> > 
> > Do you think you can investigate that and see if that would generate
> > a reasonable task->comm?
> 
> The problem mentioned during the discussion after the talk was that
> busybox symlinks everything to the same program, so using d_path will
> give somewhat confusing results and so separate behaviour is still
> needed (though to be fair, the current results are also confusing).

I also remember that busybox used to do symlinks, but I just looked the
latest version on the docker hub (perhaps not representative...) and
it's all hard links, which works fine with the __d_path() trick.

> > It looks like a reasonable case can be made that while /dev/fd/N is
> > a good path for interpreters, it is never a good choice for comm,
> > so perhaps we could always use argv[0] if the fdpath is of the
> > form /dev/fd/N.
> > 
> > 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.
> 
> I think only affecting task->comm would be ideal.

Yep, I did this for the test above, and it worked fine:

        if (bprm->fdpath) {
                /*
                 * If fdpath was set, execveat() made up a path that will
                 * probably not be useful to admins running ps or similar.
                 * Let's fix it up to be something reasonable.
                 */
                struct path root;
                char *path, buf[1024];

                get_fs_root(current->fs, &root);
                path = __d_path(&bprm->file->f_path, &root, buf, sizeof(buf));

                __set_task_comm(me, kbasename(path), true);
        } else {
                __set_task_comm(me, kbasename(bprm->filename), true);
        }

obviously we don't want a stack allocated buffer, but triggering on
->fdpath != NULL seems like the right thing, so we won't need a flag
either.

The question is: argv[0] or __d_path()?

Tycho