From 82ea5eb850ece3825558b53e1e9aec0c0a26c809 Mon Sep 17 00:00:00 2001 From: Tom Yu Date: Mon, 20 Dec 2004 21:16:20 +0000 Subject: [PATCH] pullup from trunk ticket: 2841 target_version: 1.4 version_fixed: 1.4 tags: pullup git-svn-id: svn://anonsvn.mit.edu/krb5/branches/krb5-1-4@16964 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 ++ 3 files changed, 81 insertions(+), 35 deletions(-) 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 -- 2.26.2