-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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?
On Thu, 2013-08-15 at 11:50 -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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:
- 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
Thanks, I also hate to see so much was duplicated.
- 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?
All there types will return that error (which is actually a File ccache error later reused by DIR and KEYRING). And no I do not think there should be any special casing.
LGTM.
Simo.
On Thu, Aug 15, 2013 at 11:50:14AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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:
- 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
- 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?
I think it is save for two reasons. First, after a quick look at the MIT source code I think that currently FILE and KEYRING do not return KRB5_FCC_NOFILE when calling krb5_cc_resolve(). And even if this is changed in some other version the current code will return an error and the authentication will fail. Your version will assume that the ticket in not valid and try to get a new one, which looks more friendly to the user than just failing.
I'll try to run some tests later today.
bye, Sumit
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlIM+LYACgkQeiVVYja6o6MNjACfSxhKZIq3nr9YSG3lro9kKQ2A zIIAni3Px6SSQ9CU/x3ltMW2VTJ1Scan =pTjc -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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
- 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);
-----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
- 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.
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
- 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.
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
- 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
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce -
Stephen Gallagher -
Sumit Bose