Skip Menu |
 

History Show all quoted text — Show full headers
# Sun Dec 25 22:00:22 2005 hartmans (Sam Hartman) - 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)
Download (untitled) / with headers
text/plain 3.1KiB

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 */
# Sun Dec 25 22:04:24 2005 hartmans (Sam Hartman) - Target_Version 1.4.4 added
# Mon Dec 26 13:42:04 2005 tlyu (Taylor Yu) - 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:
Download (untitled) / with headers
text/plain 77B
Note that this patch introduces a memory leak under some conditions.

---Tom
# Mon Dec 26 17:04:47 2005 tlyu (Taylor Yu) - 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:
Download (untitled) / with headers
text/plain 2.3KiB
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 */

# Mon Dec 26 17:06:59 2005 tlyu (Taylor Yu) - Status changed from 'new' to 'open'
# Mon Dec 26 17:07:00 2005 tlyu (Taylor Yu) - Given to tlyu (Taylor Yu)
# Mon Dec 26 17:07:01 2005 tlyu (Taylor Yu) - Component krb5-libs added
# Mon Dec 26 17:07:01 2005 tlyu (Taylor Yu) - Version_reported 1.4.3 added
# Tue Dec 27 01:19:44 2005 jaltman (Jeffrey Altman) - Correspondence added
Download (untitled) / with headers
text/plain 521B
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 gc_frm_kdc-diff-3.txt
text/plain 2.5KiB
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;
}
}
}
# Tue Dec 27 20:52:03 2005 tlyu (Taylor Yu) - 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:
Download (untitled) / with headers
text/plain 492B
[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
# Wed Dec 28 00:27:03 2005 "Jeffrey Altman [Kermit Project]" <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:
Download (untitled) / with headers
text/plain 603B
Tom Yu via RT wrote:
Show quoted text
> [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 smime.p7s
application/x-pkcs7-signature 3.1KiB

Message body not shown because it is not plain text.

# Wed Dec 28 18:02:39 2005 tlyu (Taylor Yu) - Status changed from 'open' to 'resolved'
# Wed Dec 28 18:02:40 2005 tlyu (Taylor Yu) - Tags pullup added
# Wed Dec 28 18:02:41 2005 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: CVS Commit
Download (untitled) / with headers
text/plain 441B
* 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
# Wed Dec 28 19:28:43 2005 tlyu (Taylor Yu) - 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:
Download (untitled) / with headers
text/plain 126B
The preceding commit is Jeff's change minus the hunks at 160 (dead
code) and 422 (remove style nit patch to minimize change).
# Wed Mar 08 17:25:38 2006 tlyu (Taylor Yu) - Version_Fixed 1.4.4 added
# Wed Mar 08 17:25:38 2006 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: CVS Commit
Download (untitled) / with headers
text/plain 178B
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
# Wed Dec 16 18:02:45 2015 tlyu (Taylor Yu) - CustomField pullup deleted