RT RT/krbdev.mit.edu: Ticket #3313 doublefree in gc_frm_kdc.c Signed in as guest.
[Logout]

[Home] [Search] [Configuration]

[Display] [History] [Basics] [Dates] [People] [Links] [Jumbo]

 
 

 The Basics  
Id
3313
Status
resolved
Worked
0 min
Priority
0/0
Queue
krb5
 

 Keyword Selections  
Component
  • krb5-libs
Version_reported
  • 1.4.3
Version_Fixed
  • 1.4.4
Target_Version
  • 1.4.4
Tags
  • pullup
 

 Relationships  
Depends on:
Depended on by:
Parents:
Children:

Refers to:
Referred to by:
 
 Dates  
Created: Sun Dec 25 22:00:21 2005
Starts: Not set
Started: Mon Dec 26 17:06:59 2005
Last Contact: Wed Mar 8 17:25:41 2006
Due: Not set
Updated: Wed Mar 8 17:25:41 2006 by tlyu
 

 People  
Owner
 tlyu
Requestors
 hartmans@mit.edu
Cc
 
AdminCc
 
 

 More about Sam Hartman  
Comments about this user:
No comment entered about this user
This user's 25 highest priority tickets:
 

History   Display mode: [Brief headers] [Full headers]
      Sun Dec 25 22:00:22 2005  hartmans - Ticket created    
     
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 */


Download (untitled) 3.1k
      Sun Dec 25 22:04:24 2005  hartmans - Target_Version 1.4.4 added    
      Mon Dec 26 13:42:04 2005  tlyu - Correspondence added    
     
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


Download (untitled) 77b
      Mon Dec 26 17:04:47 2005  tlyu - Correspondence added    
     
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 */



Download (untitled) 2.3k
      Mon Dec 26 17:06:59 2005  tlyu - Status changed from new to open    
      Mon Dec 26 17:07:00 2005  tlyu - Given to tlyu    
      Mon Dec 26 17:07:01 2005  tlyu - Component krb5-libs added    
      Mon Dec 26 17:07:01 2005  tlyu - Version_reported 1.4.3 added    
      Tue Dec 27 01:19:44 2005  jaltman - Correspondence added    
     
 

     
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

Download (untitled) 521b
     
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;
 	    }
 	}
     }

Download gc_frm_kdc-diff-3.txt 2.5k
      Tue Dec 27 20:52:03 2005  tlyu - Correspondence added    
     
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


Download (untitled) 492b
      Wed Dec 28 00:27:03 2005  jaltman@columbia.edu - Correspondence added    
     
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

Download (untitled) 603b
     
 
Download smime.p7s 3.1k
      Wed Dec 28 18:02:39 2005  tlyu - Status changed from open to resolved    
      Wed Dec 28 18:02:40 2005  tlyu - Tags pullup added    
      Wed Dec 28 18:02:41 2005  tlyu - Correspondence added    
     
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


Download (untitled) 441b
      Wed Dec 28 19:28:43 2005  tlyu - Comments added    
     
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).


Download (untitled) 126b
      Wed Mar  8 17:25:38 2006  tlyu - Version_Fixed 1.4.4 added    
      Wed Mar  8 17:25:38 2006  tlyu - Correspondence added    
     
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


Download (untitled) 178b