* Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)
2024-11-29 4:23 ` [GIT PULL] execve updates for v6.13-rc1 (take 2) Eric W. Biederman
@ 2024-11-29 4:48 ` Al Viro
2024-11-29 17:00 ` Casey Schaufler
2024-11-29 19:33 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Al Viro @ 2024-11-29 4:48 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Casey Schaufler, Linus Torvalds, Kees Cook, linux-kernel,
Christophe JAILLET, Nir Lichtman, Tycho Andersen, Vegard Nossum,
linux-security-module
On Thu, Nov 28, 2024 at 10:23:18PM -0600, Eric W. Biederman wrote:
> > 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.
... presumably because the pathname might have changed its meaning
just as we called execve(). Which is why we want it to show up in
comm, got it.
</sarcasm>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)
2024-11-29 4:23 ` [GIT PULL] execve updates for v6.13-rc1 (take 2) Eric W. Biederman
2024-11-29 4:48 ` Al Viro
@ 2024-11-29 17:00 ` Casey Schaufler
2024-11-29 19:33 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Casey Schaufler @ 2024-11-29 17:00 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, Kees Cook, linux-kernel, Christophe JAILLET,
Nir Lichtman, Tycho Andersen, Vegard Nossum,
linux-security-module, Al Viro, Casey Schaufler
On 11/28/2024 8:23 PM, Eric W. Biederman wrote:
> Al Viro <viro@zeniv.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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)
2024-11-29 4:23 ` [GIT PULL] execve updates for v6.13-rc1 (take 2) Eric W. Biederman
2024-11-29 4:48 ` Al Viro
2024-11-29 17:00 ` Casey Schaufler
@ 2024-11-29 19:33 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Eric W. Biederman @ 2024-11-29 19:33 UTC (permalink / raw)
To: Casey Schaufler
Cc: Linus Torvalds, Kees Cook, linux-kernel, Christophe JAILLET,
Nir Lichtman, Tycho Andersen, Vegard Nossum,
linux-security-module, Al Viro
Casey and the smack folks my apologies for copying you in.
I just read the code below a little more carefully and it is definitely
a systemd bug.
mac_smack_read_fd reads the xattr that smack will apply as a label if it
is present. So there is no reason for systemd to apply the label
itself. Worse smack_bprm_creds_for_exec applies checks before
applying the label (aka is the superblock trusted) that systemd doesn't.
Which means systemd might apply a label from a smack xattr when
smack wouldn't.
> 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;
> }
Which means the systemd code should really be:
> static int setup_smack(
> const ExecParameters *params,
> const ExecContext *context) {
> int r;
>
> assert(params);
> 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) {
> r = mac_smack_apply_pid(0, params->fallback_smack_process_label);
> if (r < 0)
> return r;
> }
>
> return 0;
> }
At which point systemd has no need to open the executable file
descriptor and thus no need to play with fexecve.
Eric
^ permalink raw reply [flat|nested] 4+ messages in thread