Ticket History #699: sendauth broken?

>Number: 699
>Category: krb5-libs
>Synopsis: sendauth broken?
>Confidential: yes
>Severity: serious
>Priority: medium
>Responsible: tlyu
>State: closed
>Class: sw-bug
>Submitter-Id: unknown
>Arrival-Date: Mon Mar 08 21:20:00 EST 1999
>Last-Modified: Mon Mar 08 23:02:48 EST 1999
>Originator: papowell@astart.com
>Organization:
>Release:
>Environment:
>Description:

------- Start of forwarded message -------
Date: Mon, 8 Mar 1999 17:52:19 -0800 (PST)
From: papowell@astart.com
Message-Id: <199903090152.RAA15439@astart4.astart.com>
To: mwhitson@MIT.EDU
Subject: Kerberos 5, please forward

I seem to have misfiled/misplaced the email message you
sent to me about the folks working on Kerberos 5. Apparently some bugs
are still in the Kerberos 5 library, especially in the routines
that I use.

File: ./lib/krb5/krb/sendauth.c

Problem: wrong value being returned on function exit.
Problem: dead (unreferenced) malloced() values are not freed on exit.

Original code, approximately line 213:
retval = 0; /* Normal return */
if (out_creds) {
*out_creds = credsp;
}

out_creds is a pointer to where you want the credentials
returned. At line 147 we have the code:


if (!in_creds->ticket.length) {
if ((retval = krb5_get_credentials(context, 0,
use_ccache, in_creds, &credsp)))
goto error_return;
credspout = credsp; <<<<--- value to be returned
} else {
credsp = in_creds;
}

So credspout is set to credsp, saving credsp's value
so that later assignments to credsp does not change it and lose the
returned credentials in the process.

I think what you really want is:

retval = 0; /* Normal return */
if (out_creds) {
*out_creds = credspout;
}


Now, if you cast your eye down a little further, you will discover
that you have:

error_return:
krb5_free_cred_contents(context, &creds);
if (!out_creds && credspout)
* krb5_free_creds(context, credspout);
* if (!ccache && use_ccache)
* krb5_cc_close(context, use_ccache);
return(retval);
}


The lines marked with * have the interesting side effect of
freeing the credspout value, which in the original code has
the sometimes disastrous effects of freeing the return value as well.

I believe that the correct code should be:

error_return:
krb5_free_cred_contents(context, &creds);
! if(out_creds == 0) krb5_free_creds(context, credspout);
! if (!ccache && use_ccache) krb5_cc_close(context, use_ccache);
return(retval);
}

Note that we now free creds - which is ok as it is not the credspout
value at this point.

If there is no out_creds value, we free credspout, eliminating a memory
leak.

Finally, we free close and free up stuff if we are not caching it.

Here is a diff -c of the code.

Enjoy.


Patrick Powell Astart Technologies,
papowell@astart.com 9475 Chesapeake Drive, Suite D,
Network and System San Diego, CA 92123
Consulting 619-874-6543 FAX 619-279-8424
LPRng - Print Spooler (http://www.astart.com)


***************
*** 229,242 ****
}
retval = 0; /* Normal return */
if (out_creds) {
! *out_creds = credspout;
}

error_return:
krb5_free_cred_contents(context, &creds);
! if(out_creds == 0) krb5_free_creds(context, credspout);
! if (!ccache && use_ccache) krb5_cc_close(context, use_ccache);
return(retval);
}

! #endif
--- 213,228 ----
}
retval = 0; /* Normal return */
if (out_creds) {
! *out_creds = credsp;
}

error_return:
krb5_free_cred_contents(context, &creds);
! if (!out_creds && credspout)
! krb5_free_creds(context, credspout);
! if (!ccache && use_ccache)
! krb5_cc_close(context, use_ccache);
return(retval);
}