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

Lukas Slebodnik lslebodn at redhat.com
Fri Apr 12 22:31:24 UTC 2013


On (12/04/13 18:06), Jakub Hrozek wrote:
>On Fri, Apr 12, 2013 at 05:31:33PM +0200, Jakub Hrozek wrote:
>> On Thu, Apr 11, 2013 at 12:06:22PM +0200, Lukas Slebodnik wrote:
>> > On (11/04/13 10:29), Jakub Hrozek wrote:
>> > >On Thu, Apr 11, 2013 at 10:28:01AM +0200, Jakub Hrozek wrote:
>> > >> Just one nitpick, can you change this block:
>> > >> 
>> > >> +    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;
>> > >> +    }
>> > >> 
>> > >> Change dir to be "char *" not "const char*" and use
>> > >> talloc_asprintf(tmp_ctx, "%s/%s", dir);
>> > >
>> > >talloc_asprintf(tmp_ctx, "%s/primary", dir);
>> > >
>> > >sorry.
>> > 
>> > Patch attached.
>> > 
>> > LS
>> 
>> Ack, works fine. I will just change one thing before pushing, no need to
>> re-send:
>> 
>> >      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"));
>> > +        }
>> 
>> I will just change the debug level here to something more verbose. I
>> think we should let the user know why the login failed.
>
>Pushed to master.
>
>Lukas, can you also send a version that applies cleanly on the sssd-1-9
>branch? I think we should fix this bug in 1.9 too.

Attaching patch for branch sssd-1-9.
I did not test patch on RHEL 6, I have to create VM.

LS
-------------- next part --------------
>From 638e44edc83f0d82aa3a9d73c2c06199d0c60d7f 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 | 108 +++++++++++++++++++++++++++-------------
 2 files changed, 85 insertions(+), 35 deletions(-)

diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index fb37ad4f6876f6fb354793908a1efe41ea47f66d..e2172e98efcfccdf34646a51712e9f6a526b8e32 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",
@@ -467,7 +472,12 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
             ret = check_old_ccache(ccache_file, kr, realm,
                                    &kr->active_ccache_present,
                                    &kr->valid_tgt_present);
-            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 b404301e7ed35f3ccc193f64989f2a33a488fcb7..d85ca20d193abc96826f581622341610250d2ec6 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -775,40 +775,40 @@ 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,
               ("stat failed [%d][%s].\n", ret, strerror(ret)));
         return ret;
-    } else if (ret == EOK) {
-        if (stat_buf.st_uid != uid) {
-            DEBUG(SSSDBG_OP_FAILURE,
-                  ("Cache file [%s] exists, but is owned by [%d] instead of "
-                   "[%d].\n", ccname, stat_buf.st_uid, uid));
-            return EINVAL;
-        }
+    }
 
-        switch (type) {
+    if (stat_buf.st_uid != uid) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Cache file [%s] exists, but is owned by [%d] instead of "
+               "[%d].\n", ccname, stat_buf.st_uid, uid));
+        return EINVAL;
+    }
+
+    switch (type) {
 #ifdef HAVE_KRB5_DIRCACHE
-            case SSS_KRB5_TYPE_DIR:
-                ret = S_ISDIR(stat_buf.st_mode);
-                break;
+        case SSS_KRB5_TYPE_DIR:
+            ret = S_ISDIR(stat_buf.st_mode);
+            break;
 #endif /* HAVE_KRB5_DIRCACHE */
-            case SSS_KRB5_TYPE_FILE:
-                ret = S_ISREG(stat_buf.st_mode);
-                break;
-            default:
-                DEBUG(SSSDBG_CRIT_FAILURE, ("Unsupported ccache type\n"));
-                return EINVAL;
-        }
-
-        if (ret == 0) {
-            DEBUG(SSSDBG_OP_FAILURE,
-                  ("Cache file [%s] exists, but is not the expected type\n",
-                   ccname));
+        case SSS_KRB5_TYPE_FILE:
+            ret = S_ISREG(stat_buf.st_mode);
+            break;
+        default:
+            DEBUG(SSSDBG_CRIT_FAILURE, ("Unsupported ccache type\n"));
             return EINVAL;
-        }
+    }
+
+    if (ret == 0) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Cache file [%s] exists, but is not the expected type\n",
+              ccname));
+        return EINVAL;
     }
 
     ret = check_if_uid_is_active(uid, &active);
@@ -868,10 +868,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);
@@ -997,6 +1000,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;
@@ -1005,7 +1009,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) {
@@ -1026,29 +1032,62 @@ 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_asprintf(tmp_ctx, "%s/primary", dir);
+    if (!primary_file) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf 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);
@@ -1080,6 +1119,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.1.4



More information about the sssd-devel mailing list