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