Ticket History #6284: memory leaks in error conditions

# Thu Dec 04 10:46:09 2008 tsitkova (Zhanna Tsitkova) - Ticket created

Subject:

memory leaks in error conditions

Download (untitled) / with headers
text/plain 239B

If error condition occurs, some buffers won't be freed in kdc/do_as_req.c and kadmin/server/schpw.c

Vendor's priority - task
Vendor's patch - LHA-6331022-chpw-leak ;
LHA-6331022-krb5kdc-pws-plug-memory-leak

# Thu Dec 04 10:46:09 2008 tsitkova (Zhanna Tsitkova) - Ticket 6284 MemberOf ticket 6204.

# Thu Dec 04 13:42:44 2008 tsitkova (Zhanna Tsitkova) - Comments added

Download (untitled) / with headers
text/plain 19B

Including the patch

Download LHA-6331022-chpw-leak.patch
text/plain 1.2KiB

diff -Nur -x '*~' -x '*.orig' -x '*.rej' -x '*.pbxbtree' -x '*.pbxindex' -x lha.mode1v3 -x lha.mode2v3 -x lha.pbxuser -x windows -x .DS_Store Kerberos.AEP-6.5fc1.orig/KerberosFramework/Kerberos5/Sources/kadmin/server/schpw.c Kerberos.AEP-6.5fc1/KerberosFramework/Kerberos5/Sources/kadmin/server/schpw.c
--- Kerberos.AEP-6.5fc1.orig/KerberosFramework/Kerberos5/Sources/kadmin/server/schpw.c 2008-11-09 17:47:50.000000000 -0800
+++ Kerberos.AEP-6.5fc1/KerberosFramework/Kerberos5/Sources/kadmin/server/schpw.c 2008-11-09 19:22:59.000000000 -0800
@@ -37,6 +37,7 @@
char *clientstr;
size_t clen;
char *cdots;
+ const char *errmsg = NULL;

ret = 0;
rep->length = 0;
@@ -221,11 +222,15 @@

clen = strlen(clientstr);
trunc_name(&clen, &cdots);
+ if (ret)
+ errmsg = krb5_get_error_message (context, ret);
krb5_klog_syslog(LOG_NOTICE, "chpw request from %s for %.*s%s: %s",
inet_ntoa(sockfrom->sin_addr),
(int) clen, clientstr, cdots,
- ret ? krb5_get_error_message (context, ret) : "success");
+ errmsg ? errmsg : "success");
krb5_free_unparsed_name(context, clientstr);
+ if (errmsg)
+ krb5_free_error_message(context, errmsg);

if (ret) {
if ((ret != KADM5_PASS_Q_TOOSHORT) &&

# Thu Dec 04 13:43:29 2008 tsitkova (Zhanna Tsitkova) - Comments added

Download (untitled) / with headers
text/plain 22B

Including the patch #2

Download LHA-6331022-krb5kdc-pws-plug-memory-leak.patch
text/plain 1.7KiB

diff -Nur -x '*~' -x '*.orig' -x '*.rej' -x '*.pbxbtree' -x '*.pbxindex' -x lha.mode1v3 -x lha.mode2v3 -x lha.pbxuser -x windows -x .DS_Store Kerberos.AEP-6.5fc1.orig/KerberosFramework/Kerberos5/Sources/kdc/do_as_req.c Kerberos.AEP-6.5fc1/KerberosFramework/Kerberos5/Sources/kdc/do_as_req.c
--- Kerberos.AEP-6.5fc1.orig/KerberosFramework/Kerberos5/Sources/kdc/do_as_req.c 2008-11-09 21:03:38.000000000 -0800
+++ Kerberos.AEP-6.5fc1/KerberosFramework/Kerberos5/Sources/kdc/do_as_req.c 2008-11-09 22:05:27.000000000 -0800
@@ -443,18 +443,15 @@
if(kdc_notify_pws_apple || kdc_active_realm->realm_pws_enabled){
errcode = kdc_update_pws(cname, 0, 1, server);
if (errcode) {
- status = "CHECK_PWS_ACCT";
- goto errout;
+ krb5_free_data(kdc_context, *response);
+ *response = NULL;
+ status = "CHECK_PWS_ACCT";
+ goto errout;
}
}
#endif

- /* these parts are left on as a courtesy from krb5_encode_kdc_rep so we
- can use them in raw form if needed. But, we don't... */
- memset(reply.enc_part.ciphertext.data, 0, reply.enc_part.ciphertext.length);
- free(reply.enc_part.ciphertext.data);
-
rep_etypes2str(rep_etypestr, sizeof(rep_etypestr), &reply);
krb5_klog_syslog(LOG_INFO,
"AS_REQ (%s) %s: ISSUE: authtime %d, "
@@ -475,6 +472,11 @@

errout:

+ /* these parts are left on as a courtesy from krb5_encode_kdc_rep so we
+ can use them in raw form if needed. But, we don't... */
+ memset(reply.enc_part.ciphertext.data, 0, reply.enc_part.ciphertext.length);
+ free(reply.enc_part.ciphertext.data);
+
#ifdef APPLE_KDC_MODS
/* call code to notify PWS of the failure iff not NEEDS_Preauth */
if(kdc_notify_pws_apple || kdc_active_realm->realm_pws_enabled){

# Thu Dec 04 15:09:43 2008 tsitkova (Zhanna Tsitkova) - Cc lha (Love Hornquist Astrand) added

# Fri Dec 05 13:51:24 2008 tsitkova (Zhanna Tsitkova) - Given to ghudson@mit.edu (Greg Hudson)

# Thu Jan 22 14:10:22 2009 ghudson@mit.edu (Greg Hudson) - Correspondence added

Download (untitled) / with headers
text/plain 634B

First patch looks fine; I adapted it to the current code (which was
changed fairly heavily by Luke) and will commit shortly.

I don't understand the second patch. The first hunk appears to be for
Apple-specific code, so I'm ignoring that. The other hunks move a
free(reply.enc_part.ciphertext.data) into the errorout label. But (a)
that move seems unnecessary, since in the old location the data was
freed immediately after it was allocated (i.e. on successful return from
krb5_encode_kdc_rep), and (b) that move seems incorrect, since "goto
errout" can happen in cases where reply.enc_part.ciphertext.data hasn't
been initialized.

# Thu Jan 22 14:19:38 2009 ghudson@mit.edu (Greg Hudson) - Status changed from 'new' to 'open'

# Thu Jan 22 14:19:38 2009 ghudson@mit.edu (Greg Hudson) - Correspondence added

From:	ghudson@mit.edu
Subject:	SVN Commit

Download (untitled) / with headers
text/plain 268B

Adapted patch from Apple: in kadmind's process_chpw_request, make sure
to free error message strings.

https://github.com/krb5/krb5/commit/6422c0b5646f4f8e40108c167c50d7aea9abb6da
Commit By: ghudson
Revision: 21776
Changed Files:
U trunk/src/kadmin/server/schpw.c

# Thu Jan 22 19:21:49 2009 lha (Love Hornquist Astrand) - Comments added

From:	Love Hörnquist Åstrand <lha@apple.com>
To:	Zhanna Tsitkova via RT <rt-comment@krbdev.mit.edu>
Subject:	Re: [krbdev.mit.edu #6284] memory leaks in error conditions
Date:	Thu, 22 Jan 2009 16:21:47 -0800
RT-Send-Cc:

Download (untitled) / with headers
text/plain 863B

22 jan 2009 kl. 11.10 skrev Greg Hudson via RT:

Show quoted text

> First patch looks fine; I adapted it to the current code (which was
> changed fairly heavily by Luke) and will commit shortly.
>
> I don't understand the second patch. The first hunk appears to be for
> Apple-specific code, so I'm ignoring that. The other hunks move a
> free(reply.enc_part.ciphertext.data) into the errorout label. But (a)
> that move seems unnecessary, since in the old location the data was
> freed immediately after it was allocated (i.e. on successful return
> from
> krb5_encode_kdc_rep), and (b) that move seems incorrect, since "goto
> errout" can happen in cases where reply.enc_part.ciphertext.data
> hasn't
> been initialized.

There needs to be an if(reply.enc_part.ciphertext.data) protecting.

The reason we need this is that the pws case add an extra goto errout;

Love

# Tue Mar 20 11:42:50 2018 ghudson@mit.edu (Greg Hudson) - Status changed from 'open' to 'resolved'

# Tue Mar 20 11:42:50 2018 ghudson@mit.edu (Greg Hudson) - Tags nochange added