Correct a segfault in kadmind initialization

Review Request #192 - Created April 1, 2015 and updated

Information
Dale Ghent
illumos-gate
Reviewers
csiden

4467: kadmind: segfault in acquire_accept_cred+0x140()

kadmind(1M) segfaults on startup when the kdb name is not first checked that it is null. krb5_ktkdb_resolve() was not doing enough to ensure that the returned keytab_id->data struct was populated correctly. The attached patch corrects this.

This issue was first reported in illumos issue 4467

Issues

  • 0
  • 0
  • 2
  • 2
Description From Last Updated
Josef 'Jeff' Sipek

   
usr/src/lib/krb5/kdb/keytab.c (Diff revision 1)
 
 

This wrapper for the pointer seems a bit silly. I'm assuming that this isn't the only use of it and so it doesn't make sense to nuke it.

  1. Yeah, it mirrors the general convention used in this code base. I wanted to fix without drastically departing from the theme of the rest of the untouched code, however antiquated or distasteful it might be.

usr/src/lib/krb5/kdb/keytab.c (Diff revision 1)
 
 

Is it really supposed to be malloc/krb5_xfree? Is there no krb5_malloc that should be used here?

  1. There is no general-purpose private malloc in this krb5 suite. The use of malloc() + krb5_xfree() here mirrors what appears to be normal use elsewhere in the krb5 codebase. krb5_xfree() exists "to keep lint happy" - there is likely some pre-ANSI C reason for this that escapes me. See: http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/gssapi/mechs/krb5/include/krb5.h#2603

Josef 'Jeff' Sipek
Ship It!
Richard PALO

   
usr/src/lib/krb5/kdb/keytab.c (Diff revision 1)
 
 

perhaps a silly question, but is there chance that krb5_ktkdb_resolve be called with *name='\0'?

  1. I inspected other resolver functions in the krb5 code and, while those use a combination of calloc() and strcpy() to acheive the same result as my use of strdup() in this patch, none seem concerned at all that name might be "\0"

  2. Doesn't strdup("") return "", which is just what you want?

    This is an improvement over the older code. Is it a showstopper?

  3. Well, I don't think it is a show-stopper, just would sort of like knowing the intention with a name like ""...

    That is, a name which is "" isn' it meant to mean no name... or effectively NULL (or default)?

  4. krb5_ktkdb_resolve() is directly called in one place with name = NULL. It's used in other places, however, called via pointer and with names other than NULL.

  5. Again, I notice upstream MIT KRB5 hasn't touched this module, so I don't consider this at all a show-stopper.
    Sorry, old habit looking at this from a formal methods point of view...

Marcel Telka

Any chance to see this fix integrated?

https://www.listbox.com/member/archive/182179/2015/04/sort/time_rev/page/20/entry/20:552/20150404034639:B19CD4E6-DA9E-11E4-8EBF-D7E9065ACED8/

Loading...