[709] in Kerberos-V5-bugs
some K5 bugs...
daemon@ATHENA.MIT.EDU (grossa@SDSC.EDU)
Tue Sep 6 10:42:13 1994
Date: Tue, 6 Sep 94 07:42:55 PDT
From: grossa@SDSC.EDU
To: krb5-bugs@MIT.EDU
Hello,
Here are some problems I found with Kerberos V5 beta 4 pl 2 .
I'm running on a NeXT with NS3.2 . I've found 4 memory leaks, a minor
problem with kdb5_mkdums, 3 minor NeXT related configuration problems,
and a copy of the last version of my patches for the master database
key version number propagation (kadmind and kdc weren't using mkvno).
I don't have a record of what I sent so I'm sorry if these are
duplicated.
I'm not sure that my explanation of the 3rd memory leak is good so
please ask if it's not clear.
Thank you for looking into these, I appreciate your effort.
Thank you,
Andrew Gross
=========================================================================
inbuf.data is freed twice:
...
free(inbuf.data);
decode_kadmind_reply(msg_data, &rd_priv_resp);
free(inbuf.data);
...
=========================================================================
*** src-ref/kadmin/client/kadmin_add.c Thu Jun 23 23:20:48 1994
--- src/kadmin/client/kadmin_add.c Tue Sep 6 04:07:52 1994
***************
*** 263,269 ****
free(inbuf.data);
return(1);
}
- free(inbuf.data);
decode_kadmind_reply(msg_data, &rd_priv_resp);
--- 263,268 ----
=========================================================================
When adm_modify_kdb() is called from one of the change functions,
newprinc and principal are identical pointers. Thus setting
entry->mod_name to principal causes problems upon return as the
newprinc is freed and then the entry struct, but entry->mod_name was
already freed causing problems.
=========================================================================
diff -b -w -c -r src-ref/kadmin/server/adm_funcs.c src/kadmin/server/adm_funcs.c
*** src-ref/kadmin/server/adm_funcs.c Fri Jun 24 20:19:01 1994
--- src/kadmin/server/adm_funcs.c Mon Sep 5 02:36:37 1994
***************
*** 182,188 ****
#ifdef SANDIA
entry->attributes &= ~KRB5_KDB_REQUIRES_PWCHANGE;
#endif
! entry->mod_name = (krb5_principal) principal;
}
if (key && key->length) {
--- 182,189 ----
#ifdef SANDIA
entry->attributes &= ~KRB5_KDB_REQUIRES_PWCHANGE;
#endif
! /*DREW - this isn't used and would be freed twice */
! /* entry->mod_name = (krb5_principal) principal;*/
}
if (key && key->length) {
=========================================================================
This patch is just for the symptom. The problem lies between
krb5_rd_req_decoded() and process_client() (kadmind). In short,
when process_client() calls krb5_recvauth(), it passes in a keyproc
(cpw_keyproc) and a keyprocarg (&cpw_key). This is passed to
krb5_rd_req_decoded() which calls the keyproc to get a key, tkt_key.
krb5_rd_req_decoded() uses this key and then frees it. The problem
lies in that there are 2 cases: (1) keyprocarg->key is null and the
keyproc mallocs some space, in which case krb5_rd_req_decoded()
freeing it is ok, and (2) where keyprocarg->key is preset and tkt_key
gets just a pointer to the keyprocarg->key , now, when
krb5_rd_req_decoded() frees tkt_key, it has left a hole in the
keyprocarg structure. Promptly upon returning from krb5_recvauth(),
process_client() frees the keyprocarg->key and bombs.
=========================================================================
diff -b -w -c -r src-ref/kadmin/server/adm_process.c src/kadmin/server/adm_process.c
*** src-ref/kadmin/server/adm_process.c Fri Jun 24 20:19:10 1994
--- src/kadmin/server/adm_process.c Sat Sep 3 19:28:22 1994
***************
*** 277,286 ****
error_message(retval));
(void) sprintf(retbuf, "kadmind error during recvauth: %s\n",
error_message(retval));
! krb5_free_keyblock(cpw_key.key);
goto finish;
}
! krb5_free_keyblock(cpw_key.key);
/* Check if ticket was issued using password (and not tgt)
* within the last 5 minutes
--- 277,286 ----
error_message(retval));
(void) sprintf(retbuf, "kadmind error during recvauth: %s\n",
error_message(retval));
! /*DREW krb5_free_keyblock(cpw_key.key);*/
goto finish;
}
! /*DREW krb5_free_keyblock(cpw_key.key);*/
/* Check if ticket was issued using password (and not tgt)
* within the last 5 minutes
=========================================================================
The code goes as:
...
char retbuf[512];
...
final_msg.data = retbuf;
...
if (retval = krb5_mk_priv(&final_msg,
...
&msg_data)) {
syslog(LOG_ERR, "kadmind error Error Performing Final mk_priv");
free(final_msg.data);
goto finish;
}
free(final_msg.data);
...
The "final_msg.data = retbuf;" doesn't copy the data and a
non-malloc-ed pointer is freed.
=========================================================================
diff -b -w -c -r src-ref/kadmin/server/adm_process.c src/kadmin/server/adm_process.c
***************
*** 422,431 ****
0,
&msg_data)) {
syslog(LOG_ERR, "kadmind error Error Performing Final mk_priv");
! free(final_msg.data);
goto finish;
}
! free(final_msg.data);
/* Send Final Reply to Client */
if (retval = krb5_write_message(&client_server_info.client_socket,
--- 422,431 ----
0,
&msg_data)) {
syslog(LOG_ERR, "kadmind error Error Performing Final mk_priv");
! /*DREW free(final_msg.data);*/
goto finish;
}
! /*DREW free(final_msg.data);*/
/* Send Final Reply to Client */
if (retval = krb5_write_message(&client_server_info.client_socket,
=========================================================================
The problem here is that kdb5_mkdums doesn't initialize all of the
fields of the new principal. In particular, kdc5_hammer will fail
complaining that all of the passwords have expired.
=========================================================================
diff -b -w -c -r src-ref/tests/create/kdb5_mkdums.c src/tests/create/kdb5_mkdums.c
*** src-ref/tests/create/kdb5_mkdums.c Tue Aug 31 20:20:41 1993
--- src/tests/create/kdb5_mkdums.c Tue Sep 6 00:29:55 1994
***************
*** 291,296 ****
--- 291,297 ----
newentry.max_renewable_life = mblock.max_rlife;
newentry.mkvno = mblock.mkvno;
newentry.expiration = mblock.expiration;
+ newentry.pw_expiration = mblock.expiration;
newentry.mod_name = master_princ;
if (retval = krb5_timeofday(&newentry.mod_date)) {
com_err(progname, retval, "while fetching date");
***************
*** 302,307 ****
--- 303,312 ----
newentry.salt_type = KRB5_KDB_SALTTYPE_NORMAL;
newentry.salt_length = 0;
newentry.salt = 0;
+ newentry.alt_key.length = 0;
+ newentry.alt_key.contents = 0;
+ newentry.alt_salt_length = 0;
+ newentry.alt_salt = 0;
retval = krb5_db_put_principal(&newentry, &one);
if (retval) {
=========================================================================
This is a minor problem with appl/bsd/configure on a NeXT. In the
POSIX environment the "union wait" struct is defined but the macros
(e.g., WIFSTOPPED) are defined for an int. The olny way to catch this
is to use one in the test as follows.
=========================================================================
diff -b -w -c -r src-ref/appl/bsd/configure src/appl/bsd/configure
*** src-ref/appl/bsd/configure Wed Aug 10 09:58:51 1994
--- src/appl/bsd/configure Tue Sep 6 00:23:12 1994
***************
*** 1084,1095 ****
rm -f conftest*
test -n "$silent" || echo "checking for union wait"
cat > conftest.${ac_ext} <<EOF
#include "confdefs.h"
#include <sys/wait.h>
int main() { return 0; }
! int t() { union wait i;; return 0; }
EOF
if eval $ac_compile; then
:
--- 1084,1099 ----
rm -f conftest*
+ # *** Need WIFSTOPPED as a test since NeXT defines the union wait
+ # *** even in POSIX code, thus the old test succeeded and yet
+ # *** krlogin.c failed to compile. DREW
+ #
test -n "$silent" || echo "checking for union wait"
cat > conftest.${ac_ext} <<EOF
#include "confdefs.h"
#include <sys/wait.h>
int main() { return 0; }
! int t() { union wait i;; WIFSTOPPED(i); return 0; }
EOF
if eval $ac_compile; then
:
=========================================================================
This is a related bug on a NeXT. the #if does not take into account
compiling with POSIX on in which wait() takes an int . The symbol
_POSIX_SOURCE is defined in the POSIX environment, but I don't know
how portable this is.
=========================================================================
diff -b -w -c -r src-ref/slave/kpropd.c src/slave/kpropd.c
*** src-ref/slave/kpropd.c Thu Aug 4 13:42:52 1994
--- src/slave/kpropd.c Tue Sep 6 00:12:54 1994
***************
*** 761,767 ****
/* <sys/param.h> has been included, so BSD will be defined on
BSD systems */
! #if BSD > 0 && BSD <= 43
#ifndef WEXITSTATUS
#define WEXITSTATUS(w) (w).w_retcode
#endif
--- 761,767 ----
/* <sys/param.h> has been included, so BSD will be defined on
BSD systems */
! #if BSD > 0 && BSD <= 43 && !defined(_POSIX_SOURCE)
#ifndef WEXITSTATUS
#define WEXITSTATUS(w) (w).w_retcode
#endif
diff -b -w -c -r src-ref/kadmin/server/adm_network.c src/kadmin/server/adm_network.c
*** src-ref/kadmin/server/adm_network.c Thu Jun 2 09:23:46 1994
--- src/kadmin/server/adm_network.c Tue Sep 6 00:13:45 1994
***************
*** 86,92 ****
* <sys/param.h> has been included, so BSD will be defined on
* BSD systems
*/
! #if BSD > 0 && BSD <= 43
#ifndef WEXITSTATUS
#define WEXITSTATUS(w) (w).w_retcode
#define WTERMSIG(w) (w).w_termsig
--- 86,92 ----
* <sys/param.h> has been included, so BSD will be defined on
* BSD systems
*/
! #if BSD > 0 && BSD <= 43 && !defined(_POSIX_SOURCE)
#ifndef WEXITSTATUS
#define WEXITSTATUS(w) (w).w_retcode
#define WTERMSIG(w) (w).w_termsig
=========================================================================
Yet another NeXT bug (guess what I working on). sys/ioctl.h
needs to be included on a NeXT as that is where FIONBIO and a few
other TIOC*'s are defined.
=========================================================================
diff -b -w -c -r src-ref/appl/bsd/krlogind.c src/appl/bsd/krlogind.c
*** src-ref/appl/bsd/krlogind.c Sun Aug 7 17:39:59 1994
--- src/appl/bsd/krlogind.c Sat Sep 3 01:25:58 1994
***************
*** 177,182 ****
--- 180,189 ----
#ifdef HAVE_SYS_FILIO_H
/* get FIONBIO from sys/filio.h, so what if it is a compatibility feature */
#include <sys/filio.h>
+ #endif
+
+ #ifdef NeXT
+ #include <sys/ioctl.h>
#endif
#ifndef SETPGRP_TWOARG
=========================================================================
I'm not sure if I sent in the final version of my patches for the
mkvno problem (where the KDC and kadmind didn't propagate the mkvno
into new principals), so here they are.
=========================================================================
diff -b -w -c -r src-ref/kadmin/server/adm_extern.c src/kadmin/server/adm_extern.c
*** src-ref/kadmin/server/adm_extern.c Tue Aug 31 20:02:56 1993
--- src/kadmin/server/adm_extern.c Sun Sep 4 23:49:57 1994
***************
*** 51,56 ****
--- 51,57 ----
krb5_keyblock tgs_key;
krb5_kvno tgs_kvno;
+ krb5_kvno tgs_mkvno;
krb5_data inbuf;
krb5_data msg_data;
diff -b -w -c -r src-ref/kadmin/server/adm_extern.h src/kadmin/server/adm_extern.h
*** src-ref/kadmin/server/adm_extern.h Fri Dec 24 14:24:19 1993
--- src/kadmin/server/adm_extern.h Sun Sep 4 23:54:31 1994
***************
*** 59,64 ****
--- 59,65 ----
extern krb5_keyblock tgs_key;
extern krb5_kvno tgs_kvno;
+ extern krb5_kvno tgs_mkvno;
extern krb5_principal tgs_server;
extern global_client_server_info client_server_info;
diff -b -w -c -r src-ref/kadmin/server/adm_funcs.c src/kadmin/server/adm_funcs.c
*** src-ref/kadmin/server/adm_funcs.c Fri Jun 24 20:19:01 1994
--- src/kadmin/server/adm_funcs.c Mon Sep 5 02:36:37 1994
***************
*** 174,180 ****
entry->kvno = KDB5_VERSION_NUM;
entry->max_life = KDB5_MAX_TKT_LIFE;
entry->max_renewable_life = KDB5_MAX_REN_LIFE;
! entry->mkvno = mblock.mkvno;
entry->expiration = KDB5_EXP_DATE;
entry->mod_name = master_princ;
} else { /* Modify existing entry */
--- 174,180 ----
entry->kvno = KDB5_VERSION_NUM;
entry->max_life = KDB5_MAX_TKT_LIFE;
entry->max_renewable_life = KDB5_MAX_REN_LIFE;
! entry->mkvno = tgs_mkvno;
entry->expiration = KDB5_EXP_DATE;
entry->mod_name = master_princ;
} else { /* Modify existing entry */
diff -b -w -c -r src-ref/kadmin/server/adm_server.c src/kadmin/server/adm_server.c
*** src-ref/kadmin/server/adm_server.c Tue Jun 28 22:33:00 1994
--- src/kadmin/server/adm_server.c Sun Sep 4 23:49:31 1994
***************
*** 343,348 ****
--- 343,349 ----
}
tgs_kvno = server_entry.kvno;
+ tgs_mkvno = server_entry.mkvno;
krb5_db_free_principal(&server_entry, number_of_entries);
return(0);
}
diff -b -w -c -r src-ref/kdc/kerberos_v4.c src/kdc/kerberos_v4.c
*** src-ref/kdc/kerberos_v4.c Thu Aug 4 13:43:58 1994
--- src/kdc/kerberos_v4.c Sun Sep 4 23:24:46 1994
***************
*** 70,75 ****
--- 70,77 ----
/* take this out when we don't need it anymore */
int krbONE = 1;
+ extern u_char master_key_version;
+
#ifdef notdef
static struct sockaddr_in sin = {AF_INET};
#endif
***************
*** 90,98 ****
/*
static C_Block user_key;
static C_Block service_key;
- */
static u_char master_key_version;
- /*
static char k_instance[INST_SZ];
*/
static char log_text[128];
--- 92,98 ----
diff -b -w -c -r src-ref/kdc/main.c src/kdc/main.c
*** src-ref/kdc/main.c Tue Jun 28 22:33:35 1994
--- src/kdc/main.c Sat Sep 3 22:51:19 1994
***************
*** 56,61 ****
--- 56,65 ----
#include "extern.h"
#include "kdc5_err.h"
+ #ifdef KRB4
+ u_char master_key_version;
+ #endif /*KRB4*/
+
static void
kdc_com_err_proc(whoami, code, format, pvar)
const char *whoami;
***************
*** 356,361 ****
--- 360,368 ----
return retval;
}
tgs_kvno = server.kvno;
+ #ifdef KRB4
+ master_key_version = server.mkvno;
+ #endif /*KRB4*/
krb5_db_free_principal(&server, nprincs);
return 0;
}
