[PATCHES] Simplify credential krb5 cache manipulation

[Simo Sorce]

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.

[Sumit Bose]

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(-)

I really like cleanup that comes with the patches but I would prefer not
to use setresuid() in the backend code because it allows any SSSD user
to kill the complete sssd_be process.

If there is no other safe way to handle the credential cache I think the
related operations should be moved from the backend code to the
krb5_child.

bye,
Sumit
> 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

[Simo Sorce]

On Thu, 2013-09-05 at 13:47 +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(-)
>
> I really like cleanup that comes with the patches but I would prefer not
> to use setresuid() in the backend code because it allows any SSSD user
> to kill the complete sssd_be process.
>
> If there is no other safe way to handle the credential cache I think the
> related operations should be moved from the backend code to the
> krb5_child.

That would be quite a different set of patches, I guess we can open a
bug to do that after this patchset goes in, but it will be hard, we do
some sysdb operations in there and I am not sure we want to allow the
child to do that.

Why do you think any user can kill sssd_be if we change user ?
IIRC we block all signals that could do that already.

Simo.

[Simo Sorce]

On Thu, 2013-09-05 at 09:16 -0400, Simo Sorce wrote:
> On Thu, 2013-09-05 at 13:47 +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(-)
> >
> > I really like cleanup that comes with the patches but I would prefer not
> > to use setresuid() in the backend code because it allows any SSSD user
> > to kill the complete sssd_be process.
> >
> > If there is no other safe way to handle the credential cache I think the
> > related operations should be moved from the backend code to the
> > krb5_child.
>
> That would be quite a different set of patches, I guess we can open a
> bug to do that after this patchset goes in, but it will be hard, we do
> some sysdb operations in there and I am not sure we want to allow the
> child to do that.
>
> Why do you think any user can kill sssd_be if we change user ?
> IIRC we block all signals that could do that already.

Btw, rebased patches on top of master are kept here:
http://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=ccname

Given the size I'll resend the patchset to the list if there is any
change request, but not for mere rebases for now.

Simo.

[Sumit Bose]

On Thu, Sep 05, 2013 at 09:16:02AM -0400, Simo Sorce wrote:
> On Thu, 2013-09-05 at 13:47 +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(-)
> >
> > I really like cleanup that comes with the patches but I would prefer not
> > to use setresuid() in the backend code because it allows any SSSD user
> > to kill the complete sssd_be process.
> >
> > If there is no other safe way to handle the credential cache I think the
> > related operations should be moved from the backend code to the
> > krb5_child.
>
> That would be quite a different set of patches, I guess we can open a
> bug to do that after this patchset goes in, but it will be hard, we do
> some sysdb operations in there and I am not sure we want to allow the
> child to do that.
>
> Why do you think any user can kill sssd_be if we change user ?
> IIRC we block all signals that could do that already.

Because I was successfull 1 out of 4 times without trying hard.

bye,
Sumit
> Simo.
>
> -- 
> Simo Sorce * Red Hat, Inc * New York
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

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

[Simo Sorce]

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.

[Sumit Bose]

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

[Simo Sorce]

On Mon, 2013-09-09 at 12:17 +0200, Sumit Bose wrote:
> 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.

Ouch you are right,

attached a fixed patch 0001, all others rebase on top without issue.

Simo.

[Sumit Bose]

On Mon, Sep 09, 2013 at 11:42:19AM -0400, Simo Sorce wrote:
> On Mon, 2013-09-09 at 12:17 +0200, Sumit Bose wrote:
> > 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.
>
> Ouch you are right,
>
> attached a fixed patch 0001, all others rebase on top without issue.
>
> Simo.

ACK

So now patches 1-14 have ACKs, fell free to push them.

Patch 15 or it's alternative are still under discussion in a different
thread.

bye,
Sumit
> -- 
> Simo Sorce * Red Hat, Inc * New York

[Simo Sorce]

On Mon, 2013-09-09 at 18:32 +0200, Sumit Bose wrote:
> On Mon, Sep 09, 2013 at 11:42:19AM -0400, Simo Sorce wrote:
> > On Mon, 2013-09-09 at 12:17 +0200, Sumit Bose wrote:
> > > 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.
> >
> > Ouch you are right,
> >
> > attached a fixed patch 0001, all others rebase on top without issue.
> >
> > Simo.
>
> ACK
>
> So now patches 1-14 have ACKs, fell free to push them.
>
> Patch 15 or it's alternative are still under discussion in a different
> thread.

Thank you, pushed all except 15 to master only.

Simo.