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. I'll review the 15th patch together with the alternative one and send my comments separately. bye, Sumit > From e68d428e3d2b6de23d42aeaf174ab7f04fd593b7 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 | 126 > ++++++++++++++++++++++++++++++++++ > src/providers/krb5/krb5_utils.h | 6 ++ > 2 files changed, 132 insertions(+) > > diff --git a/src/providers/krb5/krb5_become_user.c > b/src/providers/krb5/krb5_become_user.c > index > 70bc5630ece21505b58bd1a8795d7ab4b7864460..e78ad04734d85175ede82c4635503f6bf6af1a2c > 100644 > --- a/src/providers/krb5/krb5_become_user.c > +++ b/src/providers/krb5/krb5_become_user.c > @@ -70,3 +70,129 @@ 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 */ > + free(ssc); talloc_free or talloc_zfree > + ssc = NULL; > + 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, gid, -1); only setting the egid should be sufficient and safer > + 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, uid, -1); only setting the euid should be sufficient and safer > + 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 > .... > From 69347a7f7281b1cdbe3906bcc86a8f9c515808e3 Mon Sep 17 00:00:00 2001 > From: Simo Sorce <s...@redhat.com> > Date: Mon, 2 Sep 2013 23:52:46 -0400 > Subject: [PATCH 13/15] krb5_child: Simplify ccache creation > > The containing ccache directory is precreated by the parent code, > so there is no special need to do so here for any type. > Also the special handling for the FILE ccache temporary file is not really > useful, because libkrb5 internally unlinks and then recreate the file, so > mkstemp cannot really prevent subtle races, it can only make sure the file is > unique at creation time. > > Resolves: > https://fedorahosted.org/sssd/ticket/2061 > --- .... > -static krb5_error_code create_ccache_file(krb5_context ctx, > - krb5_principal princ, > - char *ccname, krb5_creds *creds) > +static errno_t handle_randomized(TALLOC_CTX *mem_ctx, char *in) mem_ctx is not used in handle_randomized() > { > - krb5_error_code kerr; > - krb5_ccache tmp_cc = NULL; > - char *cc_file_name; > - int fd = -1; > size_t ccname_len; > - char *dummy; > - char *tmp_ccname; > - TALLOC_CTX *tmp_ctx = NULL; > - mode_t old_umask; > + char *ccname = NULL; > + int ret; > + int fd; > > - DEBUG(SSSDBG_FUNC_DATA, ("Creating ccache at [%s]\n", ccname)); > - > - if (strncmp(ccname, "FILE:", 5) == 0) { > - cc_file_name = ccname + 5; > + /* We only treat the FILE type case in a special way due to the history > + * of storing FILE type ccache in /tmp and associated security issues */ > + if (in[0] == '/') { > + ccname = in; > + } else if (strncmp(in, "FILE:", 5) == 0) { > + ccname = in + 5; > } else { > - cc_file_name = ccname; > - } > - > - if (cc_file_name[0] != '/') { > - DEBUG(1, ("Ccache filename is not an absolute path.\n")); > - return EINVAL; > - } > - > - tmp_ctx = talloc_new(tmp_ctx); > - if (tmp_ctx == NULL) { > - DEBUG(1, ("talloc_new failed.\n")); > - return ENOMEM; > - } > - > - dummy = strrchr(cc_file_name, '/'); > - tmp_ccname = talloc_strndup(tmp_ctx, cc_file_name, > - (size_t) (dummy-cc_file_name)); > - if (tmp_ccname == NULL) { > - DEBUG(1, ("talloc_strdup failed.\n")); > - kerr = ENOMEM; > - goto done; > - } > - tmp_ccname = talloc_asprintf_append(tmp_ccname, "/.krb5cc_dummy_XXXXXX"); > - if (tmp_ccname == NULL) { > - kerr = ENOMEM; > - goto done; > - } > - > - old_umask = umask(077); > - fd = mkstemp(tmp_ccname); > - umask(old_umask); > - if (fd == -1) { > - kerr = errno; > - DEBUG(SSSDBG_CRIT_FAILURE, > - ("mkstemp failed [%d][%s].\n", kerr, strerror(kerr))); > - goto done; > - } > - > - kerr = krb5_cc_resolve(ctx, tmp_ccname, &tmp_cc); > - if (kerr != 0) { > - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); > - goto done; > - } > - > - kerr = store_creds_in_ccache(ctx, princ, tmp_cc, creds); > - if (fd != -1) { > - close(fd); > - fd = -1; > - } > - if (kerr != 0) { > - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); > - goto done; > - } > - > - > - ccname_len = strlen(cc_file_name); > - if (ccname_len >= 6 && strcmp(cc_file_name + (ccname_len - 6), "XXXXXX") > == 0) { > - fd = mkstemp(cc_file_name); > + return EOK; > + } > + > + ccname_len = strlen(ccname); > + if (ccname_len >= 6 && strcmp(ccname + (ccname_len - 6), "XXXXXX") == 0) > { > + /* NOTE: this call is only used to create a unique name, as later > + * krb5_cc_initialize() will unlink and recreate the file. > + * This is ok because this part of the code is called with > + * privileges already dropped when handling user ccache, or the > ccache > + * is stored in a private directory. So we do not have huge issues if > + * something races, we mostly care only about not accidentally use > + * an existing name and thus failing in the process of saving the > + * cache. Malicious races can only be avoided by libkrb5 itself. */ > + fd = mkstemp(ccname); > if (fd == -1) { > - kerr = errno; > - DEBUG(SSSDBG_CRIT_FAILURE, > - ("mkstemp failed [%d][%s].\n", kerr, strerror(kerr))); > - goto done; > - } > - } > - > - kerr = rename(tmp_ccname, cc_file_name); > - if (kerr == -1) { > - kerr = errno; > - DEBUG(1, ("rename failed [%d][%s].\n", kerr, strerror(kerr))); > - } > - > - DEBUG(SSSDBG_TRACE_LIBS, ("Created ccache file: [%s]\n", ccname)); > - > -done: > - if (kerr != 0 && tmp_cc != NULL) { > - krb5_cc_destroy(ctx, tmp_cc); > - } > - > - if (fd != -1) { > - close(fd); > - } > - > - talloc_free(tmp_ctx); > - return kerr; > -} > - _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
