[SSSD] [PATCH] KRB5: Refactor cc_*_check_existing

Jakub Hrozek jhrozek at redhat.com
Thu Aug 22 17:37:55 UTC 2013


On Thu, Aug 22, 2013 at 07:27:07PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 22, 2013 at 10:18:36AM -0400, Stephen Gallagher wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 08/22/2013 10:11 AM, Stephen Gallagher wrote:
> > > On 08/15/2013 11:50 AM, Stephen Gallagher wrote:
> > >> There was duplicated code in cc_file_check_existing() and in 
> > >> cc_dir_check_existing(). I pulled them into the same function.
> > > 
> > >> There are two changes made to the original code here: 1) Fixes a 
> > >> use-after-free bug in cc_file_check_existing(). In the original 
> > >> code, we called krb5_free_context() and then used that context 
> > >> immediately after that in krb5_cc_close(). This patch corrects
> > >> the ordering
> > > 
> > >> 2) The krb5_cc_resolve() call handles KRB5_FCC_NOFILE for all
> > >> cache types. Previously, this was only handled for DIR caches.
> > > 
> > >> This second part I need someone with Kerberos knowledge to
> > >> verify. Is there a risk of receiving this error for the FILE or
> > >> KEYRING types, and if so is this handling still acceptable or
> > >> should they be special-cased?
> > > 
> > > 
> > > 
> > > Self-nack. New patch attached (I was never actually returning the 
> > > validity result).
> > > 
> > > Interdiff:
> > > 
> > > diff --git a/src/providers/krb5/krb5_utils.c 
> > > b/src/providers/krb5/krb5_utils.c index 47c45d0..8166435 100644 ---
> > > a/src/providers/krb5/krb5_utils.c +++
> > > b/src/providers/krb5/krb5_utils.c @@ -750,6 +750,7 @@
> > > check_cc_validity(const char *location, }
> > > 
> > > ret = EOK; +    *_valid = valid;
> > > 
> > > done: if (ccache) krb5_cc_close(context, ccache);
> > > 
> > 
> > 
> > Adding another consistency patch that goes along with this:
> > 
> > 
> > Only set active and valid on success
> > 
> > The FILE cache only sets the return values of _active and _bool if the
> > entire function succeeds. The DIR cache was setting it even on failure.
> > This patch makes both consistent. This will benefit static analysis
> > tools which would be able to detect if the variable is ever used
> > uninitialized anywhere.
> > 
> > 
> > Both patches are attached for simplicity.
> 
> ACK to both. I tested that the ccache is reused if exists and recreated
> if doesn't exist for both DIR and FILE ccaches.

Pushed to master and sssd-1-10



More information about the sssd-devel mailing list