From mwhitson@MIT.EDU Mon Mar 8 21:19:35 1999 Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id VAA01480 for ; Mon, 8 Mar 1999 21:19:35 -0500 Received: from W20-SPARE-ULTRA2.MIT.EDU by MIT.EDU with SMTP id AA20452; Mon, 8 Mar 99 21:19:14 EST Received: by w20-spare-ultra2 (8.8.8+Sun/4.7) id VAA07990; Mon, 8 Mar 1999 21:19:32 -0500 (EST) Message-Id: Date: 08 Mar 1999 21:19:31 -0500 From: papowell@astart.com (by way of Mike Whitson ) Sender: mwhitson@w20-spare-ultra2.MIT.EDU To: krb5-bugs@MIT.EDU Subject: 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); } ------- End of forwarded message ------- >How-To-Repeat: >Fix: >Audit-Trail: Responsible-Changed-From-To: gnats-admin->krb5-unassigned Responsible-Changed-By: tlyu Responsible-Changed-When: Mon Mar 8 21:39:10 1999 Responsible-Changed-Why: Responsible-Changed-From-To: krb5-unassigned->tlyu Responsible-Changed-By: tlyu Responsible-Changed-When: Mon Mar 8 21:39:16 1999 Responsible-Changed-Why: refiled State-Changed-From-To: open-feedback State-Changed-By: tlyu State-Changed-When: Mon Mar 8 21:39:35 1999 State-Changed-Why: From: Tom Yu To: papowell@astart.com Cc: krb5-bugs@MIT.EDU Subject: Re: krb5-libs/699: sendauth broken? Date: Mon, 8 Mar 1999 22:38:11 -0500 (EST) I got your bug report by way of Mike Whitson. Please send future bug reports directly to krb5-bugs@mit.edu, preferably by using the krb5-send-pr program. Your patches seem to be reversed; please generate them by "diff -c foo.c.orig foo.c" in the future. Also, stating what version of krb5 your're using would also help. It seems that we've seen a bug report from you about this before, [krb5-libs/357]. :-) Anyway it's not clear that you want to return *out_creds = credspout, because if out_creds != NULL and in_creds contains a valid ticket, then you arguably want to return *out_creds = credsp = in_creds. Actually it seems that what you really want to do here is to set credspout = NULL once you assign *out_creds = credsp, so that the cleanup code doesn't go about freeing it, and then conditionalize the freeing of credspout on whether credspout is non-NULL at all, rather than (out_creds == NULL && credspout != NULL). Your patch has the side effect of attempting to free a NULL pointer if error_return is jumped to prior to the allocation of credspout and the user has supplied a NULL out_creds. I've enclosed a patch that I believe to be more correct, against krb5-current, though it probably will apply to krb5-1.0.5 as well. I will also queue this for the krb5-1.0.6 release once I test things out. ---Tom Index: sendauth.c =================================================================== RCS file: /cvs/krbdev/krb5/src/lib/krb5/krb/sendauth.c,v retrieving revision 5.29 diff -u -r5.29 sendauth.c --- sendauth.c 1997/02/07 19:27:28 5.29 +++ sendauth.c 1999/03/09 03:20:07 @@ -213,12 +213,13 @@ } retval = 0; /* Normal return */ if (out_creds) { - *out_creds = credsp; + *out_creds = credsp; + credspout = NULL; } error_return: krb5_free_cred_contents(context, &creds); - if (!out_creds && credspout) + if (credspout != NULL) krb5_free_creds(context, credspout); if (!ccache && use_ccache) krb5_cc_close(context, use_ccache); From: Tom Yu To: krb5-bugs@MIT.EDU Cc: Subject: Re: krb5-libs/699: sendauth broken? Date: Mon, 8 Mar 1999 22:57:10 -0500 (EST) ------- start of forwarded message (RFC 934 encapsulation) ------- Date: Mon, 8 Mar 1999 19:54:21 -0800 (PST) From: papowell@astart.com Message-Id: <199903090354.TAA15544@astart4.astart.com> To: tlyu@MIT.EDU Subject: Re: krb5-libs/699: sendauth broken? Thanks. Makes sense in the context of null pointers being passed. Patrick ------- end ------- State-Changed-From-To: feedback-closed State-Changed-By: tlyu State-Changed-When: Mon Mar 8 23:02:22 1999 State-Changed-Why: patched and queued for 1.0.6 >Unformatted: