To: krb5-bugs@MIT.EDU
Subject: doublefree in gc_frm_kdc.c
Date: Sun, 25 Dec 2005 21:59:41 -0500 (EST)
From: hartmans@MIT.EDU (Sam Hartman)


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@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@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 */

To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3313] doublefree in gc_frm_kdc.c
From: Tom Yu <tlyu@MIT.EDU>
Date: Mon, 26 Dec 2005 13:41:53 -0500
RT-Send-Cc: 

Note that this patch introduces a memory leak under some conditions.

---Tom

To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3313] doublefree in gc_frm_kdc.c
From: Tom Yu <tlyu@MIT.EDU>
Date: Mon, 26 Dec 2005 17:04:42 -0500
RT-Send-Cc: 

I have come up with the following improved patch.  It should take care
of a few double-free situations not caught by Jeff's patch, as well as
some memory leaks introduced by Jeff's patch.  I'm still not convinced
that certain improbable error conditions won't result in additional
problems, but I think they're limited to null pointer derefs caused by
pre-existing code in the function.

Index: gc_frm_kdc.c
===================================================================
--- gc_frm_kdc.c	(revision 17577)
+++ gc_frm_kdc.c	(working copy)
@@ -231,13 +231,14 @@
 		goto cleanup;

 	    otgt = tgt;
-	    free_otgt = 1;
+	    free_otgt = free_tgt;
 	    free_tgt = 0;

 	    retval = krb5_cc_retrieve_cred(context, ccache, retr_flags,
 					   &tgtq, &tgt);
 	    if (retval == 0) {
-	        krb5_free_cred_contents(context, &otgt);
+		if (free_otgt)
+		    krb5_free_cred_contents(context, &otgt);
 		free_otgt = 0;
 	        free_tgt = 1;
 		/* We are now done - proceed to got/finally have tgt */
@@ -250,8 +251,8 @@
 		/* with current tgt.                              */
 		/* Copy back in case invalided */
 		tgt = otgt;
+		free_tgt = free_otgt;
 		free_otgt = 0;
-		free_tgt = 1;
 		if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
 		    retval = KRB5_PROG_ETYPE_NOSUPP;
 		    goto cleanup;
@@ -305,7 +306,7 @@
 			    goto cleanup;

 			otgt = tgt;
-			free_otgt = 1;
+			free_otgt = free_tgt;
 			free_tgt = 0;
 			retval = krb5_cc_retrieve_cred(context, ccache,
 							    retr_flags,
@@ -324,8 +325,8 @@
 			    /* not in the cache so try and get one with our current tgt. */

 			    tgt = otgt;
+			    free_tgt = free_otgt;
 			    free_otgt = 0;
-			    free_tgt = 1;
 			    if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
 				retval = KRB5_PROG_ETYPE_NOSUPP;
 				goto cleanup;
@@ -363,8 +364,11 @@
 				krb5_free_cred_contents(context, &otgt);
 				free_otgt = 0;
 			    }
+			    if (free_tgt)
+				krb5_free_cred_contents(context, &tgt);

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

 			/* got one as close as possible, now start all over */
@@ -413,12 +417,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 */

Here is a revision to tom's latest patch.  I am more comfortable with
testing the 'free_tgt' and 'free_otgt' variables and freeing the data
structures at the time we need to replace the value instead of
attempting to make sure that we always clean up after the code as soon
as we think we might be done to put things into the correct state for
later re-use.

I have verified that all of the 'tgt', 'otgt', 'tgtq' and 'tgtr'
variables are appropriately initialized and freed regardless of the
error states.

Jeffrey Altman

Index: gc_frm_kdc.c
===================================================================
--- gc_frm_kdc.c
(revision 17555)
+++ gc_frm_kdc.c	(working copy)
@@ -160,9 +160,13 @@
 	if
((retval = krb5_copy_principal(context, int_server, &tgtq.server)))
 	    goto cleanup;

+	if (free_tgt)
+	    krb5_free_cred_contents(context, &tgt);
+
 	if ((retval = krb5_cc_retrieve_cred(context, ccache,
 					    retr_flags,
 					    &tgtq, &tgt))) {
+	    free_tgt = 0;
 	    goto cleanup;
 	}
 	free_tgt = 1;
@@ -230,15 +234,15 @@
 					      &tgtq.server)))
 		goto cleanup;

+	    if (free_otgt)
+		krb5_free_cred_contents(context, &otgt);
 	    otgt = tgt;
-	    free_otgt = 1;
+	    free_otgt = free_tgt;
 	    free_tgt = 0;

 	    retval = krb5_cc_retrieve_cred(context, ccache, retr_flags,
 					   &tgtq, &tgt);
 	    if (retval == 0) {
-	        krb5_free_cred_contents(context, &otgt);
-		free_otgt = 0;
 	        free_tgt = 1;
 		/* We are now done - proceed to got/finally have tgt */
 	    } else {
@@ -250,8 +254,8 @@
 		/* with current tgt.                              */
 		/* Copy back in case invalided */
 		tgt = otgt;
+		free_tgt = free_otgt;
 		free_otgt = 0;
-		free_tgt = 1;
 		if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
 		    retval = KRB5_PROG_ETYPE_NOSUPP;
 		    goto cleanup;
@@ -304,16 +308,15 @@
 							  &tgtq.server)))
 			    goto cleanup;

+			if (free_otgt)
+			    krb5_free_cred_contents(context, &otgt);
 			otgt = tgt;
-			free_otgt = 1;
+			free_otgt = free_tgt;
 			free_tgt = 0;
 			retval = krb5_cc_retrieve_cred(context, ccache,
 							    retr_flags,
 							    &tgtq, &tgt);
 			if (retval == 0) {
-			    if (free_otgt)
-			      krb5_free_cred_contents(context, &otgt);
-			    free_otgt = 0;
 			    free_tgt = 1;
 			    /* Continues with 'got one as close as possible' */
 			} else {
@@ -324,8 +327,8 @@
 			    /* not in the cache so try and get one with our current
tgt. */

 			    tgt = otgt;
+			    free_tgt = free_otgt;
 			    free_otgt = 0;
-			    free_tgt = 1;
 			    if (!krb5_c_valid_enctype(tgt.keyblock.enctype)) {
 				retval = KRB5_PROG_ETYPE_NOSUPP;
 				goto cleanup;
@@ -359,9 +362,9 @@
 			    krb5_free_creds(context, tgtr);
 			    tgtr = NULL;

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

 			    tgt = *ret_tgts[ntgts++];
@@ -422,7 +425,8 @@
 
 		/* we're done if it is the target */

-		if (!*next_server++) break;
+		if (!*next_server++)
+		    break;
 	    }
 	}
     }

To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3313] doublefree in gc_frm_kdc.c
From: Tom Yu <tlyu@MIT.EDU>
Date: Tue, 27 Dec 2005 20:51:53 -0500
RT-Send-Cc: 

[Line numbers refer to file after application of Jeff's patch.]

The extra call to free_cred_contents() at line 164 will never be
executed, as there is no way for free_tgt to be 1 at that point.
Otherwise, it appears to be functionally nearly identical to my
patch.  Jeff, were there any cases your patch covers which mine does
not?  I didn't think there were, but I wanted to make sure I wasn't
missing something.

This file badly needs rewriting.  (I've got a rewrite in progress.)

---Tom

Date: Wed, 28 Dec 2005 00:26:48 -0500
From: Jeffrey Altman <jaltman@columbia.edu>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3313] doublefree in gc_frm_kdc.c
RT-Send-Cc: 

 

Tom Yu via RT wrote:
> [Line numbers refer to file after application of Jeff's patch.]
>
> The extra call to free_cred_contents() at line 164 will never be
> executed, as there is no way for free_tgt to be 1 at that point.
> Otherwise, it appears to be functionally nearly identical to my
> patch.  Jeff, were there any cases your patch covers which mine does
> not?  I didn't think there were, but I wanted to make sure I wasn't
> missing something.

The new patch does not cover any new cases but it is less likely to
be susceptible to similar bugs in the future if changes are made.

Jeffrey Altman

From: tlyu@mit.edu
Subject: CVS Commit

	* gc_frm_kdc.c (krb5_get_cred_from_kdc_opt): Cause free_tgt and
	free_otgt to track the states of tgt and otgt correctly, to avoid
	a double-free condition which previously happened when this
	function returned to krb5_get_credentials(), which proceeded to
	free a previously freed TGT in the returned TGT list.

Commit By: tlyu



Revision: 17578
Changed Files:
U   trunk/src/lib/krb5/krb/ChangeLog
U   trunk/src/lib/krb5/krb/gc_frm_kdc.c

To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #3313] CVS Commit
From: Tom Yu <tlyu@MIT.EDU>
Date: Wed, 28 Dec 2005 19:28:33 -0500
RT-Send-Cc: 

The preceding commit is Jeff's change minus the hunks at 160 (dead
code) and 422 (remove style nit patch to minimize change).

From: tlyu@mit.edu
Subject: CVS Commit

pull up r17578 from trunk

Commit By: tlyu



Revision: 17718
Changed Files:
U   branches/krb5-1-4/src/lib/krb5/krb/ChangeLog
U   branches/krb5-1-4/src/lib/krb5/krb/gc_frm_kdc.c