[SSSD] [PATCHES] Simplify credential krb5 cache manipulation

Sumit Bose sbose at redhat.com
Mon Sep 9 10:17:34 UTC 2013


On Fri, Sep 06, 2013 at 09:20:58AM -0400, Simo Sorce wrote:
> On Fri, 2013-09-06 at 15:04 +0200, Sumit Bose wrote:
> > On Tue, Sep 03, 2013 at 10:07:13PM -0400, Simo Sorce wrote:
> > > After the recent patches to explicitly enable the KEYRING type in SSSD I
> > > realized that the code that manipulates ccaches had grown too much, and,
> > > most importantly, was doing unnecessary operations already performed in
> > > an abstract way by krb5 functions.
> > > 
> > > This patch set mostly addresses ticket #2061
> > > 
> > > The aims has been to remove as much as possible type-specific code,
> > > resorting to type specific behavior only as an explicit exception where
> > > necessary due to historical or other reasons.
> > > 
> > > The combined diff gives a nice total stat of:
> > >   815 insertions(+)
> > >   1529 deletions(-)
> > > 
> > > 
> > > The last patch is an attempt to address ticket #2071,
> > > 
> > > It was necessary to add it here otherwise sssd will fail to operate
> > > correctly with some templates (as noted in #2071).
> > > However I am not sure that's the way we want to resolve the problem.
> > > The patch aimed at maintaining as much as possible a reasonable
> > > behavior, although the intended behavior was not really written
> > > anywhere. Personally I would rather scrap the patch and instead provide
> > > a new one that would simply stop creating public directories at all, I
> > > do not think it is sssd's role to fix/create directories that should be
> > > set up by the admin appropriately ahead of time (either manually of via
> > > tmpfiles.d or whatever).
> > > 
> > > I will try to follow up with a proposed patch that 'simplifies' sssd
> > > behavior instead of fixing it for #2071
> > > 
> > > Simo.
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> > 
> > I've reviewed the first 14 patches and my comments and the result of a
> > discussion on irc about how security can be improved when running as a
> > user are below.
> 
> Thank you, comments inline and new patchset attached.
> 
> > I'll review the 15th patch together with the alternative one and send my
> > comments separately.
> > 
> > bye,
> > Sumit
> 
> 
> [..]
> 
> >                                         ret, strerror(ret)));
> > > +            /* free ssc immediately otherwise the code will try to restore
> > > +             * wrong creds */
> > > +            free(ssc);
> > 
> > talloc_free or talloc_zfree
> 
> ouch thanks a lot for catching this one.
> 
> [..]
> 
> > > +    /* change gid now, (leaves saved gid to current, so we can restore) */
> > > +    ret = setresgid(gid, gid, -1);
> > 
> > only setting the egid should be sufficient and safer
> 
> [..]
> 
> > > +    if (uid != 0) {
> > > +        /* change uid, (leaves saved uid to current, so we can restore) */
> > > +        ret = setresuid(uid, uid, -1);
> > 
> > only setting the euid should be sufficient and safer
> 
> 
> As discussed on IRC change both the above to retain the real uid/gid by
> leaving -1 in there, this way a user process cannot successfully send
> SIGKILL to sssd_be.
> 
> 
> [..]
> 
> 
> > > +static errno_t handle_randomized(TALLOC_CTX *mem_ctx, char *in)
> > 
> > mem_ctx is not used in handle_randomized()
> 
> Ah remnants of a previous version, removed the mem_ctx here and also
> removed the tmp_ctx from the calling function, as handle_randomized()
> was actually the only user.
> 
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> From 331513efff86c377203c7ea9a86381c989233b8e Mon Sep 17 00:00:00 2001
> From: Simo Sorce <simo at redhat.com>
> Date: Wed, 28 Aug 2013 21:19:32 -0400
> Subject: [PATCH 01/15] krb5: Add calls to change and restore credentials
> 
> In some cases we want to temporarily assume user credentials but allow the
> process to regain back the original credentials (normally regaining uid 0).
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2061
> ---
>  src/providers/krb5/krb5_become_user.c | 125 ++++++++++++++++++++++++++++++++++
>  src/providers/krb5/krb5_utils.h       |   6 ++
>  2 files changed, 131 insertions(+)
> 
> diff --git a/src/providers/krb5/krb5_become_user.c b/src/providers/krb5/krb5_become_user.c
> index 70bc5630ece21505b58bd1a8795d7ab4b7864460..5539e114d33a7a2b0dfabb66f03144eddd230658 100644
> --- a/src/providers/krb5/krb5_become_user.c
> +++ b/src/providers/krb5/krb5_become_user.c
> @@ -70,3 +70,128 @@ errno_t become_user(uid_t uid, gid_t gid)
>      return EOK;
>  }
>  
> +struct sss_creds {
> +    uid_t uid;
> +    gid_t gid;
> +    int num_gids;
> +    gid_t gids[];
> +};
> +
> +errno_t restore_creds(struct sss_creds *saved_creds);
> +
> +/* This is a reversible version of become_user, and returns the saved
> + * credentials so that creds can be switched back calling restore_creds */
> +errno_t switch_creds(TALLOC_CTX *mem_ctx,
> +                     uid_t uid, gid_t gid,
> +                     int num_gids, gid_t *gids,
> +                     struct sss_creds **saved_creds)
> +{
> +    struct sss_creds *ssc = NULL;
> +    int size;
> +    int ret;
> +
> +    DEBUG(SSSDBG_FUNC_DATA, ("Switch user to [%d][%d].\n", uid, gid));
> +
> +    if (saved_creds) {
> +        /* save current user credentials */
> +        size = getgroups(0, NULL);
> +        if (size == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Getgroups failed! (%d, %s)\n",
> +                                        ret, strerror(ret)));
> +            goto done;
> +        }
> +
> +        ssc = talloc_size(mem_ctx,
> +                          (sizeof(struct sss_creds) + size * sizeof(gid_t)));
> +        if (!ssc) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Allocation failed!\n"));
> +            ret = ENOMEM;
> +            goto done;
> +        }
> +        ssc->num_gids = size;
> +
> +        size = getgroups(ssc->num_gids, ssc->gids);
> +        if (size == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Getgroups failed! (%d, %s)\n",
> +                                        ret, strerror(ret)));
> +            /* free ssc immediately otherwise the code will try to restore
> +             * wrong creds */
> +            talloc_zfree(ssc);
> +            goto done;
> +        }
> +
> +        /* we care only about effective ids */
> +        ssc->uid = geteuid();
> +        ssc->gid = getegid();
> +    }
> +
> +    /* if we are regaining root set euid first so that we have CAP_SETUID back,
> +     * ane the other calls work too, otherwise call it last so that we can
> +     * change groups before we loose CAP_SETUID */
> +    if (uid == 0) {
> +        ret = setresuid(0, 0, 0);
> +        if (ret == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("setresuid failed [%d][%s].\n", ret, strerror(ret)));
> +            goto done;
> +        }
> +    }
> +
> +    /* TODO: use prctl to get/set capabilities too ? */
> +
> +    /* try to setgroups first should always work if CAP_SETUID is set,
> +     * otherwise it will always fail, failure is not critical though as
> +     * generally we only really care about uid and at mot primary gid */
> +    ret = setgroups(num_gids, gids);
> +    if (ret == -1) {
> +        ret = errno;
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              ("setgroups failed [%d][%s].\n", ret, strerror(ret)));
> +    }
> +
> +    /* change gid now, (leaves saved gid to current, so we can restore) */
> +    ret = setresgid(gid, -1, -1);
> +    if (ret == -1) {
> +        ret = errno;
> +        DEBUG(SSSDBG_CRIT_FAILURE,
> +              ("setresgid failed [%d][%s].\n", ret, strerror(ret)));
> +        goto done;
> +    }
> +
> +    if (uid != 0) {
> +        /* change uid, (leaves saved uid to current, so we can restore) */
> +        ret = setresuid(uid, -1, -1);

I think this is wrong, the first argument of setresuid() is the real UID
the second the effective. To retain the real and change the effective
one the line should read

+        ret = setresuid(-1, uid, -1);

same for setresgid().

ACK to patches 2-14.

bye,
Sumit

> +        if (ret == -1) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("setresuid failed [%d][%s].\n", ret, strerror(ret)));
> +            goto done;
> +        }
> +    }
> +
> +    ret = 0;
> +
> +done:
> +    if (ret) {
> +        if (ssc) {
> +            /* attempt to restore creds first */
> +            restore_creds(ssc);
> +            talloc_free(ssc);
> +        }
> +    } else if (saved_creds) {
> +        *saved_creds = ssc;
> +    }
> +    return ret;
> +}
> +
> +errno_t restore_creds(struct sss_creds *saved_creds)
> +{
> +    return switch_creds(saved_creds,
> +                        saved_creds->uid,
> +                        saved_creds->gid,
> +                        saved_creds->num_gids,
> +                        saved_creds->gids, NULL);
> +}
> diff --git a/src/providers/krb5/krb5_utils.h b/src/providers/krb5/krb5_utils.h
> index cdc9f23641905cff300077f708087e98ba683e0d..aac3ec72ec7e1664ae96cc4e53d368e22448f5f1 100644
> --- a/src/providers/krb5/krb5_utils.h
> +++ b/src/providers/krb5/krb5_utils.h
> @@ -80,6 +80,12 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
>                               bool case_sensitive, bool *private_path);
>  
>  errno_t become_user(uid_t uid, gid_t gid);
> +struct sss_creds;
> +errno_t switch_creds(TALLOC_CTX *mem_ctx,
> +                     uid_t uid, gid_t gid,
> +                     int num_gids, gid_t *gids,
> +                     struct sss_creds **saved_creds);
> +errno_t restore_creds(struct sss_creds *saved_creds);
>  
>  errno_t get_ccache_file_data(const char *ccache_file, const char *client_name,
>                               struct tgt_times *tgtt);
> -- 
> 1.8.3.1
> 



More information about the sssd-devel mailing list