[SSSD] [PATCH] Fix krbcc dir creation issue with MIT krb5 1.11

Lukas Slebodnik lslebodn at redhat.com
Wed Apr 10 16:57:07 UTC 2013


On (10/04/13 08:48), Simo Sorce wrote:
>On Wed, 2013-04-10 at 11:45 +0200, Lukas Slebodnik wrote:
>> On (04/04/13 12:09), Simo Sorce wrote:
>> >On Thu, 2013-04-04 at 12:14 +0200, Lukas Slebodnik wrote:
>> >> ehlo,
>> >> 
>> >> In krb5-libs 1.11, function krb5_cc_resolve verify if credential cache dir
>> >> exists. If it doesn't exist, than it will be created. We
>> >> have to ensure, that it will be created with right uid, gid and permissions.
>> >> 
>> >> There is check in krb_auth_send, whether cached user data in ldb contains
>> >> SYSDB_CCACHE_FILE attribute. If it is available, then old ccache is checked in
>> >> function check_old_ccache. Gdm clean user directory (in /run/user/)
>> >> after logout. This is the reason why SYSDB_CCACHE_FILE attribute was found
>> >> in ldb cache and directory did not exist and then in function krb5_cc_resolve
>> >> was created with wrong permissions.
>> >> 
>> >> Patch attached.
>> >
>> >Although this approach may work I am not really comfortable with it.
>> >
>> >Here is an alternative approach:
>> >
>> >1. Before calling krb5_cc_resolve() do a stat.
>> function cc_residual_is_used call stat, but nonexisting file(directory)
>> was ignored (handled like EOK)
>
>Time to un-ignore it :-)
>
>> >2. If the file/dir does not exist just return an error w/o calling any
>> >krb5 function at all.
>> >
>> >I think this should also solve the issue, and will not involve change of
>> >uid/gid.
>> >
>> >This method has a very small race that can hardly be triggered though,
>> >it would require someone to remove the cache after the stat and before
>> >krb5_cc_resolve() is called. In that case auth would fail, but we should
>> >be able to recover on the next authentication by removing the file if it
>> >has the wrong permissions (I think this is already done, if not we can
>> >fix it with a second patch that adds checking for uid/gid after the stat
>> >and a call to cc_file_rmove/cc_dir_remove).
>> cc_dir_remove call cc_file_remove, so directory will not be removed.
>
>That is fine, we nee to check for dir/primary in dir case I think.
>
>Simo.
>
>-- 
>Simo Sorce * Red Hat, Inc * New York
>

Attaching updated patch, which also check dir/primary.

LS
-------------- next part --------------
>From 001fb9011db29abc2740024e6d2f6d7f8e23663e Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 6 Apr 2013 17:58:53 +0200
Subject: [PATCH] Fix krbcc dir creation issue with MIT krb5 1.11

In krb5-libs >= 1.11, function krb5_cc_resolve verify if credential cache dir
exists. If it doesn't exist, than it will be created with process permissions
and not user permissions.

Function cc_residual_is_used has already checked for non existing
directory, but it wasn't considered to be a failure and therefore next call
of krb5_init_context will create directory with wrong permissions.

Now if directory doesn't exist, it will be handled like there was not ccache
attribute in sysdb cache. We also check if "primary" file in ccache directory
has right permissions. But we ignore missing "primary" file.

https://fedorahosted.org/sssd/ticket/1822
---
 src/providers/krb5/krb5_auth.c  | 12 +++++++-
 src/providers/krb5/krb5_utils.c | 61 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 00025bfc156eaf641217194c6301f4d70a773a73..5baea0bc84bb6991d32300210d4bb4db3bcee5d0 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -106,6 +106,11 @@ check_old_ccache(const char *old_ccache, struct krb5child_req *kr,
 
     ret = old_cc_ops->check_existing(old_ccache, kr->uid, realm, kr->upn,
                                      cc_template, active, valid);
+    if (ret == ENOENT) {
+        DEBUG(SSSDBG_TRACE_FUNC,
+              ("Saved ccache %s doesn't exist.\n", old_ccache));
+        return ret;
+    }
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
               ("Cannot check if saved ccache %s is active and valid\n",
@@ -617,7 +622,12 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
             ret = check_old_ccache(ccache_file, kr, realm,
                                    &kr->active_ccache,
                                    &kr->valid_tgt);
-            if (ret != EOK) {
+            if (ret == ENOENT) {
+                DEBUG(SSSDBG_FUNC_DATA,
+                      ("Ignoring ccache attribute [%s], because it doesn't"
+                       "exist.\n", ccache_file));
+                ccache_file = NULL;
+            } else if (ret != EOK) {
                 DEBUG(SSSDBG_CRIT_FAILURE,
                       ("check_if_ccache_file_is_used failed.\n"));
                 goto done;
diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
index ad77c7cc8305a98cc263cd7c6222979f361d0155..e35544b4b12610e909af45051b0a152db04c3693 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -776,7 +776,7 @@ cc_residual_is_used(uid_t uid, const char *ccname,
             DEBUG(SSSDBG_FUNC_DATA, ("Cache file [%s] does not exist, "
                                      "it will be recreated\n", ccname));
             *result = false;
-            return EOK;
+            return ENOENT;
         }
 
         DEBUG(SSSDBG_OP_FAILURE,
@@ -869,10 +869,13 @@ cc_file_check_existing(const char *location, uid_t uid,
 
     ret = cc_residual_is_used(uid, filename, SSS_KRB5_TYPE_FILE, &active);
     if (ret != EOK) {
-        DEBUG(SSSDBG_OP_FAILURE, ("Could not check if ccache is active. "
-                                  "Will create a new one.\n"));
+        if (ret != ENOENT) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                  ("Could not check if ccache is active.\n"));
+        }
         cc_check_template(cc_template);
         active = false;
+        return ret;
     }
 
     kerr = krb5_init_context(&context);
@@ -998,6 +1001,7 @@ cc_dir_check_existing(const char *location, uid_t uid,
                       const char *cc_template, bool *_active, bool *_valid)
 {
     bool active = false;
+    bool active_primary = false;
     bool valid = false;
     krb5_ccache ccache = NULL;
     krb5_context context = NULL;
@@ -1006,7 +1010,9 @@ cc_dir_check_existing(const char *location, uid_t uid,
     const char *filename;
     const char *dir;
     char *tmp;
+    char *primary_file;
     errno_t ret;
+    TALLOC_CTX *tmp_ctx;
 
     type = sss_krb5_get_type(location);
     if (type != SSS_KRB5_TYPE_DIR) {
@@ -1027,29 +1033,63 @@ cc_dir_check_existing(const char *location, uid_t uid,
         return EINVAL;
     }
 
-    tmp = talloc_strdup(NULL, filename);
-    if (!tmp) return ENOMEM;
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
+        return ENOMEM;
+    }
+
+    tmp = talloc_strdup(tmp_ctx, filename);
+    if (!tmp) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_strdup failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
 
     dir = dirname(tmp);
     if (!dir) {
         DEBUG(SSSDBG_CRIT_FAILURE,
               ("Cannot base get directory of %s\n", location));
-        return EINVAL;
+        ret = EINVAL;
+        goto done;
     }
 
     ret = cc_residual_is_used(uid, dir, SSS_KRB5_TYPE_DIR, &active);
-    talloc_free(tmp);
     if (ret != EOK) {
-        DEBUG(SSSDBG_OP_FAILURE, ("Could not check if ccache is active. "
-                                  "Will create a new one.\n"));
+        if (ret != ENOENT) {
+            DEBUG(SSSDBG_OP_FAILURE,
+                  ("Could not check if ccache is active.\n"));
+        }
         cc_check_template(cc_template);
         active = false;
+        goto done;
+    }
+
+    /* If primary file isn't in ccache dir, we will ignore it.
+     * But if primary file has wrong permissions, we will fail.
+     */
+    primary_file = talloc_strdup_append(discard_const_p(char, dir),
+                                        "/primary");
+    if (!primary_file) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_strdup_append failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
+    ret = cc_residual_is_used(uid, primary_file, SSS_KRB5_TYPE_FILE,
+                              &active_primary);
+    if (ret != EOK && ret != ENOENT) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Could not check if file 'primary' [%s] in dir ccache"
+               " is active.\n", primary_file));
+        active = false;
+        goto done;
     }
 
     krberr = krb5_init_context(&context);
     if (krberr) {
         DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to init kerberos context\n"));
-        return EIO;
+        ret = EIO;
+        goto done;
     }
 
     krberr = krb5_cc_resolve(context, location, &ccache);
@@ -1081,6 +1121,7 @@ cc_dir_check_existing(const char *location, uid_t uid,
 
     ret = EOK;
 done:
+    talloc_free(tmp_ctx);
     if (ccache) krb5_cc_close(context, ccache);
     krb5_free_context(context);
     *_active = active;
-- 
1.8.2



More information about the sssd-devel mailing list