[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