[SSSD] [PATCH] krb5: Check return value of sss_krb5_princ_realm

Lukas Slebodnik lslebodn at redhat.com
Tue Nov 25 15:48:02 UTC 2014


On (25/11/14 16:32), Pavel Reichl wrote:
>
>On 11/25/2014 03:33 PM, Lukas Slebodnik wrote:
>>On (14/10/14 14:44), Lukas Slebodnik wrote:
>>>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.
>>>
>>Attached are updated patches which can be applied on current master.
>>
>>LS
>>
>I think you rebased wrong patch - there were some changes which are not in
>this patch - you can see my prev post and you reaction to see what I mean.
yep,

I just sent wrong patch. The problematic was the 2nd patch.

LS
-------------- next part --------------
>From e49b739906f752b7cc9a7a5ec56fb442519adcca 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 1/2] 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 9eb34e17dc1059da9c346d4635a9f3e283308328..89240b86d06a9f57f12d09636b23b9de7a48b7c6 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -758,8 +758,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 2b2637a50cdd288a1128bad67b699f1faed4a278 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_ccache.c |  5 +++++
 src/providers/krb5/krb5_child.c  |  8 ++++++++
 src/util/sss_krb5.c              | 12 ++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/src/providers/krb5/krb5_ccache.c b/src/providers/krb5/krb5_ccache.c
index 7aa36b744ddcf7e46edcc26405a5101645b8b546..8da562b50456d2344b10bfb410cc0d54de317069 100644
--- a/src/providers/krb5/krb5_ccache.c
+++ b/src/providers/krb5/krb5_ccache.c
@@ -574,6 +574,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/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 94cd34e433cf6a197860d233fbf9ca30cd3eb535..ec22665b5fd2d2cfa7863b36e077d2218f8d2acb 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -974,6 +974,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);
@@ -1136,6 +1140,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/util/sss_krb5.c b/src/util/sss_krb5.c
index 89240b86d06a9f57f12d09636b23b9de7a48b7c6..b5cc5537602c66d5f2296e296d8e127d34d27a8d 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -212,6 +212,14 @@ 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) {
@@ -279,6 +287,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