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

Jakub Hrozek jhrozek at redhat.com
Thu Aug 22 17:27:07 UTC 2013


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.



More information about the sssd-devel mailing list