[systemd-devel] [PATCH 2/2] selinux: set selinux context applied on exec() before closing all fds

[Zbigniew Jędrzejewski-Szmek]

On Mon, Oct 13, 2014 at 04:57:13PM +0200, Michal Sekletar wrote:
> We need original socket_fd around otherwise label_get_child_mls_label fails with
> -EINVAL return code.
> ---
>  src/core/execute.c | 58 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/src/core/execute.c b/src/core/execute.c
> index b165b33..d64fa7c 100644
> --- a/src/core/execute.c
> +++ b/src/core/execute.c
> @@ -1578,6 +1578,36 @@ static int exec_child(ExecCommand *command,
>                  }
>          }
>  
I don't like the fact that now SELINUX context would be applied before
closing of the fds, and other contexts after. I think it would be much nicer
to simply extract getting the label, i.e. this part:

> +                        if (params->selinux_context_net && socket_fd >= 0) {
> +                                _cleanup_free_ char *label = NULL;
> +
> +                                err = label_get_child_mls_label(socket_fd, command->path, &label);
> +                                if (err < 0) {
> +                                        *error = EXIT_SELINUX_CONTEXT;
> +                                        return err;
> +                                }

above the closing of the fds, and keep the rest where it was.

> +#ifdef HAVE_SELINUX
> +        if (params->apply_permissions) {
> +                if (use_selinux()) {
> +                        if (context->selinux_context) {
> +                                err = setexeccon(context->selinux_context);
> +                                if (err < 0 && !context->selinux_context_ignore) {
> +                                        *error = EXIT_SELINUX_CONTEXT;
> +                                        return err;
> +                                }
> +                        }
> +
> +                        if (params->selinux_context_net && socket_fd >= 0) {
> +                                _cleanup_free_ char *label = NULL;
> +
> +                                err = label_get_child_mls_label(socket_fd, command->path, &label);
> +                                if (err < 0) {
> +                                        *error = EXIT_SELINUX_CONTEXT;
> +                                        return err;
> +                                }
> +
> +                                err = setexeccon(label);
> +                                if (err < 0) {
> +                                        *error = EXIT_SELINUX_CONTEXT;
> +                                        return err;
> +                                }
> +                        }
> +                }
> +        }
> +#endif

BTW, it is quite confusing for me that setexeccon() is called
twice. IIUC, this is because "only the descurity level is used
from the information provided the peer". Is this correct? Could
you add a comment to the code that explains this?

Zbyszek

> +
>          /* We repeat the fd closing here, to make sure that
>           * nothing is leaked from the PAM modules. Note that
>           * we are more aggressive this time since socket_fd
> @@ -1665,34 +1695,6 @@ static int exec_child(ExecCommand *command,
>                  }
>  #endif
>  
> -#ifdef HAVE_SELINUX
> -                if (use_selinux()) {
> -                        if (context->selinux_context) {
> -                                err = setexeccon(context->selinux_context);
> -                                if (err < 0 && !context->selinux_context_ignore) {
> -                                        *error = EXIT_SELINUX_CONTEXT;
> -                                        return err;
> -                                }
> -                        }
> -
> -                        if (params->selinux_context_net && socket_fd >= 0) {
> -                                _cleanup_free_ char *label = NULL;
> -
> -                                err = label_get_child_mls_label(socket_fd, command->path, &label);
> -                                if (err < 0) {
> -                                        *error = EXIT_SELINUX_CONTEXT;
> -                                        return err;
> -                                }
> -
> -                                err = setexeccon(label);
> -                                if (err < 0) {
> -                                        *error = EXIT_SELINUX_CONTEXT;
> -                                        return err;
> -                                }
> -                        }
> -                }
> -#endif
> -
>  #ifdef HAVE_APPARMOR
>                  if (context->apparmor_profile && use_apparmor()) {
>                          err = aa_change_onexec(context->apparmor_profile);