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

Eric W. Biederman
2024-11-29T19:33:39Z

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