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 <s...@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 > _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel