Fixed a bug by s/u16/SEEK_CURR/; the bug prevented this mitdb backend from parsing MIT KDB entries with multiple kvnos in non-increasing order.

Fixed a double-free bug that was triggered by MIT KDB entries with
multiple kvnos in non-increasing order.

Added lots of comments regarding the MIT KDB entry format.

Signed-off-by: Love Hornquist Astrand <lha@h5l.org>

lib/hdb/hdb-mitdb.c

@@ -215,7 +215,21 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
     
     krb5_storage_set_byteorder(sp, KRB5_STORAGE_BYTEORDER_LE);
 
-    /* 16: baselength */
+    /*
+     * 16: baselength
+     *
+     * The story here is that these 16 bits have to be a constant:
+     * KDB_V1_BASE_LENGTH.  Once upon a time a different value here
+     * would have been used to indicate the presence of "extra data"
+     * between the "base" contents and the {principal name, TL data,
+     * keys} that follow it.  Nothing supports such "extra data"
+     * nowadays, so neither do we here.
+     *
+     * XXX But... surely we ought to log about this extra data, or skip
+     * it, or something, in case anyone has MIT KDBs with ancient
+     * entries in them...  Logging would allow the admin to know which
+     * entries to dump with MIT krb5's kdb5_util.
+     */
     CHECK(ret = krb5_ret_uint16(sp, &u16));
     if (u16 != KDB_V1_BASE_LENGTH) { ret = EINVAL; goto out; }
     /* 32: attributes */
@@ -273,6 +287,10 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
     CHECK(ret = krb5_ret_uint16(sp, &u16));
     /* length: principal */
     {
+	/*
+	 * Note that the principal name includes the NUL in the entry,
+	 * but we don't want to take chances, so we add an extra NUL.
+	 */
 	p = malloc(u16 + 1);
 	krb5_storage_read(sp, p, u16);
 	p[u16] = '\0';
@@ -284,17 +302,25 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
            16: tl data length
            length: length */
     for (i = 0; i < num_tl; i++) {
+	/* 16: TL data type */
 	CHECK(ret = krb5_ret_uint16(sp, &u16));
+	/* 16: TL data length */
 	CHECK(ret = krb5_ret_uint16(sp, &u16));
 	krb5_storage_seek(sp, u16, SEEK_CUR);
     }
-    /* for num key data times
+    /*
-       16: version (num keyblocks)
+     * for num key data times
-       16: kvno
+     * 16: "version"
-       for version times:
+     * 16: kvno
-           16: type
+     * for version times:
-           16: length
+     *     16: type
-           length: keydata */
+     *     16: length
+     *     length: keydata
+     *
+     * "version" here is really 1 or 2, the first meaning there's only
+     * keys for this kvno, the second meaning there's keys and salt[s?].
+     * That's right... hold that gag reflex, you can do it.
+     */
     for (i = 0; i < num_keys; i++) {
 	int keep = 0;
 	uint16_t version;
@@ -307,12 +333,19 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
 	if (((entry->kvno < u16) && kvno == 0) || kvno == u16) {
 	    keep = 1;
 	    entry->kvno = u16;
-	    for (j = 0; j < entry->keys.len; j++) {
+	    /*
+	     * Found a higher kvno than earlier, so free the old highest
+	     * kvno keys.
+	     *
+	     * XXX Of course, we actually want to extract the old kvnos
+	     * as well, for some of the kadm5 APIs.  We shouldn't free
+	     * these keys, but keep them elsewhere.
+	     */
+	    for (j = 0; j < entry->keys.len; j++)
 		free_Key(&entry->keys.val[j]);
-		free(entry->keys.val);
+	    free(entry->keys.val);
-		entry->keys.len = 0;
+	    entry->keys.len = 0;
-		entry->keys.val = NULL;
+	    entry->keys.val = NULL;
-	    }
 	} else if (entry->kvno == u16)
 	    keep = 1;
 
@@ -343,17 +376,29 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
 		uint16_t type;
 		CHECK(ret = krb5_ret_uint16(sp, &type));
 		CHECK(ret = krb5_ret_uint16(sp, &u16));
-		if (j == 0) { /* key */
+		if (j == 0) {
+		    /* This "version" means we have a key */
 		    k->key.keytype = type;
 		    if (u16 < 2) {
 			ret = EINVAL;
 			goto out;
 		    }
+		    /*
+		     * MIT stores keys encrypted keys as {16-bit length
+		     * of plaintext key, {encrypted key}}.  The reason
+		     * for this is that the Kerberos cryptosystem is not
+		     * length-preserving.  Heimdal's approach is to
+		     * truncate the plaintext to the expected length of
+		     * the key given its enctype, so we ignore this
+		     * 16-bit length-of-plaintext-key field.
+		     */
 		    krb5_storage_seek(sp, 2, SEEK_CUR); /* skip real length */
-		    k->key.keyvalue.length = u16 - 2;
+		    k->key.keyvalue.length = u16 - 2;   /* adjust cipher len */
 		    k->key.keyvalue.data = malloc(k->key.keyvalue.length);
-		    krb5_storage_read(sp, k->key.keyvalue.data, k->key.keyvalue.length);
+		    krb5_storage_read(sp, k->key.keyvalue.data,
-		} else if (j == 1) { /* salt */
+				      k->key.keyvalue.length);
+		} else if (j == 1) {
+		    /* This "version" means we have a salt */
 		    k->salt = calloc(1, sizeof(*k->salt));
 		    if (k->salt == NULL) {
 			ret = ENOMEM;
@@ -371,15 +416,27 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
 		    }			
 		    fix_salt(context, entry, entry->keys.len - 1);
 		} else {
+		    /*
+		     * Whatever this "version" might be, we skip it
+		     *
+		     * XXX A krb5.conf parameter requesting that we log
+		     * about strangeness like this, or return an error
+		     * from here, might be nice.
+		     */
 		    krb5_storage_seek(sp, u16, SEEK_CUR);
 		}
 	    }
 	} else {
-	    /* skip */
+	    /*
+	     * XXX For now we skip older kvnos, but we should extract
+	     * them...
+	     */
 	    for (j = 0; j < version; j++) {
+		/* enctype */
 		CHECK(ret = krb5_ret_uint16(sp, &u16));
+		/* encrypted key (or plaintext salt) */
 		CHECK(ret = krb5_ret_uint16(sp, &u16));
-		krb5_storage_seek(sp, u16, u16);
+		krb5_storage_seek(sp, u16, SEEK_CUR);
 	    }
 	}
     }
@@ -391,6 +448,9 @@ mdb_value2entry(krb5_context context, krb5_data *data, krb5_kvno kvno, hdb_entry
 
     return 0;
  out:
+    if (ret == HEIM_ERR_EOF)
+	/* Better error code than "end of file" */
+	ret = HEIM_ERR_BAD_HDBENT_ENCODING;
     if (p)
 	free(p);
     free_hdb_entry(entry);

lib/krb5/heim_err.et

@@ -19,6 +19,7 @@ error_code BAD_MKEY,		"Failed to get the master key"
 error_code SERVICE_NOMATCH,	"Unacceptable service used"
 error_code NOT_SEEKABLE,	"File descriptor not seekable"
 error_code TOO_BIG,		"Offset too large"
+error_code BAD_HDBENT_ENCODING,	"Invalid HDB entry encoding"
 
 index 64
 prefix HEIM_PKINIT