Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

[Eric W. Biederman]

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?

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?

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

Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

[Al Viro]

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>

Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

[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
>

Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

[Eric W. Biederman]

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