krb5-1.12 - KDC does not properly log client principal for failed TGS_REQ

[Richard Basch]

While logging was previously fixed to record the service principal (fixed in
1.12.1), the client principal is not logged during certain errors (e.g.
Ticket expired). I believe the following code fix will address the problem.
I will commit this to my github repository, but I first wanted to see if
anyone sees any issue with this fix.

In particular, it changes the behavior of the cleanup routine within
krb5_rd_req_decoded_opt(), but as far as I can tell, if people are using the
API correctly, this should be relatively safe, but I am curious if anyone
foresees a condition where this might result in a memory leak in an
application which is conformant to the API or any other ill effects.

Thoughts/feedback?


-----Original Message-----
From: Richard Basch [mailto:probe at mail.bright-prospects.com] 
Sent: Monday, June 2, 2014 12:39 AM
To: basch at alum.mit.edu; richard.basch at gs.com
Subject: 

diff --git a/src/kdc/kdc_util.c b/src/kdc/kdc_util.c index 5409078..1379dbd
100644
--- a/src/kdc/kdc_util.c
+++ b/src/kdc/kdc_util.c
@@ -345,6 +345,8 @@ cleanup_auth_context:
 
 cleanup:
     if (retval != 0) {
+        if (!*ticket && apreq && apreq->ticket &&
apreq->ticket->enc_part2)
+            krb5_copy_ticket(kdc_context, apreq->ticket, ticket);
         krb5_free_keyblock(kdc_context, *tgskey);
         *tgskey = NULL;
     }
diff --git a/src/lib/krb5/krb/rd_req_dec.c b/src/lib/krb5/krb/rd_req_dec.c
index 4b952f5..3151f62 100644
--- a/src/lib/krb5/krb/rd_req_dec.c
+++ b/src/lib/krb5/krb/rd_req_dec.c
@@ -561,14 +561,17 @@ rd_req_decoded_opt(krb5_context context,
krb5_auth_context *auth_context,
             *ap_req_options |= AP_OPTS_USE_SUBKEY;
     }
 
-    retval = 0;
-
 cleanup:
     if (desired_etypes != NULL)
         free(desired_etypes);
     if (permitted_etypes != NULL &&
         permitted_etypes != (*auth_context)->permitted_etypes)
         free(permitted_etypes);
+#if 0
+    /*
+     * Apps should call krb5_free_ticket, so this should not be required.
+     * The KDC and other apps needs the output even in some error
conditions.
+     */
     if (retval) {
         /* only free if we're erroring out...otherwise some
            applications will need the output. */ @@ -576,6 +579,7 @@
cleanup:
             krb5_free_enc_tkt_part(context, req->ticket->enc_part2);
         req->ticket->enc_part2 = NULL;
     }
+#endif
     if (check_valid_flag)
         krb5_free_keyblock_contents(context, &decrypt_key);