[SSSD] [PATCH] Move ccache operations to krb5_child, allow the krb5_auth code to run unprivileged

Simo Sorce simo at redhat.com
Thu Nov 6 14:56:13 UTC 2014


Comments inline.
(I'll send multiple emails per patch, where necessary)

On Wed, 5 Nov 2014 18:36:06 +0100
Jakub Hrozek <jhrozek at redhat.com> wrote:

> From c9d484638009371cff3b1213b5640a7827b9e1ba Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek <jhrozek at redhat.com>
> Date: Mon, 13 Oct 2014 21:13:38 +0200
> Subject: [PATCH 2/6] KRB5: Drop privileges in the child, not the back
> end
> 
> In future patches, sssd_be will be running as a non-privileged user,
> who will execute the setuid krb5_child. In this case, the child will
> start as root and drop the privileges as soon as possible.
> 
> However, we need to also remove the privilege drop in sssd_be, because
> if we dropped to the user who is authenticating, we wouldn't be even
> allowed to execute krb5_child. The krb5_child permissions should be
> 4750, owned by root.sssd, to make sure only root and sssd can execute
> the child and if executed by sssd, the child will run as root.

Should we check that the parent process really is running either as
root or the sssd user and fail otherwise ?

> ---
>  src/providers/krb5/krb5_child.c         | 57
> +++++++++++++++++++++++++--------
> src/providers/krb5/krb5_child_handler.c |  8 ----- 2 files changed,
> 44 insertions(+), 21 deletions(-)
> 
> diff --git a/src/providers/krb5/krb5_child.c
> b/src/providers/krb5/krb5_child.c index
> 3234a4e6c740db5e05f7db8eb7f4ea0cc126e7ce..1c657de2d96d9fcea6d041af9cc9217863732b44
> 100644 --- a/src/providers/krb5/krb5_child.c +++
> b/src/providers/krb5/krb5_child.c @@ -1840,11 +1840,52 @@ static int
> k5c_setup_fast(struct krb5_req *kr, bool demand) return EOK;
>  }
>  
> -static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> +static errno_t check_use_fast(bool *_fast)
>  {
> -    krb5_error_code kerr;
>      char *use_fast_str;
> +    bool fast = false;
> +
> +    use_fast_str = getenv(SSSD_KRB5_USE_FAST);

Although the launching user is somewhat trusted .. should we stop using
environment variables and instead pass info via command line options ?

The reason I ask is that I would like to see all getenv() calls changed
to secure_getenv() calls where possible.

> +    if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") ==
> 0) {
> +        DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
> +    } else if (strcasecmp(use_fast_str, "try") == 0 ||
> +               strcasecmp(use_fast_str, "demand") == 0) {
> +        fast = true;
> +    } else {
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +                "Unsupported value [%s] for krb5_use_fast.\n",
> +                use_fast_str);
> +        return EINVAL;
> +    }
> +
> +    *_fast = fast;
> +    return EOK;
> +}
> +
> +static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> +{
> +    krb5_error_code kerr;
>      int parse_flags;
> +    bool use_fast;
> +
> +    kerr = check_use_fast(&use_fast);
> +    if (kerr != EOK) {
> +        return kerr;
> +    }
> +
> +    if (offline || (use_fast == false && kr->validate == false)) {
> +        /* If krb5_child was started as setuid, but we don't need to
> +         * perform either validation or FAST, just drop privileges to
> +         * the SSSD user. The same applies to the offline case

Do I read this wrongly ? Or is this comment misleading ?
As far as I can tell you are dropping privileges to the user trying to
authenticate not the SSSD user? (Did I miss another change where SSSD's
user uid/gid values are set in kr ?)

> +         */
> +        kerr = become_user(kr->uid, kr->gid);
> +        if (kerr != 0) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
> +            return kerr;
> +        }
> +    }
> +    DEBUG(SSSDBG_TRACE_INTERNAL,
> +          "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(),
> getegid()); 
>      kr->realm = getenv(SSSD_KRB5_REALM);
>      if (kr->realm == NULL) {
> @@ -1931,18 +1972,8 @@ static int k5c_setup(struct krb5_req *kr,
> uint32_t offline) if (!offline) {
>          set_canonicalize_option(kr->options);
>  
> -        use_fast_str = getenv(SSSD_KRB5_USE_FAST);
> -        if (use_fast_str == NULL || strcasecmp(use_fast_str,
> "never") == 0) {
> -            DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
> -        } else if (strcasecmp(use_fast_str, "try") == 0) {
> +        if (use_fast) {
>              kerr = k5c_setup_fast(kr, false);
> -        } else if (strcasecmp(use_fast_str, "demand") == 0) {
> -            kerr = k5c_setup_fast(kr, true);

This change seem to be breaking k5c_setup_fast, in the sense that you
never pass along the "demand" option and therefore the code will always
fall back even though the configuration wanted to forbid downgrades.

I think this makes me NACK this patch (and I do not see any other
change that addresses this point in the other patches).

> -        } else {
> -            DEBUG(SSSDBG_CRIT_FAILURE,
> -                  "Unsupported value [%s] for krb5_use_fast.\n",
> -                   use_fast_str);
> -            return EINVAL;
>          }
>      }
>  
> diff --git a/src/providers/krb5/krb5_child_handler.c
> b/src/providers/krb5/krb5_child_handler.c index
> 4ba939deb3e0e282b3ca9b2b21226ea7e64e311b..71c7f9c9f662e16b94afda0c8c0ae24666f0ba15
> 100644 --- a/src/providers/krb5/krb5_child_handler.c +++
> b/src/providers/krb5/krb5_child_handler.c @@ -284,14 +284,6 @@ static
> errno_t fork_child(struct tevent_req *req) pid = fork();
>  
>      if (pid == 0) { /* child */
> -        if (state->kr->run_as_user) {
> -            ret = become_user(state->kr->uid, state->kr->gid);
> -            if (ret != EOK) {
> -                DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
> -                return ret;
> -            }
> -        }
> -
>          err = exec_child(state,
>                           pipefd_to_child, pipefd_from_child,
>                           KRB5_CHILD,
> state->kr->krb5_ctx->child_debug_fd);



-- 
Simo Sorce * Red Hat, Inc * New York



More information about the sssd-devel mailing list