[SSSD] [PATCH] Don't call semanage transaction unless the context has changed

Jakub Hrozek jhrozek at redhat.com
Tue Apr 14 13:47:38 UTC 2015


On Tue, Apr 14, 2015 at 03:18:24PM +0200, Michal Židek wrote:
> On 04/10/2015 03:24 PM, Jakub Hrozek wrote:
> >Hi,
> >
> >the attached patches fix ticket
> >https://fedorahosted.org/sssd/ticket/2624
> >
> >It turns out that calling libsemanage transactions is a fairly intensive
> >operation that involves copying multiple files under the /etc/selinux
> >hierarchy to a temporary subtree and then back. With the patches, the
> >context from IPA is checked against the local database first and only
> >applied if it differs.
> >
> 
> Hi,
> 
> the patches look good, but you used talloc_strdup in
> libsss_semanage and dlopen test failed.
> 
> src/tests/dlopen-tests.c:149:F:dlopen:test_dlopen_base:0: Error opening
> libsss_semanage.so: [dlopen() failed:
> /home/user/gitrepo/sssd/.libs/libsss_semanage.so: undefined symbol:
> talloc_strdup]
> 
> I think you can feel free to link the library with talloc,
> there is no particular reason for not doing so, other than
> it was not needed until now. It is an "sss" internal library
> so talloc will have to be around anyway.
> 
> Michal

Yeah, I don't suppose talloc dependency would be a problem. See attached
patches.
-------------- next part --------------
>From 4de2f547bc03d17404d5d87f5d00a6574c92ad3c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 10 Apr 2015 10:55:22 +0200
Subject: [PATCH 1/3] selinux: Disconnect before closing the handle

libsemanage documentation says:
~~~~
be sure that a semanage_disconnect() was previously called if the handle
was connected.
~~~~

Otherwise we get a memory leak.
---
 src/util/sss_semanage.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
index b85831c3d3f262f49b19082e96aa62ccf3afeaa8..d141de1c671e6d62a731e56b10ee14069f27ae87 100644
--- a/src/util/sss_semanage.c
+++ b/src/util/sss_semanage.c
@@ -68,6 +68,13 @@ static void sss_semanage_error_callback(void *varg,
     free(message);
 }
 
+static void sss_semanage_close(semanage_handle_t *handle)
+{
+    /* Calling disconnect on a disconnected handle is safe */
+    semanage_disconnect(handle);
+    semanage_handle_destroy(handle);
+}
+
 static semanage_handle_t *sss_semanage_init(void)
 {
     int ret;
@@ -110,7 +117,7 @@ static semanage_handle_t *sss_semanage_init(void)
 
     return handle;
 fail:
-    semanage_handle_destroy(handle);
+    sss_semanage_close(handle);
     return NULL;
 }
 
@@ -278,7 +285,7 @@ int set_seuser(const char *login_name, const char *seuser_name,
     ret = EOK;
 done:
     semanage_seuser_key_free(key);
-    semanage_handle_destroy(handle);
+    sss_semanage_close(handle);
     return ret;
 }
 
@@ -350,7 +357,7 @@ int del_seuser(const char *login_name)
 
     ret = EOK;
 done:
-    semanage_handle_destroy(handle);
+    sss_semanage_close(handle);
     return ret;
 }
 
-- 
2.1.0

-------------- next part --------------
>From a2fd9fd69b58de5a90bc0736830ae10f38908589 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 10 Apr 2015 11:06:44 +0200
Subject: [PATCH 2/3] selinux: Begin and end the transaction on the same
 nesting level

Transaction should be started and commited on the same code nesting or
abstraction level. Also, transactions are really costly with libselinux
and splitting them from initialization will make init function reusable
by read-only libsemanage functions.
---
 src/util/sss_semanage.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
index d141de1c671e6d62a731e56b10ee14069f27ae87..c0342498cbd0495733a0bf701a06a02cfb705fc7 100644
--- a/src/util/sss_semanage.c
+++ b/src/util/sss_semanage.c
@@ -109,12 +109,6 @@ static semanage_handle_t *sss_semanage_init(void)
         goto fail;
     }
 
-    ret = semanage_begin_transaction(handle);
-    if (ret != 0) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot begin SELinux transaction\n");
-        goto fail;
-    }
-
     return handle;
 fail:
     sss_semanage_close(handle);
@@ -243,6 +237,13 @@ int set_seuser(const char *login_name, const char *seuser_name,
         goto done;
     }
 
+    ret = semanage_begin_transaction(handle);
+    if (ret != 0) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot begin SELinux transaction\n");
+        ret = EIO;
+        goto done;
+    }
+
     ret = semanage_seuser_key_create(handle, login_name, &key);
     if (ret != 0) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create SELinux user key\n");
@@ -303,6 +304,13 @@ int del_seuser(const char *login_name)
         goto done;
     }
 
+    ret = semanage_begin_transaction(handle);
+    if (ret != 0) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot begin SELinux transaction\n");
+        ret = EIO;
+        goto done;
+    }
+
     ret = semanage_seuser_key_create(handle, login_name, &key);
     if (ret != 0) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create SELinux user key\n");
-- 
2.1.0

-------------- next part --------------
>From 4118b786edbb81b75581532b97a8a82046d5956c Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Thu, 9 Apr 2015 22:18:35 +0200
Subject: [PATCH 3/3] selinux: Only call semanage if the context actually
 changes

https://fedorahosted.org/sssd/ticket/2624

Add a function to query the libsemanage database for a user context and
only update the database if the context differes from the one set on the
server.

Adds talloc dependency to libsss_semanage.
---
 Makefile.am                       |  5 +++
 src/providers/ipa/selinux_child.c | 35 ++++++++++++++++---
 src/util/sss_semanage.c           | 71 +++++++++++++++++++++++++++++++++++++++
 src/util/util.h                   |  2 ++
 4 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d9724093d0415c4ba1c092a915cab7092ecee67f..52fbd510d67489dfd65b003c99673dc0869cfdc0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -789,10 +789,15 @@ endif
 libsss_util_la_LDFLAGS = -avoid-version
 
 pkglib_LTLIBRARIES += libsss_semanage.la
+libsss_semanage_la_CFLAGS = \
+    $(AM_CFLAGS) \
+    $(TALLOC_CFLAGS) \
+    $(NULL)
 libsss_semanage_la_SOURCES = \
     src/util/sss_semanage.c \
     $(NULL)
 libsss_semanage_la_LIBADD = \
+    $(TALLOC_LIBS) \
     libsss_debug.la \
     $(NULL)
 if BUILD_SEMANAGE
diff --git a/src/providers/ipa/selinux_child.c b/src/providers/ipa/selinux_child.c
index 81c1de877ef08a299d07837fefcd195d465849fa..7c5731d66b7d0ed17b7be18c4adaa65394002fc4 100644
--- a/src/providers/ipa/selinux_child.c
+++ b/src/providers/ipa/selinux_child.c
@@ -165,6 +165,29 @@ static int sc_set_seuser(const char *login_name, const char *seuser_name,
     return ret;
 }
 
+static bool seuser_needs_update(struct input_buffer *ibuf)
+{
+    bool needs_update = true;
+    char *db_seuser = NULL;
+    char *db_mls_range = NULL;
+    errno_t ret;
+
+    ret = get_seuser(ibuf, ibuf->username, &db_seuser, &db_mls_range);
+    DEBUG(SSSDBG_TRACE_INTERNAL,
+          "get_seuser: ret: %d seuser: %s mls: %s\n",
+          ret, db_seuser ? db_seuser : "unknown",
+          db_mls_range ? db_mls_range : "unknown");
+    if (ret == EOK && db_seuser && db_mls_range &&
+            strcmp(db_seuser, ibuf->seuser) == 0 &&
+            strcmp(db_mls_range, ibuf->mls_range) == 0) {
+        needs_update = false;
+    }
+
+    talloc_free(db_seuser);
+    talloc_free(db_mls_range);
+    return needs_update;
+}
+
 int main(int argc, const char *argv[])
 {
     int opt;
@@ -177,6 +200,7 @@ int main(int argc, const char *argv[])
     struct input_buffer *ibuf = NULL;
     struct response *resp = NULL;
     ssize_t written;
+    bool needs_update;
 
     struct poptOption long_options[] = {
         POPT_AUTOHELP
@@ -296,10 +320,13 @@ int main(int argc, const char *argv[])
 
     DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");
 
-    ret = sc_set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot set SELinux login context.\n");
-        goto fail;
+    needs_update = seuser_needs_update(ibuf);
+    if (needs_update == true) {
+        ret = sc_set_seuser(ibuf->username, ibuf->seuser, ibuf->mls_range);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "Cannot set SELinux login context.\n");
+            goto fail;
+        }
     }
 
     ret = prepare_response(main_ctx, ret, &resp);
diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
index c0342498cbd0495733a0bf701a06a02cfb705fc7..01a2f41d8752e127f2aa1b72faa61c23f315edd7 100644
--- a/src/util/sss_semanage.c
+++ b/src/util/sss_semanage.c
@@ -369,6 +369,71 @@ done:
     return ret;
 }
 
+int get_seuser(TALLOC_CTX *mem_ctx, const char *login_name,
+               char **_seuser, char **_mls_range)
+{
+    errno_t ret;
+    const char *seuser;
+    const char *mls_range;
+    semanage_handle_t *sm_handle = NULL;
+    semanage_seuser_t *sm_user = NULL;
+    semanage_seuser_key_t *sm_key = NULL;
+
+    sm_handle = sss_semanage_init();
+    if (sm_handle == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create SELinux handle\n");
+        ret = EIO;
+        goto done;
+    }
+
+    ret = semanage_seuser_key_create(sm_handle, login_name, &sm_key);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot create key for %s\n", login_name);
+        ret = EIO;
+        goto done;
+    }
+
+    ret = semanage_seuser_query(sm_handle, sm_key, &sm_user);
+    if (ret < 0) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot query for %s\n", login_name);
+        ret = EIO;
+        goto done;
+    }
+
+    seuser = semanage_seuser_get_sename(sm_user);
+    if (seuser != NULL) {
+        *_seuser = talloc_strdup(mem_ctx, seuser);
+        if (*_seuser == NULL) {
+            ret = ENOMEM;
+            goto done;
+        }
+        DEBUG(SSSDBG_OP_FAILURE,
+              "SELinux user for %s: %s\n", login_name, *_seuser);
+    } else {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot get sename for %s\n", login_name);
+    }
+
+    mls_range = semanage_seuser_get_mlsrange(sm_user);
+    if (mls_range != NULL) {
+        *_mls_range = talloc_strdup(mem_ctx, mls_range);
+        if (*_mls_range == NULL) {
+            ret = ENOMEM;
+            goto done;
+        }
+        DEBUG(SSSDBG_OP_FAILURE,
+              "SELinux range for %s: %s\n", login_name, *_mls_range);
+    } else {
+        DEBUG(SSSDBG_CRIT_FAILURE, "Cannot get mlsrange for %s\n", login_name);
+    }
+
+    ret = EOK;
+done:
+    semanage_seuser_key_free(sm_key);
+    semanage_seuser_free(sm_user);
+    sss_semanage_close(sm_handle);
+    return ret;
+}
+
 #else /* HAVE_SEMANAGE */
 int set_seuser(const char *login_name, const char *seuser_name,
                const char *mls)
@@ -380,4 +445,10 @@ int del_seuser(const char *login_name)
 {
     return EOK;
 }
+
+int get_seuser(TALLOC_CTX *mem_ctx, const char *login_name,
+               char **_seuser, char **_mls_range)
+{
+    return EOK;
+}
 #endif  /* HAVE_SEMANAGE */
diff --git a/src/util/util.h b/src/util/util.h
index d831d533f38a9ecae1f24265e3f2869eb6144a8b..c86bcea5b01f6bc02d18b17a2660262105648cf9 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -655,6 +655,8 @@ errno_t restore_creds(struct sss_creds *saved_creds);
 int set_seuser(const char *login_name, const char *seuser_name,
                const char *mlsrange);
 int del_seuser(const char *login_name);
+int get_seuser(TALLOC_CTX *mem_ctx, const char *login_name,
+               char **_seuser, char **_mls_range);
 
 /* convert time from generalized form to unix time */
 errno_t sss_utc_to_time_t(const char *str, const char *format, time_t *unix_time);
-- 
2.1.0



More information about the sssd-devel mailing list