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

diff --git a/src/lib/kadm5/srv/ChangeLog b/src/lib/kadm5/srv/ChangeLog
index dcadace0799bf06a6c04331886619f606653bbec..eeba8685c97686749cb033660b404e27cb6dcc0f 100644 (file)
--- a/src/lib/kadm5/srv/ChangeLog
+++ b/src/lib/kadm5/srv/ChangeLog
@@ -1,3 +1,13 @@
+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.

diff --git a/src/lib/kadm5/srv/svr_principal.c b/src/lib/kadm5/srv/svr_principal.c
index c567f83690ba1323798c20bc0666f0e0f03889d2..7dc2d8f6bc3d507d62062b0772a2e13240991639 100644 (file)
--- 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 bba583b625563a40dedddef45414c4b1abde4899..fe26d2abffa9afd88ac3666f98b58e9a7a460e58 100644 (file)
--- a/src/tests/dejagnu/krb-standalone/ChangeLog
+++ b/src/tests/dejagnu/krb-standalone/ChangeLog
@@ -1,3 +1,10 @@
+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