[SSSD] [PATCH] https://fedorahosted.org/sssd/ticket/2410

Jakub Hrozek jhrozek at redhat.com
Tue Aug 26 12:50:20 UTC 2014


On Tue, Aug 26, 2014 at 02:21:00PM +0200, Sumit Bose wrote:
> > > This is working as expected, but using krb5_cc_new_unique() and
> > > krb5_cc_get_name() from libkrb5 might even result in more simple code?
> > 
> > In theory yes, I have a patch that I tested already:
> >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=ldapchild&id=b692d625202d7ee512f0affb2a38c49ec02403d6o
> > 
> > But I'm not sure this approach will work well for us, because:
> >     1) The ccache is always created in /tmp when using
> >     krb5_cc_new_unique(). In the krb5 code it looks like
> >     krb5_cc_new_unique calls krb5_fcc_generate_new() which
> >     unconditionally uses a templatized file in /tmp. Because on many
> >     systems, tmp is a tmpfs, the rename errored out with error 18
> >     (Invalid cross-device link).
> > 
> >     2) Rename from one directory to another is not friedly to SELinux.
> > 
> > So while the code is way cleaner than handling the templates ourselves,
> > I think we would have a way to tell krb5_cc_new_unique() to accept a
> > directory to create the ccache at..or copy the file ourselves.
> 
> ah, sorry, I didn't realized that the hint option is unused. I agree
> that you original approach is better. But please free the temporary
> strings.

OK, done. I used a tmp_ctx for this change, because we have several
other temporary strings that suffer from the same. I also added the
extra debug message you requested earlier.

btw when I was testing the changes to make sure there is no
use-after-free, I realized we have many leaks in the ldap_child itself.
Using:
command = /usr/bin/valgrind --trace-children=yes /usr/libexec/sssd/sssd_be --domain ipa.example.com --debug-level 3

in the domain section. I guess that's worth of a ticket, but not urgent
either.
-------------- next part --------------
>From bc66dc8829648456d2f69ea9533afa29a550938c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 26 Aug 2014 14:35:55 +0200
Subject: [PATCH 1/2] LDAP: Use tmp_ctx in ldap_child for temporary data

Using a global memory context for short-lived private data might lead to
memory growth.
---
 src/providers/ldap/ldap_child.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index e2fe3f232b3f051bf1d94617c2c711b7087130c7..d0552d57c733b71fa2d901d6c9f0f9dd5cbbd8b5 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -184,6 +184,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
     int canonicalize = 0;
     int kdc_time_offset_usec;
     int ret;
+    TALLOC_CTX *tmp_ctx;
 
     krberr = krb5_init_context(&context);
     if (krberr) {
@@ -192,6 +193,12 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
     }
     DEBUG(SSSDBG_TRACE_INTERNAL, "Kerberos context initialized\n");
 
+    tmp_ctx = talloc_new(memctx);
+    if (tmp_ctx == NULL) {
+        krberr = KRB5KRB_ERR_GENERIC;
+        goto done;
+    }
+
     krberr = set_child_debugging(context);
     if (krberr != EOK) {
         DEBUG(SSSDBG_MINOR_FAILURE, "Cannot set krb5_child debugging\n");
@@ -205,14 +212,14 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
             goto done;
         }
 
-        realm_name = talloc_strdup(memctx, default_realm);
+        realm_name = talloc_strdup(tmp_ctx, default_realm);
         krb5_free_default_realm(context, default_realm);
         if (!realm_name) {
             krberr = KRB5KRB_ERR_GENERIC;
             goto done;
         }
     } else {
-        realm_name = talloc_strdup(memctx, realm_str);
+        realm_name = talloc_strdup(tmp_ctx, realm_str);
         if (!realm_name) {
             krberr = KRB5KRB_ERR_GENERIC;
             goto done;
@@ -223,10 +230,10 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
 
     if (princ_str) {
         if (!strchr(princ_str, '@')) {
-            full_princ = talloc_asprintf(memctx, "%s@%s",
+            full_princ = talloc_asprintf(tmp_ctx, "%s@%s",
                                          princ_str, realm_name);
         } else {
-            full_princ = talloc_strdup(memctx, princ_str);
+            full_princ = talloc_strdup(tmp_ctx, princ_str);
         }
     } else {
         char hostname[HOST_NAME_MAX + 1];
@@ -240,7 +247,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
 
         DEBUG(SSSDBG_TRACE_LIBS, "got hostname: [%s]\n", hostname);
 
-        ret = select_principal_from_keytab(memctx, hostname, realm_name,
+        ret = select_principal_from_keytab(tmp_ctx, hostname, realm_name,
                 keytab_name, &full_princ, NULL, NULL);
         if (ret) {
             krberr = KRB5_KT_IOERR;
@@ -283,7 +290,7 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
         goto done;
     }
 
-    ccname = talloc_asprintf(memctx, "FILE:%s/ccache_%s", DB_PATH, realm_name);
+    ccname = talloc_asprintf(tmp_ctx, "FILE:%s/ccache_%s", DB_PATH, realm_name);
     if (!ccname) {
         krberr = KRB5KRB_ERR_GENERIC;
         goto done;
@@ -362,10 +369,11 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
 #endif
 
     krberr = 0;
-    *ccname_out = ccname;
+    *ccname_out = talloc_steal(memctx, ccname);
     *expire_time_out = my_creds.times.endtime - kdc_time_offset;
 
 done:
+    talloc_free(tmp_ctx);
     if (krberr != 0) KRB5_SYSLOG(krberr);
     if (keytab) krb5_kt_close(context, keytab);
     if (context) krb5_free_context(context);
-- 
1.9.3

-------------- next part --------------
>From fe8401195007ef9ba7e28a0e4c9a107e8ca27354 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 26 Aug 2014 09:43:09 +0200
Subject: [PATCH 2/2] LDAP: Use randomized ccname for storing credentials

https://fedorahosted.org/sssd/ticket/2410

If two ldap_child processes attempt to prime the ccache at the same time
for the same domain, the ldap_child might fail with:
    [ldap_child_get_tgt_sync] (0x0040): Failed to init ccache: Internal credentials cache error
    [main] (0x0020): ldap_child_get_tgt_sync failed.

To avoid the race-condition, the ldap_child process now creates the
ccache randomized and before returning to the caller, renames the
randomized ccache to a permanent one.
---
 src/providers/ldap/ldap_child.c | 44 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
index d0552d57c733b71fa2d901d6c9f0f9dd5cbbd8b5..6ef7bd204bb028b0dc1f0ccb8b6e52cdacf044f2 100644
--- a/src/providers/ldap/ldap_child.c
+++ b/src/providers/ldap/ldap_child.c
@@ -168,7 +168,9 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
                                                const char **ccname_out,
                                                time_t *expire_time_out)
 {
+    int fd;
     char *ccname;
+    char *ccname_dummy;
     char *realm_name = NULL;
     char *full_princ = NULL;
     char *default_realm = NULL;
@@ -185,6 +187,8 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
     int kdc_time_offset_usec;
     int ret;
     TALLOC_CTX *tmp_ctx;
+    char *ccname_file_dummy;
+    char *ccname_file;
 
     krberr = krb5_init_context(&context);
     if (krberr) {
@@ -290,14 +294,34 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
         goto done;
     }
 
-    ccname = talloc_asprintf(tmp_ctx, "FILE:%s/ccache_%s", DB_PATH, realm_name);
-    if (!ccname) {
-        krberr = KRB5KRB_ERR_GENERIC;
+    ccname_file_dummy = talloc_asprintf(tmp_ctx, "%s/ccache_%s_XXXXXX",
+                                        DB_PATH, realm_name);
+    ccname_file = talloc_asprintf(tmp_ctx, "%s/ccache_%s",
+                                  DB_PATH, realm_name);
+    if (ccname_file_dummy == NULL || ccname_file == NULL) {
+        ret = ENOMEM;
         goto done;
     }
-    DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname);
 
-    krberr = krb5_cc_resolve(context, ccname, &ccache);
+    fd = mkstemp(ccname_file_dummy);
+    if (fd == -1) {
+        ret = errno;
+        goto done;
+    }
+    /* We only care about creating a unique file name here, we don't
+     * need the fd
+     */
+    close(fd);
+
+    ccname_dummy = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file_dummy);
+    ccname = talloc_asprintf(tmp_ctx, "FILE:%s", ccname_file);
+    if (ccname_dummy == NULL || ccname == NULL) {
+        krberr = ENOMEM;
+        goto done;
+    }
+    DEBUG(SSSDBG_TRACE_INTERNAL, "keytab ccname: [%s]\n", ccname_dummy);
+
+    krberr = krb5_cc_resolve(context, ccname_dummy, &ccache);
     if (krberr) {
         DEBUG(SSSDBG_OP_FAILURE, "Failed to set cache name: %s\n",
                   sss_krb5_get_error_message(context, krberr));
@@ -368,6 +392,16 @@ static krb5_error_code ldap_child_get_tgt_sync(TALLOC_CTX *memctx,
     kdc_time_offset = 0;
 #endif
 
+    DEBUG(SSSDBG_TRACE_INTERNAL,
+          "Renaming [%s] to [%s]\n", ccname_file_dummy, ccname_file);
+    ret = rename(ccname_file_dummy, ccname_file);
+    if (ret == -1) {
+        ret = errno;
+        DEBUG(SSSDBG_CRIT_FAILURE,
+              "rename failed [%d][%s].\n", ret, strerror(ret));
+        goto done;
+    }
+
     krberr = 0;
     *ccname_out = talloc_steal(memctx, ccname);
     *expire_time_out = my_creds.times.endtime - kdc_time_offset;
-- 
1.9.3



More information about the sssd-devel mailing list