[krbdev.mit.edu #3313] doublefree in gc_frm_kdc.c

[Sam Hartman via RT]

Hi.  There is public discussion of a double free at
http://bugs.debian.org/344543 .

Here's the bug report and patch thanks to Jeff Altman:

Today I discovered that with the latest release it is possible to generate a double-free condition
in krb5_get_credentials() if the acquisition of a service ticket via a multi-hop cross-realm authentication
fails due to a policy error.  In this case, krb5_get_cred_from_kdc_opt() obtains valid TGTs
and subsequently destroys them before returning to the caller.

The cause of the problem is that the variable 'free_tgt' does not properly track the state of the 'tgt'
variable.  'free_tgt' must only be set to a non-zero value iff 'tgt' is storing a cred that is not stored
in the 'ret_tgts' array.   The existing code has two errors that are each repeated two times.   One,
after assigning "tgt = *ret_tgts[i];"  the value of 'free_tgt' is not reset to 0.  Two, the value of 'free_tgt'
is set to 1 within each iteration of the loop without regards to whether or not the value of 'tgt' changed.

The patch provided below correctly tracks the behavior.  

I am not convinced a problem only occurs in the cross realm case.

Jeffrey Altman



Index: ChangeLog
===================================================================
--- ChangeLog   (revision 17479)
+++ ChangeLog   (working copy)
@@ -1,3 +1,10 @@
+2005-11-17  Jeffrey Altman <jaltman at mit.edu>
+
+       * gc_frm_kdc.c (krb5_get_cred_from_kdc_opt):
+         properly track the state of 'tgt' via 'free_tgt' so that
+          we can avoid double-free'ing memory when we return to
+          krb5_get_credentials().
+
 2005-10-19  Ken Raeburn  <raeburn at mit.edu>

        * Makefile.in (t_ser): Add dl library and thread link options,
Index: gc_frm_kdc.c
===================================================================
--- gc_frm_kdc.c        (revision 17479)
+++ gc_frm_kdc.c        (working copy)
@@ -251,7 +251,6 @@
                /* Copy back in case invalided */
                tgt = otgt;
                free_otgt = 0;
-               free_tgt = 1;
                if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
                    retval = KRB5_PROG_ETYPE_NOSUPP;
                    goto cleanup;
@@ -325,7 +324,6 @@

                            tgt = otgt;
                            free_otgt = 0;
-                           free_tgt = 1;
                            if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
                                retval = KRB5_PROG_ETYPE_NOSUPP;
                                goto cleanup;
@@ -365,6 +363,7 @@
                            }

                            tgt = *ret_tgts[ntgts++];
+                           free_tgt = 0;
                        }

                        /* got one as close as possible, now start all over */
@@ -413,12 +412,11 @@
                krb5_free_creds(context, tgtr);
                tgtr = NULL;

-               if (free_tgt) {
+               if (free_tgt)
                    krb5_free_cred_contents(context, &tgt);
-                   free_tgt = 0;
-               }

                tgt = *ret_tgts[ntgts++];
+               free_tgt = 0;

                /* we're done if it is the target */