+2004-12-20 Tom Yu <tlyu@mit.edu>
+
+ * 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 <tlyu@mit.edu>
* libkadm5srv.exports: Update for previous renaming.
* 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.
* 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
+2004-12-20 Tom Yu <tlyu@mit.edu>
+
+ * 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 <raeburn@mit.edu>
* gssapi.exp (run_client, doit): Use portbase to compute all port
--- /dev/null
+# 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
projects / krb5.git / commitdiff