From 4fb2a03c3943858d1e73fe9ef3e06a9da4bf7d6a Mon Sep 17 00:00:00 2001 From: Tom Yu Date: Mon, 20 Dec 2004 21:15:41 +0000 Subject: [PATCH] fix MITKRB5-SA-2004-004 ticket: new git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@16961 dc483132-0cff-0310-8789-dd5450dbe970 --- src/lib/kadm5/srv/ChangeLog | 10 + src/lib/kadm5/srv/svr_principal.c | 99 +++++---- src/tests/dejagnu/krb-standalone/ChangeLog | 7 + src/tests/dejagnu/krb-standalone/pwhist.exp | 215 ++++++++++++++++++++ 4 files changed, 296 insertions(+), 35 deletions(-) create mode 100644 src/tests/dejagnu/krb-standalone/pwhist.exp diff --git a/src/lib/kadm5/srv/ChangeLog b/src/lib/kadm5/srv/ChangeLog index dcadace07..eeba8685c 100644 --- a/src/lib/kadm5/srv/ChangeLog +++ b/src/lib/kadm5/srv/ChangeLog @@ -1,3 +1,13 @@ +2004-12-20 Tom Yu + + * svr_principal.c (add_to_history): Rewrite somewhat, using + temporary variables to make things somewhat more readable. Fix + buffer overflow case where the next pointer points into + unallocated space but resizing wasn't done, i.e., when someone + decreases the policy history count to the exact "right" number. + Fix some memory leaks. To avoid losing entries, shift some + entries forward after growing the array. + 2004-08-21 Tom Yu * libkadm5srv.exports: Update for previous renaming. diff --git a/src/lib/kadm5/srv/svr_principal.c b/src/lib/kadm5/srv/svr_principal.c index c567f8369..7dc2d8f6b 100644 --- a/src/lib/kadm5/srv/svr_principal.c +++ b/src/lib/kadm5/srv/svr_principal.c @@ -989,35 +989,46 @@ void free_history_entry(krb5_context context, osa_pw_hist_ent *hist) * array where the next element should be written, and must be [0, * adb->old_key_len). */ -#define KADM_MOD(x) (x + adb->old_key_next) % adb->old_key_len static kadm5_ret_t add_to_history(krb5_context context, osa_princ_ent_t adb, kadm5_policy_ent_t pol, osa_pw_hist_ent *pw) { osa_pw_hist_ent *histp; - int i; + uint32_t nhist; + unsigned int i, knext, nkeys; + nhist = pol->pw_history_num; /* A history of 1 means just check the current password */ - if (pol->pw_history_num == 1) + if (nhist <= 1) return 0; + nkeys = adb->old_key_len; + knext = adb->old_key_next; /* resize the adb->old_keys array if necessary */ - if (adb->old_key_len < pol->pw_history_num-1) { + if (nkeys + 1 < nhist) { if (adb->old_keys == NULL) { adb->old_keys = (osa_pw_hist_ent *) - malloc((adb->old_key_len + 1) * sizeof (osa_pw_hist_ent)); + malloc((nkeys + 1) * sizeof (osa_pw_hist_ent)); } else { adb->old_keys = (osa_pw_hist_ent *) realloc(adb->old_keys, - (adb->old_key_len + 1) * sizeof (osa_pw_hist_ent)); + (nkeys + 1) * sizeof (osa_pw_hist_ent)); } if (adb->old_keys == NULL) return(ENOMEM); - memset(&adb->old_keys[adb->old_key_len],0,sizeof(osa_pw_hist_ent)); - adb->old_key_len++; - } else if (adb->old_key_len > pol->pw_history_num-1) { + memset(&adb->old_keys[nkeys], 0, sizeof(osa_pw_hist_ent)); + nkeys = ++adb->old_key_len; + /* + * To avoid losing old keys, shift forward each entry after + * knext. + */ + for (i = nkeys - 1; i > knext; i--) { + adb->old_keys[i] = adb->old_keys[i - 1]; + } + memset(&adb->old_keys[knext], 0, sizeof(osa_pw_hist_ent)); + } else if (nkeys + 1 > nhist) { /* * The policy must have changed! Shrink the array. * Can't simply realloc() down, since it might be wrapped. @@ -1027,46 +1038,64 @@ static kadm5_ret_t add_to_history(krb5_context context, * where N = pw_history_num - 1 is the length of the * shortened list. Matt Crawford, FNAL */ + /* + * M = adb->old_key_len, N = pol->pw_history_num - 1 + * + * tmp[0] .. tmp[N-1] = old[(knext-N)%M] .. old[(knext-1)%M] + */ int j; - histp = (osa_pw_hist_ent *) - malloc((pol->pw_history_num - 1) * sizeof (osa_pw_hist_ent)); - if (histp) { - for (i = 0; i < pol->pw_history_num - 1; i++) { - /* We need the number we use the modulus operator on to be - positive, so after subtracting pol->pw_history_num-1, we - add back adb->old_key_len. */ - j = KADM_MOD(i - (pol->pw_history_num - 1) + adb->old_key_len); - histp[i] = adb->old_keys[j]; + osa_pw_hist_t tmp; + + tmp = (osa_pw_hist_ent *) + malloc((nhist - 1) * sizeof (osa_pw_hist_ent)); + if (tmp == NULL) + return ENOMEM; + for (i = 0; i < nhist - 1; i++) { + /* + * Add nkeys once before taking remainder to avoid + * negative values. + */ + j = (i + nkeys + knext - (nhist - 1)) % nkeys; + tmp[i] = adb->old_keys[j]; + } + /* Now free the ones we don't keep (the oldest ones) */ + for (i = 0; i < nkeys - (nhist - 1); i++) { + j = (i + nkeys + knext) % nkeys; + histp = &adb->old_keys[j]; + for (j = 0; j < histp->n_key_data; j++) { + krb5_free_key_data_contents(context, &histp->key_data[j]); } - /* Now free the ones we don't keep (the oldest ones) */ - for (i = 0; i < adb->old_key_len - (pol->pw_history_num - 1); i++) - for (j = 0; j < adb->old_keys[KADM_MOD(i)].n_key_data; j++) - krb5_free_key_data_contents(context, - &adb->old_keys[KADM_MOD(i)].key_data[j]); - free((void *)adb->old_keys); - adb->old_keys = histp; - adb->old_key_len = pol->pw_history_num - 1; - adb->old_key_next = 0; - } else { - return(ENOMEM); + free(histp->key_data); } + free((void *)adb->old_keys); + adb->old_keys = tmp; + nkeys = adb->old_key_len = nhist - 1; + knext = adb->old_key_next = 0; } + /* + * If nhist decreased since the last password change, and nkeys+1 + * is less than the previous nhist, it is possible for knext to + * index into unallocated space. This condition would not be + * caught by the resizing code above. + */ + if (knext + 1 > nkeys) + knext = adb->old_key_next = 0; /* free the old pw history entry if it contains data */ - histp = &adb->old_keys[adb->old_key_next]; + histp = &adb->old_keys[knext]; for (i = 0; i < histp->n_key_data; i++) krb5_free_key_data_contents(context, &histp->key_data[i]); - + free(histp->key_data); + /* store the new entry */ - adb->old_keys[adb->old_key_next] = *pw; + adb->old_keys[knext] = *pw; /* update the next pointer */ - if (++adb->old_key_next == pol->pw_history_num-1) - adb->old_key_next = 0; + if (++adb->old_key_next == nhist - 1) + adb->old_key_next = 0; return(0); } -#undef KADM_MOD #ifdef USE_PASSWORD_SERVER diff --git a/src/tests/dejagnu/krb-standalone/ChangeLog b/src/tests/dejagnu/krb-standalone/ChangeLog index bba583b62..fe26d2abf 100644 --- a/src/tests/dejagnu/krb-standalone/ChangeLog +++ b/src/tests/dejagnu/krb-standalone/ChangeLog @@ -1,3 +1,10 @@ +2004-12-20 Tom Yu + + * pwhist.exp: New file. Perform some sanity checking on password + history mechanism, including erroneous loss of history when + growing the history array. Also tries to trigger some known + buffer overflows and memory leaks. + 2004-03-14 Ken Raeburn * gssapi.exp (run_client, doit): Use portbase to compute all port diff --git a/src/tests/dejagnu/krb-standalone/pwhist.exp b/src/tests/dejagnu/krb-standalone/pwhist.exp new file mode 100644 index 000000000..f9938e091 --- /dev/null +++ b/src/tests/dejagnu/krb-standalone/pwhist.exp @@ -0,0 +1,215 @@ +# password history tests + +# one *non-interactive* kadmin.local request +proc onerq { rq pname str {flags ""} } { + global REALMNAME + global KADMIN_LOCAL + + spawn $KADMIN_LOCAL -r $REALMNAME -q "$rq $flags $pname" + expect_after { + timeout { + verbose "kadmin.local $rq $flags $pname timed out" + catch expect_after + kill [exp_pid] + close + expect eof + wait + return 0 + } eof { + verbose "kadmin.local $rq $flags $pname got EOF" + catch expect_after + wait + return 0 + } + } + expect $str + expect_after + expect eof + wait + return 1 +} + +proc addprinc { pname pw } { + global REALMNAME + + return [onerq addprinc $pname \ + "Principal \"$pname@$REALMNAME\" created." "-pw $pw"] +} + +proc delprinc { pname } { + global REALMNAME + return [onerq delprinc $pname \ + "Principal \"$pname@$REALMNAME\" deleted." "-force"] +} + +proc cpw { pname pw } { + global REALMNAME + + return [onerq cpw $pname \ + "Password for \"$pname@$REALMNAME\" changed." "-pw $pw"] +} + +proc modprinc { pname flags } { + global REALMNAME + + return [onerq modprinc $pname \ + "Principal \"$pname@$REALMNAME\" modified." $flags] +} + +proc addpol { pname } { + if ![onerq addpol $pname ""] { + return 0 + } + return [onerq getpol $pname "Policy: $pname"] +} + +proc delpol { pname } { + onerq delpol $pname "" -force + return [onerq getpol $pname \ + "Policy does not exist while retrieving policy \"$pname\"."] +} + +proc modpol { pname flags } { + return [onerq modpol $pname "" $flags] +} + +# Mandatory command must return true. +# Issues a break in its parent on failure. +proc mustrun { cmd } { + if ![eval $cmd] { + perror "mandatory command failed: $cmd" + uplevel break + } +} + +# Fail test if command fails. +# Issues a break in its parent on failure. +proc chkpass { cmd } { + upvar test test + if ![eval $cmd] { + verbose "unexpected failure: $cmd" + fail $test + uplevel break + } +} + +# Fail test if command succeeds. +# Issues a break in its parent on failure. +proc chkfail { cmd } { + upvar test test + if [eval $cmd] { + verbose "unexpected success: $cmd" + fail $test + uplevel break + } +} + +# wrapper to run command (actually usually sequence of commands) +# +# If any part of CMD throws an exception, set failall, otherwise pass. +# If failall is already true, report unresolved. +proc wraptest { test cmd } { + upvar failall failall + if $failall { + unresolved $test + return + } + if [catch $cmd] { + set failall 1 + } else { + pass $test + } +} + +# Set up the kerberos database. +if {![get_hostname] \ + || ![setup_kerberos_files] \ + || ![setup_kerberos_env] \ + || ![setup_kerberos_db 0]} { + return +} + +set failall 0 +wraptest "nkeys=1, nhist=3" { + mustrun { addpol crashpol } + mustrun { modpol crashpol "-history 3"} + mustrun { addprinc crash 1111 } + mustrun { modprinc crash "-policy crashpol" } + chkpass { cpw crash 2222 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } +} +verbose {old_keys [ 1111 ->[] ]} + +# The following will result in reading/writing past array bounds if +# add_to_history() is not patched. +# +# NOTE: A pass from this test does not mean the bug isn't present; +# check with Purify, valgrind, etc. +wraptest "array bounds ok on nkeys=1, nhist 3->2" { + mustrun { modpol crashpol "-history 2" } + chkpass { cpw crash 3333 } +} +verbose {old_keys [ ->2222 ]} + +wraptest "verify nhist=2" { + mustrun { delprinc crash } + mustrun { addprinc crash 1111 } + mustrun { modprinc crash "-policy crashpol" } + chkpass { cpw crash 2222 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } +} +verbose {old_keys [ ->1111 ]} + +# The following will fail if growing the history array causes an extra +# key to be lost due to failure to shift entries. +wraptest "grow nhist 2->3" { + mustrun { modpol crashpol "-history 3" } + chkpass { cpw crash 3333 } + chkfail { cpw crash 3333 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } +} +verbose {old_keys [ 2222 ->1111 ]} + +wraptest "grow nhist 3->4" { + mustrun { modpol crashpol "-history 4" } + chkfail { cpw crash 3333 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } + chkpass { cpw crash 4444 } + chkfail { cpw crash 3333 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } +} +verbose {old_keys [ 2222 3333 ->1111 ]} +wraptest "shrink nhist 4->3" { + mustrun { modpol crashpol "-history 3" } + chkfail { cpw crash 4444 } + chkfail { cpw crash 3333 } + chkfail { cpw crash 2222 } + chkfail { cpw crash 1111 } + chkpass { cpw crash 5555 } +} +verbose {old_keys [ 4444 ->3333 ]} +wraptest "verify nhist=3" { + chkfail { cpw crash 5555 } + chkfail { cpw crash 4444 } + chkfail { cpw crash 3333 } + chkpass { cpw crash 2222 } +} +verbose {old_keys [ ->4444 5555 ]} +wraptest "shrink nhist 3->2" { + mustrun { modpol crashpol "-history 2" } + chkfail { cpw crash 2222 } + chkfail { cpw crash 5555 } + chkfail { cpw crash 4444 } + chkpass { cpw crash 3333 } +} +verbose {old_keys [ ->2222 ]} + +delprinc crash +delpol crashpol + +stop_kerberos_daemons -- 2.26.2