Only store latest keys in key history entry

If a password is changed with the -keepold option, then changed again,
the history entry contains both the latest password and the one that
was kept.  Fix create_history_entry to only store the latest kvno in
the history entry.  Also add a test to ensure that the bug is fixed.

ticket: 8354

diff --git a/src/lib/kadm5/srv/svr_principal.c b/src/lib/kadm5/srv/svr_principal.c
index 1d4365c836fb43c3979f0292460e077f480335fa..9b1efbbcceccfcc4bd122ce49c3ac6b2f0efd857 100644 (file)
--- a/src/lib/kadm5/srv/svr_principal.c
+++ b/src/lib/kadm5/srv/svr_principal.c
@@ -1084,6 +1084,16 @@ check_pw_reuse(krb5_context context,
     return(0);
 }
 
+static void
+free_history_entry(krb5_context context, osa_pw_hist_ent *hist)
+{
+    int i;
+
+    for (i = 0; i < hist->n_key_data; i++)
+        krb5_free_key_data_contents(context, &hist->key_data[i]);
+    free(hist->key_data);
+}
+
 /*
  * Function: create_history_entry
  *
@@ -1097,7 +1107,7 @@ check_pw_reuse(krb5_context context,
  *      hist_key        (r) history keyblock to encrypt key data with
  *      n_key_data      (r) number of elements in key_data
  *      key_data        (r) keys to add to the history entry
- *      hist            (w) history entry to fill in
+ *      hist_out        (w) history entry to fill in
  *
  * Effects:
  *
@@ -1109,45 +1119,62 @@ check_pw_reuse(krb5_context context,
 static
 int create_history_entry(krb5_context context,
                          krb5_keyblock *hist_key, int n_key_data,
-                         krb5_key_data *key_data, osa_pw_hist_ent *hist)
+                         krb5_key_data *key_data, osa_pw_hist_ent *hist_out)
 {
-    krb5_error_code ret;
+    int i;
+    krb5_error_code ret = 0;
     krb5_keyblock key;
     krb5_keysalt salt;
-    int i;
+    krb5_ui_2 kvno;
+    osa_pw_hist_ent hist;
 
-    hist->key_data = k5calloc(n_key_data, sizeof(krb5_key_data), &ret);
-    if (hist->key_data == NULL)
-        return ret;
+    hist_out->key_data = NULL;
+    hist_out->n_key_data = 0;
+
+    if (n_key_data < 0)
+        return EINVAL;
+
+    memset(&key, 0, sizeof(key));
+    memset(&hist, 0, sizeof(hist));
+
+    if (n_key_data == 0)
+        goto cleanup;
+
+    hist.key_data = k5calloc(n_key_data, sizeof(krb5_key_data), &ret);
+    if (hist.key_data == NULL)
+        goto cleanup;
+
+    /* We only want to store the most recent kvno, and key_data should already
+     * be sorted in descending order by kvno. */
+    kvno = key_data[0].key_data_kvno;
 
     for (i = 0; i < n_key_data; i++) {
-        ret = krb5_dbe_decrypt_key_data(context, NULL, &key_data[i], &key,
+        if (key_data[i].key_data_kvno < kvno)
+            break;
+        ret = krb5_dbe_decrypt_key_data(context, NULL,
+                                        &key_data[i], &key,
                                         &salt);
         if (ret)
-            return ret;
+            goto cleanup;
 
         ret = krb5_dbe_encrypt_key_data(context, hist_key, &key, &salt,
                                         key_data[i].key_data_kvno,
-                                        &hist->key_data[i]);
+                                        &hist.key_data[hist.n_key_data]);
         if (ret)
-            return ret;
-
+            goto cleanup;
+        hist.n_key_data++;
         krb5_free_keyblock_contents(context, &key);
         /* krb5_free_keysalt(context, &salt); */
     }
 
-    hist->n_key_data = n_key_data;
-    return 0;
-}
-
-static
-void free_history_entry(krb5_context context, osa_pw_hist_ent *hist)
-{
-    int i;
+    *hist_out = hist;
+    hist.n_key_data = 0;
+    hist.key_data = NULL;
 
-    for (i = 0; i < hist->n_key_data; i++)
-        krb5_free_key_data_contents(context, &hist->key_data[i]);
-    free(hist->key_data);
+cleanup:
+    krb5_free_keyblock_contents(context, &key);
+    free_history_entry(context, &hist);
+    return ret;
 }
 
 /*
@@ -1526,11 +1553,14 @@ kadm5_chpass_principal_3(void *server_handle,
                     goto done;
             }
 
-            ret = add_to_history(handle->context, hist_kvno, &adb, &pol,
-                                 &hist);
-            if (ret)
-                goto done;
-            hist_added = 1;
+            /* Don't save empty history. */
+            if (hist.n_key_data > 0) {
+                ret = add_to_history(handle->context, hist_kvno, &adb, &pol,
+                                     &hist);
+                if (ret)
+                    goto done;
+                hist_added = 1;
+            }
         }
 
         if (pol.pw_max_life)

diff --git a/src/tests/t_kdb.py b/src/tests/t_kdb.py
index 2d2d675de3cc6b71317aac49b020c25190e28fb2..27d0e3fe2ece5e13afbc3f2e2c76fb9bd90757c5 100755 (executable)
--- a/src/tests/t_kdb.py
+++ b/src/tests/t_kdb.py
@@ -367,6 +367,13 @@ out = realm.run([kadminl, 'getprinc', 'keylessprinc'])
 if 'Number of keys: 0' not in out:
     fail('After purgekeys -all, keys remain')
 
+# Test for 8354 (old password history entries when -keepold is used)
+realm.run([kadminl, 'addpol', '-history', '2', 'keepoldpasspol'])
+realm.run([kadminl, 'addprinc', '-policy', 'keepoldpasspol', '-pw', 'aaaa',
+           'keepoldpassprinc'])
+for p in ('bbbb', 'cccc', 'aaaa'):
+    realm.run([kadminl, 'cpw', '-keepold', '-pw', p, 'keepoldpassprinc'])
+
 realm.stop()
 
 # Briefly test dump and load.