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

Jakub Hrozek jhrozek at redhat.com
Mon Nov 10 16:11:43 UTC 2014


On Thu, Nov 06, 2014 at 09:56:13AM -0500, Simo Sorce wrote:
> Comments inline.
> (I'll send multiple emails per patch, where necessary)

Thanks for the review and sorry about the delay, some other work came
up...

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

We could only do that if we limit the user sssd runs as to either the
sssd user or root as well. Currently the 'user' configuration option
blindly accepts anything -- which might not be the best idea, because
realistically, only root or the sssd user would work.

So yeah, I'm fine with doing this change, but I also need to restrict
the user sssd runs at to root or the configured user.

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

Yes, I agree and it's been something that we planned for some time, but
I think it should be a separate patch.

The work is tracked by https://fedorahosted.org/sssd/ticket/697 btw

> 
> > +    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 ?)

The comment is misleading, thanks for catching that. Fixed.

> 
> > +         */
> > +        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).

Aah, thanks, I think I changed the code as a prototype to make sure FAST
worked at all and then forgot to finish this part..

I'll attach my patches to your other reply.

> 
> > -        } 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);



More information about the sssd-devel mailing list