Message-ID: <ed4edfaa-3dcc-4cb5-9a01-bd04d41b602b@schaufler-ca.com>
Date: Fri, 29 Nov 2024 09:00:25 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 Kees Cook <kees@...nel.org>, linux-kernel@...r.kernel.org,
 Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Nir Lichtman <nir@...htman.org>, Tycho Andersen <tandersen@...flix.com>,
 Vegard Nossum <vegard.nossum@...cle.com>,
 linux-security-module@...r.kernel.org, Al Viro <viro@...iv.linux.org.uk>,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

On 11/28/2024 8:23 PM, Eric W. Biederman wrote:
> Al Viro <viro@...iv.linux.org.uk> writes:
>
>> On Thu, Nov 28, 2024 at 06:43:31PM -0800, Linus Torvalds wrote:
>>> A sane setup has lots of options
>>>
>>>  - just use execve() with the actual name of the executable
>>>
>>>  - use hardlinks and use execveat()
>>>
>>>  - use open() on a symlink and then execveat() of that file, and get
>>> the actual name of the executable behind the symlink
>>>
>>>  - disagree about comm[] being relevant at all, and don't use it, and
>>> don't use tools that do
>>>
>>> and none of those are wrong decisions.
>> Just one thing - IMO we want to use the relative pathname when it's
>> not empty.  Even in execveat().  Because some wanker *will* decide
>> that newer is better and make libc use execveat() to implement execve().
>> Which won't be spotted for about a year, and when it does we'll get
>> seriously stuck.
> For clarity the only patches I have seen so far have been
> about the fexecve subset of execveat.  AKA when execveat is has
> not been supplied a path.
>
>> I agree that for fexecve() the only sane approach is to go by whatever
>> that opened file refers to; I'm not sold on the _usefulness_ of
>> fexecve() to start with, but if we want that thing, that's the way
>> to go.
> The craziness is that apparently systemd wants to implement execve in
> terms of fexecve, not execveat.
>
> ..
>
> Wanting to see what is going on I cloned the most recent version of
> systemd.  I see some calls that are generally redundant.  That is
> systemd opens the executable and fstat's the executable to make
> certain the executable is a regular file and not a directory symlink.
>
> Which seems harmless but pointless unless something is interesting
> is going to happen with the executable_fd before it is passed to
> the kernel to execute.
>
> The only case in systemd that does something interesting with the
> executable_fd (that I could see) has to do with smack.  Please see
> the code below.
>
> Casey do you have any idea why systemd would want to read the label from
> the executable and apply it to the current process?  Is the systemd
> smack support sensible?

Smack supports an attribute SMACK64EXEC, which identifies the label the
program should run with. I haven't looked at how fexecve() treats this.
I would think that fexecve() would have to respect this behavior just as
it would support setuid and setgid bits. I am looking at the systemd code
and it emulates the exec() behavior, so regardless of the fexecve()
behavior the program will run with the correct label.

> As it is written this seems like the kind of logic every process that
> calls execve will want to repeat.
>
> So I am wondering isn't it easier just to get the kernel to do the right
> thing and not have deeply special smack code in systemd?  Does the
> kernel already do the right thing and systemd is just confused?

The reason systemd emulates the exec() behavior is to allow for the
case where systemd starts all processes with a particular label. I
don't know why it uses fexecve().

> Right now it looks like the sane path is to sort out the systemd's
> smack support and then systemd should be able to continue using
> execve like any sane program.
>
> #if ENABLE_SMACK
> static int setup_smack(
>                 const ExecParameters *params,
>                 const ExecContext *context,
>                 int executable_fd) {
>         int r;
>
>         assert(params);
>         assert(executable_fd >= 0);
>
>         if (context->smack_process_label) {
>                 r = mac_smack_apply_pid(0, context->smack_process_label);
>                 if (r < 0)
>                         return r;
>         } else if (params->fallback_smack_process_label) {
>                 _cleanup_free_ char *exec_label = NULL;
>
>                 r = mac_smack_read_fd(executable_fd, SMACK_ATTR_EXEC, &exec_label);
>                 if (r < 0 && !ERRNO_IS_XATTR_ABSENT(r))
>                         return r;
>
>                 r = mac_smack_apply_pid(0, exec_label ?: params->fallback_smack_process_label);
>                 if (r < 0)
>                         return r;
>         }
>
>         return 0;
> }
> #endif
>
> Eric
>

Powered by blists - more mailing lists