[SSSD] [PATCH] krb5: Check return value of sss_krb5_princ_realm
Lukas Slebodnik
lslebodn at redhat.com
Tue Oct 14 12:44:49 UTC 2014
On (14/10/14 14:14), Pavel Reichl wrote:
>
>On 10/14/2014 01:59 PM, Lukas Slebodnik wrote:
>>diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
>>index b4012593d96bc951143e4bb2ba7a91d118b1a53c..7daedb47464917966350f94c7d5b0844ccee7edb 100644
>>--- a/src/util/sss_krb5.c
>>+++ b/src/util/sss_krb5.c
>>@@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts,
>> void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ,
>> const char **realm, int *len)
>> {
>>- *realm = krb5_principal_get_realm(context, princ);
>>- *len = strlen(*realm);
>>+ const char *realm_str = krb5_principal_get_realm(context, princ);
>>+
>>+ if (realm_str != NULL) {
>>+ *realm = krb5_principal_get_realm(context, princ);
>
>Why do we have to call krb5_principal_get_realm() again here? Why can't we
>just reuse value in realm_str?
>
I just forgot to squash changes before sending patches.
Updated version is attached.
LS
-------------- next part --------------
>From dd5c7ea4ebc0002e529dafeceececcbdcc96e7cc Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 14 Oct 2014 11:20:40 +0200
Subject: [PATCH] krb5: Check return value of krb5_principal_get_realm
Function krb5_principal_get_realm can return NULL an it would
case segfault in function strlen.
---
src/util/sss_krb5.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index b4012593d96bc951143e4bb2ba7a91d118b1a53c..38d44e51926bc99c6e53851d7814b6d0be254086 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -834,8 +834,15 @@ void sss_krb5_get_init_creds_opt_set_canonicalize(krb5_get_init_creds_opt *opts,
void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ,
const char **realm, int *len)
{
- *realm = krb5_principal_get_realm(context, princ);
- *len = strlen(*realm);
+ const char *realm_str = krb5_principal_get_realm(context, princ);
+
+ if (realm_str != NULL) {
+ *realm = realm_str;
+ *len = strlen(realm_str);
+ } else {
+ *realm = NULL;
+ *len = 0;
+ }
}
#else
void sss_krb5_princ_realm(krb5_context context, krb5_const_principal princ,
--
2.1.0
-------------- next part --------------
>From a18938ac0f0c1bc259aed527ce34f274d0fd0473 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Mon, 14 Oct 2013 11:21:02 +0200
Subject: [PATCH 2/2] krb5: Check return value of sss_krb5_princ_realm
sss_krb5_princ_realm set output parameter realm to NULL and len to 0
in case of failure. Clang static analysers reported warning
"Null pointer passed as an argument to a 'nonnull' parameter"
in function match_principal. It was possible, that realm_name with value NULL
could be used in strncmp.
---
src/providers/krb5/krb5_child.c | 8 ++++++++
src/providers/krb5/krb5_utils.c | 5 +++++
src/util/sss_krb5.c | 13 +++++++++++++
3 files changed, 26 insertions(+)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 945f4c76ec48b594a8033438251cb0d913ddcf0c..0aa7abb8283956c7eb9d36a2b1ebaee0ae4a26c6 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -969,6 +969,10 @@ static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
}
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length);
+ if (realm_length == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "sss_krb5_princ_realm failed.\n");
+ return KRB5KRB_ERR_GENERIC;
+ }
DEBUG(SSSDBG_TRACE_FUNC,
"Attempting kinit for realm [%s]\n",realm_name);
@@ -1112,6 +1116,10 @@ static errno_t changepw_child(struct krb5_req *kr, bool prelim)
set_changepw_options(kr->options);
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length);
+ if (realm_length == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "sss_krb5_princ_realm failed.\n");
+ return ERR_INTERNAL;
+ }
DEBUG(SSSDBG_TRACE_FUNC,
"Attempting kinit for realm [%s]\n",realm_name);
diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
index 0d2e281198390043068e21b412b57de04df6a12b..4f578e4df9fe5b87781e02555c619e4445196202 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -653,6 +653,11 @@ errno_t get_ccache_file_data(const char *ccache_file, const char *client_name,
}
sss_krb5_princ_realm(ctx, client_princ, &realm_name, &realm_length);
+ if (realm_length == 0) {
+ kerr = KRB5KRB_ERR_GENERIC;
+ DEBUG(SSSDBG_CRIT_FAILURE, "sss_krb5_princ_realm failed.\n");
+ goto done;
+ }
server_name = talloc_asprintf(NULL, "krbtgt/%.*s@%.*s",
realm_length, realm_name,
diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index 7daedb47464917966350f94c7d5b0844ccee7edb..0cfb3873bd5a5d94c704f98ffd7388ead497b2f5 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -212,6 +212,15 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
sss_krb5_princ_realm(krb_ctx, client_princ,
&realm_name,
&realm_len);
+ if (realm_len == 0) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "sss_krb5_princ_realm failed.\n");
+ if (_principal) talloc_zfree(*_principal);
+ if (_primary) talloc_zfree(*_primary);
+ ret = EINVAL;
+ goto done;
+
+ }
+
*_realm = talloc_asprintf(mem_ctx, "%.*s",
realm_len, realm_name);
if (!*_realm) {
@@ -355,6 +364,10 @@ static bool match_principal(krb5_context ctx,
bool ret = false;
sss_krb5_princ_realm(ctx, principal, &realm_name, &realm_len);
+ if (realm_len == 0) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "sss_krb5_princ_realm failed.\n");
+ return false;
+ }
tmp_ctx = talloc_new(NULL);
if (!tmp_ctx) {
--
2.1.0
More information about the sssd-devel
mailing list