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 <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 | 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 <simo(a)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;
-}
-