fix MITKRB5-SA-2004-004

ticket: new

git-svn-id: svn://anonsvn.mit.edu/krb5/trunk@16961 dc483132-0cff-0310-8789-dd5450dbe970

1 files changed, 64 insertions, 35 deletions

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