Changing the master key

[Greg Hudson]

On Sat, 2011-05-21 at 10:28 -0400, John Devitofranceschi wrote:
> We've run into a situation with MIT Kerberos 1.8.2 where the master
> key has been changed and yet the slave kdc's are still reporting that
> the original master key is being used on new principals.
>
> Slave kdc updates are happening via iprop.

I was able to reproduce and debug this problem.  The per-principal
master key version is stored in a tagged-data entry in the principal
entry.  There is a long-standing bug in the way iprop receives
tagged-data updates, causing it to ignore all but the first entry.  The
master key version happens to appear second in the list when a principal
is added, so it gets lost.

I would guess that under 1.8.x this bug causes new principals to be
inaccessible from the slave KDC, which is pretty bad.  In 1.9.x the bug
would be fairly harmless, as the KDC has stopped caring about the
principal's master key version (it just tries all master keys).  Some
admin programs still care about the per-principal master key version,
but you wouldn't typically run those on slaves.

I've tested and committed a fix to the trunk code.  There has been a
little bit of code drift between 1.8 and trunk, so I'm attaching a patch
against 1.8.x.  I haven't tested the 1.8 change except to make sure it
compiles, but I'm confident it will behave identically to the trunk
change.
-------------- next part --------------
Index: src/lib/kdb/kdb_convert.c
===================================================================
--- src/lib/kdb/kdb_convert.c	(revision 24536)
+++ src/lib/kdb/kdb_convert.c	(working copy)
@@ -637,7 +637,7 @@
         krb5_principal dbprinc;
         char *dbprincstr = NULL;
 
-        krb5_tl_data *newtl = NULL;
+        krb5_tl_data newtl;
         krb5_error_code ret;
         unsigned int more;
         unsigned int prev_n_keys = 0;
@@ -782,41 +782,14 @@
                 break;
 
             case AT_TL_DATA: {
-                int t;
-
-                cnt = u.av_tldata.av_tldata_len;
-                newtl = calloc(cnt, sizeof (krb5_tl_data));
-                if (newtl == NULL)
-                    return (ENOMEM);
-
-                for (j = 0, t = 0; j < cnt; j++) {
-                    newtl[t].tl_data_type = (krb5_int16)u.av_tldata.av_tldata_val[j].tl_type;
-                    newtl[t].tl_data_length = (krb5_int16)u.av_tldata.av_tldata_val[j].tl_data.tl_data_len;
-                    newtl[t].tl_data_contents = malloc(newtl[t].tl_data_length * sizeof (krb5_octet));
-                    if (newtl[t].tl_data_contents == NULL)
-                        /* XXX Memory leak: newtl
-                           and previously
-                           allocated elements.  */
-                        return (ENOMEM);
-
-                    (void) memcpy(newtl[t].tl_data_contents, u.av_tldata.av_tldata_val[t].tl_data.tl_data_val, newtl[t].tl_data_length);
-                    newtl[t].tl_data_next = NULL;
-                    if (t > 0)
-                        newtl[t - 1].tl_data_next = &newtl[t];
-                    t++;
+                for (j = 0; j < (int)u.av_tldata.av_tldata_len; j++) {
+                    newtl.tl_data_type = (krb5_int16)u.av_tldata.av_tldata_val[j].tl_type;
+                    newtl.tl_data_length = (krb5_int16)u.av_tldata.av_tldata_val[j].tl_data.tl_data_len;
+                    newtl.tl_data_contents = (krb5_octet *)u.av_tldata.av_tldata_val[j].tl_data.tl_data_val;
+                    newtl.tl_data_next = NULL;
+                    if ((ret = krb5_dbe_update_tl_data(context, ent, &newtl)))
+                        return (ret);
                 }
-
-                if ((ret = krb5_dbe_update_tl_data(context, ent, newtl)))
-                    return (ret);
-                for (j = 0; j < t; j++)
-                    if (newtl[j].tl_data_contents) {
-                        free(newtl[j].tl_data_contents);
-                        newtl[j].tl_data_contents = NULL;
-                    }
-                if (newtl) {
-                    free(newtl);
-                    newtl = NULL;
-                }
                 break;
 /* END CSTYLED */
             }