Patch: System keytab usage improvements

[Jeremy Allison]

On Wed, Jun 02, 2004 at 09:17:49PM -0400, Dan Perry wrote:
> >
> > 1)	The "net ads join" command says that it supports having the OU
> > for the computer's account specified on the command line.  However,
> > reading through the source code, the specified OU never gets used.  It
> > appears that it makes it all the way to the call to
> > ads_add_machine_acct(), but that function never uses the value passed to
> > org_unit when building the comp_dn string:
> > 
> > comp_dn = talloc_asprintf(ctx, "cn=%s,%s,%s", hostname,
> > ads_ou_string(NULL), ads->config.bind_path);
> > 
> > Note that NULL is passed to ads_ou_string.  Shouldn't org_unit get
> > passed there instead?
> > 
> 
> Yes, good catch.  Here's a link to an updated keytab patch that fixes that
> issues:
> 
> http://www.pppl.gov/~dperry/patches/keytab.v7.samba-3.0.5pre1.diff

I'm trying to integrate this but I think there are some memory
corruption problem with it.

For example, in the new file libads/kerberos_keytab.c we have :

+               if (!(key = (krb5_keyblock *)malloc(sizeof(*key)))) {
+                       return ENOMEM;
+               }
+
+               /* add keytab entries for all encryption types */
+               for ( i=0; enctypes[i]; i++ ) {
+
+               if (create_kerberos_key_from_string(context, host_princ, &password, key, enctypes[i])) {
+                       continue;
+                       }
+
+                       entry.principal = host_princ;
+                       entry.vno       = kvno;
+
+#if !defined(HAVE_KRB5_KEYTAB_ENTRY_KEY) && !defined(HAVE_KRB5_KEYTAB_ENTRY_KEYBLOCK)
+#error krb5_keytab_entry has no key or keyblock member
+#endif
+
+#ifdef HAVE_KRB5_KEYTAB_ENTRY_KEY      /* MIT */
+                       entry.key = *key;
+#endif
+
+#ifdef HAVE_KRB5_KEYTAB_ENTRY_KEYBLOCK         /* Heimdal */
+                       entry.keyblock = *key;
+#endif
+                       DEBUG(10,("adding keytab-entry for (%s) with encryption type (%d) and version (%d)\n",
+                                host_princ_s, enctypes[i], entry.vno));
+                ret = krb5_kt_add_entry(context, *keytab, &entry);
+                if (ret) {
+                               DEBUG(1,("adding entry to keytab failed (%s)\n", error_message(ret)));
+                        free_keytab(context, *keytab);
+                        return ret;
+                }
+               }
+               krb5_free_keyblock(context, key);

Here *key is malloced, then the contents structure assigned to entry.key or entry.keyblock.
This copies the pointer key->contents into entry.key->contents. This pointer is then freed
by the krb5_free_keyblock() call - this is the same pointer value in entry->key.contents.

There are also memory leaks in the error code paths.

Was this patch run under valgrind ?

I can fix this, but integrating this is going to take much more time as I've got
to comb the patch line by line now for these kind of mistakes :-(.

Jeremy.