From 51e9da4a66e6730653545bd4deee5fe9b4236e3e Mon Sep 17 00:00:00 2001
From: Nicolas Williams <nico103@gmail.com>
Date: Wed, 13 Jul 2011 01:49:44 -0500
Subject: [PATCH] Fixed (preemptively) a double free and added password history
 based on key history.

---
 lib/hdb/keys.c       | 27 +++++++++++++++++++++------
 lib/kadm5/chpass_s.c | 17 +++++++++++------
 lib/kadm5/keys.c     | 32 +++++++++++++++++++++++++++++---
 3 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/lib/hdb/keys.c b/lib/hdb/keys.c
index 28327f057..f36ec8c41 100644
--- a/lib/hdb/keys.c
+++ b/lib/hdb/keys.c
@@ -210,6 +210,14 @@ parse_key_set(krb5_context context, const char *key,
 }
 
 
+/**
+ * This function adds an HDB entry's current keyset to the entry's key
+ * history.  The current keyset is left alone; the caller is responsible
+ * for freeing it.
+ *
+ * @param context   Context
+ * @param entry	    HDB entry
+ */
 krb5_error_code
 hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
 {
@@ -217,7 +225,8 @@ hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
     HDB_extension *ext;
     HDB_Ext_KeySet *hist_keys;
     hdb_keyset *tmp_keysets;
-    int add = 0;
+    size_t i;
+    size_t add = 0;
 
     ext = hdb_find_extension(entry, choice_HDB_extension_data_hist_keys);
     if (ext != NULL) {
@@ -244,15 +253,21 @@ hdb_add_current_keys_to_history(krb5_context context, hdb_entry *entry)
 	hist_keys->len = 1;
     }
 
-    hist_keys->val[0].keys.val = entry->keys.val;
-    hist_keys->val[0].keys.len = entry->keys.len;
+    hist_keys->val[0].keys.len = 0;
+    hist_keys->val[0].keys.val = calloc(entry->keys.len,
+					sizeof (*hist_keys->val[0].keys.val));
+    for (i = 0; i < entry->keys.len; i++, hist_keys->val[0].keys.len++) {
+	ret = copy_Key(&entry->keys.val[i], &hist_keys->val[0].keys.val[i]);
+	if (ret) {
+	    free_HDB_extension(ext);
+	    return ret;
+	}
+    }
     hist_keys->val[0].kvno = entry->kvno;
     (void) hdb_entry_get_pw_change_time(entry, &hist_keys->val[0].set_time);
 
-    entry->keys.val = NULL;
-    entry->keys.len = 0;
-
     if (add) {
+	/* XXX hdb_replace_extension() deep-copies ext; what a waste */
 	ret = hdb_replace_extension(context, entry, ext);
 	if (ret) {
 	    free_HDB_extension(ext);
diff --git a/lib/kadm5/chpass_s.c b/lib/kadm5/chpass_s.c
index 8ffdefa47..649c5b466 100644
--- a/lib/kadm5/chpass_s.c
+++ b/lib/kadm5/chpass_s.c
@@ -77,15 +77,20 @@ change(void *server_handle,
 
 	ret = _kadm5_set_keys(context, &ent.entry, password);
 	if(ret) {
-	    _kadm5_free_keys (context->context, num_keys, keys);
+	    _kadm5_free_keys(context->context, num_keys, keys);
 	    goto out2;
 	}
+	_kadm5_free_keys(context->context, num_keys, keys);
 
-	if (cond)
-	    existsp = _kadm5_exists_keys (ent.entry.keys.val,
-					  ent.entry.keys.len,
-					  keys, num_keys);
-	_kadm5_free_keys (context->context, num_keys, keys);
+	if (cond) {
+	    HDB_extension *ext;
+
+	    ext = hdb_find_extension(&ent.entry, choice_HDB_extension_data_hist_keys);
+	    if (ext != NULL)
+		existsp = _kadm5_exists_keys_hist(ent.entry.keys.val,
+						  ent.entry.keys.len,
+						  &ext->data.u.hist_keys);
+	}
 
 	if (existsp) {
 	    ret = KADM5_PASS_REUSE;
diff --git a/lib/kadm5/keys.c b/lib/kadm5/keys.c
index d46b8db73..4da157231 100644
--- a/lib/kadm5/keys.c
+++ b/lib/kadm5/keys.c
@@ -63,16 +63,36 @@ _kadm5_init_keys (Key *keys, int len)
     }
 }
 
+/*
+ * return 1 if any key in `keys1, len1' exists in hist_keys
+ */
+int
+_kadm5_exists_keys_hist(Key *keys1, int len1, HDB_Ext_KeySet *hist_keys)
+{
+    size_t i;
+
+    for (i = 0; i < hist_keys->len; i++) {
+	if (_kadm5_exists_keys(keys1, len1,
+			       hist_keys->val[i].keys.val,
+			       hist_keys->val[i].keys.len))
+	    return 1;
+    }
+
+    return 0;
+}
+
+
 /*
  * return 1 if any key in `keys1, len1' exists in `keys2, len2'
  */
-
 int
 _kadm5_exists_keys(Key *keys1, int len1, Key *keys2, int len2)
 {
-    int i, j;
+    size_t i, j;
+    size_t checked_all_this_enctype;
 
     for (i = 0; i < len1; ++i) {
+	checked_all_this_enctype = 1;
 	for (j = 0; j < len2; j++) {
 	    if ((keys1[i].salt != NULL && keys2[j].salt == NULL)
 		|| (keys1[i].salt == NULL && keys2[j].salt != NULL))
@@ -87,8 +107,10 @@ _kadm5_exists_keys(Key *keys1, int len1, Key *keys2, int len2)
 			    keys1[i].salt->salt.length) != 0)
 		    continue;
 	    }
-	    if (keys1[i].key.keytype != keys2[j].key.keytype)
+	    if (keys1[i].key.keytype != keys2[j].key.keytype) {
+		checked_all_this_enctype = 0;
 		continue;
+	    }
 	    if (keys1[i].key.keyvalue.length != keys2[j].key.keyvalue.length)
 		continue;
 	    if (memcmp (keys1[i].key.keyvalue.data, keys2[j].key.keyvalue.data,
@@ -97,6 +119,10 @@ _kadm5_exists_keys(Key *keys1, int len1, Key *keys2, int len2)
 
 	    return 1;
 	}
+
+	/* Optimization */
+	if (checked_all_this_enctype)
+	    return 0;
     }
     return 0;
 }