-----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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlIWHbwACgkQeiVVYja6o6PixwCgsTSL37MAYFy15IWeYlXiT3yT Q2sAoIFeyktu+od/kuTmua9GjvH51KBD =mn98 -----END PGP SIGNATURE-----
>From e55e7a1a25e85428c8174c5b48b8dd9a41efde65 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <[email protected]> Date: Thu, 15 Aug 2013 11:40:59 -0400 Subject: [PATCH 3/7] KRB5: Refactor cc_*_check_existing 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. --- src/providers/krb5/krb5_utils.c | 120 ++++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 61 deletions(-) diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c index 1b6d57c608e56c995dd4e852093e9e045b2d40a8..080e12b158f590488668a8f249052d0ae777b0c0 100644 --- a/src/providers/krb5/krb5_utils.c +++ b/src/providers/krb5/krb5_utils.c @@ -704,6 +704,60 @@ done: return ret; } +static errno_t +check_cc_validity(const char *location, + const char *realm, + const char *princ, + bool *_valid) +{ + errno_t ret; + bool valid = false; + krb5_ccache ccache = NULL; + krb5_context context = NULL; + krb5_error_code krberr; + + krberr = krb5_init_context(&context); + if (krberr) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to init kerberos context\n")); + return EIO; + } + + krberr = krb5_cc_resolve(context, location, &ccache); + if (krberr == KRB5_FCC_NOFILE || ccache == NULL) { + /* KRB5_FCC_NOFILE would be returned if the directory components + * of the DIR cache do not exist, which is the case in /run + * after a reboot + */ + DEBUG(SSSDBG_TRACE_FUNC, + ("ccache %s is missing or empty\n", location)); + valid = false; + ret = EOK; + goto done; + } else if (krberr != 0) { + KRB5_DEBUG(SSSDBG_OP_FAILURE, context, krberr); + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_cc_resolve failed.\n")); + ret = EIO; + goto done; + } + + krberr = check_for_valid_tgt(context, ccache, realm, princ, &valid); + if (krberr != EOK) { + KRB5_DEBUG(SSSDBG_OP_FAILURE, context, krberr); + DEBUG(SSSDBG_CRIT_FAILURE, + ("Could not check if ccache contains a valid principal\n")); + ret = EIO; + goto done; + } + + ret = EOK; + *_valid = valid; + + done: + if (ccache) krb5_cc_close(context, ccache); + krb5_free_context(context); + return ret; +} + /*======== ccache back end utilities ========*/ struct sss_krb5_cc_be * get_cc_be_ops(enum sss_krb5_cc_type type) @@ -852,9 +906,6 @@ cc_file_check_existing(const char *location, uid_t uid, bool active; bool valid; const char *filename; - krb5_ccache ccache = NULL; - krb5_context context = NULL; - krb5_error_code kerr; filename = sss_krb5_residual_check_type(location, SSS_KRB5_TYPE_FILE); if (!filename) { @@ -878,28 +929,9 @@ cc_file_check_existing(const char *location, uid_t uid, return ret; } - kerr = krb5_init_context(&context); - if (kerr != 0) { - DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to init kerberos context\n")); - return EIO; - } - - kerr = krb5_cc_resolve(context, location, &ccache); - if (kerr != 0) { - KRB5_DEBUG(SSSDBG_OP_FAILURE, context, kerr); - krb5_free_context(context); - DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_cc_resolve failed.\n")); - return EIO; - } - - kerr = check_for_valid_tgt(context, ccache, realm, princ, &valid); - krb5_free_context(context); - krb5_cc_close(context, ccache); - if (kerr != EOK) { - KRB5_DEBUG(SSSDBG_OP_FAILURE, context, kerr); - DEBUG(SSSDBG_CRIT_FAILURE, - ("Could not check if ccache contains a valid principal\n")); - return EIO; + ret = check_cc_validity(location, realm, princ, &valid); + if (ret != EOK) { + return ret; } *_active = active; @@ -977,9 +1009,6 @@ cc_dir_check_existing(const char *location, uid_t uid, bool active = false; bool active_primary = false; bool valid = false; - krb5_ccache ccache = NULL; - krb5_context context = NULL; - krb5_error_code krberr; enum sss_krb5_cc_type type; const char *filename; const char *dir; @@ -1062,45 +1091,14 @@ cc_dir_check_existing(const char *location, uid_t uid, goto done; } - krberr = krb5_init_context(&context); - if (krberr) { - DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to init kerberos context\n")); - ret = EIO; - goto done; - } - - krberr = krb5_cc_resolve(context, location, &ccache); - if (krberr == KRB5_FCC_NOFILE || ccache == NULL) { - /* KRB5_FCC_NOFILE would be returned if the directory components - * of the DIR cache do not exist, which is the case in /run - * after a reboot - */ - DEBUG(SSSDBG_TRACE_FUNC, - ("ccache %s is missing or empty\n", location)); - valid = false; - ret = EOK; - goto done; - } else if (krberr != 0) { - KRB5_DEBUG(SSSDBG_OP_FAILURE, context, krberr); - DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_cc_resolve failed.\n")); - ret = EIO; - goto done; - } - - krberr = check_for_valid_tgt(context, ccache, realm, princ, &valid); - if (krberr != EOK) { - KRB5_DEBUG(SSSDBG_OP_FAILURE, context, krberr); - DEBUG(SSSDBG_CRIT_FAILURE, - ("Could not check if ccache contains a valid principal\n")); - ret = EIO; + ret = check_cc_validity(location, realm, princ, &valid); + if (ret != EOK) { goto done; } ret = EOK; done: talloc_free(tmp_ctx); - if (ccache) krb5_cc_close(context, ccache); - krb5_free_context(context); *_active = active; *_valid = valid; return ret; -- 1.8.3.1
>From 5270d9dca8ce887fb3200d0eda5dabb9e98fbf72 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <[email protected]> Date: Thu, 15 Aug 2013 18:40:30 -0400 Subject: [PATCH 4/7] KRB5: 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. --- src/providers/krb5/krb5_utils.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c index 080e12b158f590488668a8f249052d0ae777b0c0..883a3980077610dd0569a00438ed051da510227f 100644 --- a/src/providers/krb5/krb5_utils.c +++ b/src/providers/krb5/krb5_utils.c @@ -1006,9 +1006,9 @@ cc_dir_check_existing(const char *location, uid_t uid, const char *realm, const char *princ, const char *cc_template, bool *_active, bool *_valid) { - bool active = false; + bool active; bool active_primary = false; - bool valid = false; + bool valid; enum sss_krb5_cc_type type; const char *filename; const char *dir; @@ -1068,7 +1068,6 @@ cc_dir_check_existing(const char *location, uid_t uid, ("Could not check if ccache is active.\n")); } cc_check_template(cc_template); - active = false; goto done; } @@ -1087,7 +1086,6 @@ cc_dir_check_existing(const char *location, uid_t uid, DEBUG(SSSDBG_OP_FAILURE, ("Could not check if file 'primary' [%s] in dir ccache" " is active.\n", primary_file)); - active = false; goto done; } @@ -1096,11 +1094,12 @@ cc_dir_check_existing(const char *location, uid_t uid, goto done; } + *_active = active; + *_valid = valid; ret = EOK; + done: talloc_free(tmp_ctx); - *_active = active; - *_valid = valid; return ret; } -- 1.8.3.1
0003-KRB5-Refactor-cc_-_check_existing.patch.sig
Description: PGP signature
0004-KRB5-Only-set-active-and-valid-on-success.patch.sig
Description: PGP signature
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
