[krbdev.mit.edu #8579] AutoReply: duplicate caching of some cross-realm TGTs

["Richard E. Silverman" via RT]

>From deae811366b0e0a41c57ae5ec8662a3eac938a2f Mon Sep 17 00:00:00 2001
From: "Richard E. Silverman" <res at arcesium.com>
Date: Tue, 18 Apr 2017 22:20:50 -0400
Subject: [PATCH] Fix repeated caching of certain credentials.

Under certain circumstances, we would get duplicate caching of cross-realm
tickets. krb5_cc_store_cred() had two problems:

1) It avoided duplicate caching the second time it (potentially) stored a
   credential, but not the first time (which is where this was happening).

2) The method it used to avoid duplicates was flawed anyway. It called
   krb5_cc_remove_cred() before storing, to first remove any matching
   credentials. This seems like an odd approach to begin with... but
   worse: krb5_cc_remove_cred() is actually no-op for the file ccache
   type! (see cc_file.c:fcc_remove_cred()).

Fix this by storing the credential only if there is no matching one
already in the ccache.

I'm not at all sure that this is the right approach, as there are a number
of questions: Was the omission of the first duplicate check intentional?
Was there some reason for using the remove-then-add approach? Some other
cross-realm TGTs aren't cached at all, and we don't get such duplicate
under other circumstances -- so is the duplicate checking supposed to
happen elsewhere? Etc.
---
 src/lib/krb5/ccache/ccfns.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/lib/krb5/ccache/ccfns.c b/src/lib/krb5/ccache/ccfns.c
index 1084d519..2047ac8b 100644
--- a/src/lib/krb5/ccache/ccfns.c
+++ b/src/lib/krb5/ccache/ccfns.c
@@ -77,6 +77,26 @@ krb5_cc_close(krb5_context context, krb5_ccache cache)
 }
 
 krb5_error_code KRB5_CALLCONV
+krb5_cc_store_cred_nodup(krb5_context, krb5_ccache, krb5_creds *);
+
+krb5_error_code KRB5_CALLCONV
+krb5_cc_store_cred_nodup(krb5_context context, krb5_ccache cache,
+                         krb5_creds *creds)
+{
+    krb5_creds discard;
+
+    /* Store this credential only if there is no matching one already in
+       the cache. */
+    if (krb5_cc_retrieve_cred(context, cache, KRB5_TC_MATCH_AUTHDATA, creds,
+                              &discard)) {
+        return cache->ops->store(context, cache, creds);
+    } else {
+        krb5_free_cred_contents(context, &discard);
+        return 0;
+    }
+}
+
+krb5_error_code KRB5_CALLCONV
 krb5_cc_store_cred(krb5_context context, krb5_ccache cache,
                    krb5_creds *creds)
 {
@@ -85,7 +105,7 @@ krb5_cc_store_cred(krb5_context context, krb5_ccache cache,
     krb5_principal s1, s2;
 
     TRACE_CC_STORE(context, cache, creds);
-    ret = cache->ops->store(context, cache, creds);
+    ret = krb5_cc_store_cred_nodup(context, cache, creds);
     if (ret) return ret;
 
     /*
@@ -100,9 +120,7 @@ krb5_cc_store_cred(krb5_context context, krb5_ccache cache,
     if (!krb5_principal_compare(context, s1, s2)) {
         creds->server = s2;
         TRACE_CC_STORE_TKT(context, cache, creds);
-        /* remove any dups */
-        krb5_cc_remove_cred(context, cache, KRB5_TC_MATCH_AUTHDATA, creds);
-        ret = cache->ops->store(context, cache, creds);
+        ret = krb5_cc_store_cred_nodup(context, cache, creds);
         creds->server = s1;
     }
     krb5_free_ticket(context, tkt);
-- 
2.11.1