[SSSD] [PATCH] Do not segfault selinux_child if semanage_connect fails

Jakub Hrozek jhrozek at redhat.com
Mon May 11 08:24:58 UTC 2015


On Mon, May 11, 2015 at 09:34:01AM +0200, Lukas Slebodnik wrote:
> On (07/05/15 12:01), Jakub Hrozek wrote:
> >Hi,
> >
> >the attached patch fixes https://fedorahosted.org/sssd/ticket/2649.
> >
> >I couldn't reproduce the bug with regular testing, but only by overriding the
> >return value from semanage_connect()
> 
> >From 4116f54398994ed15a5506b7927f24ffdca3d19c Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek at redhat.com>
> >Date: Wed, 6 May 2015 08:40:12 +0200
> >Subject: [PATCH] SELINUX: Avoid disconnecting disconnected handle
> >
> >Resolves:
> >    https://fedorahosted.org/sssd/ticket/2649
> >
> >libsemanage is very strict about its API usage and actually doesn't
> >allow disconnecting a handle that is not connected. The unpatched code
> >would fail with:
> >
> >    selinux_child: handle.c:231: semanage_disconnect: Assertion `sh !=
> >    ((void *)0) && sh->funcs != ((void *)0) && sh->funcs->disconnect !=
> >    ((void *)0)' failed.
> >
> >If semanage_connect() failed.
> >---
> > src/util/sss_semanage.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
> >index 01a2f41d8752e127f2aa1b72faa61c23f315edd7..d1d03988c05dc011dbd465051d50fe6acca4f845 100644
> >--- a/src/util/sss_semanage.c
> >+++ b/src/util/sss_semanage.c
> >@@ -70,8 +70,13 @@ static void sss_semanage_error_callback(void *varg,
> > 
> > static void sss_semanage_close(semanage_handle_t *handle)
> > {
> >-    /* Calling disconnect on a disconnected handle is safe */
> >-    semanage_disconnect(handle);
> >+    if (handle == NULL) {
> >+        return;     /* semanage uses asserts */
> >+    }
> >+
> >+    if (semanage_is_connected(handle)) {
> >+        semanage_disconnect(handle);
> >+    }
> >     semanage_handle_destroy(handle);
> Althought handle could be NULL for function semanage_handle_destroy.
> The documentation is clear.
> 
>    Deallocate all space associated with a semanage_handle_t, including
>    the pointer itself.  CAUTION: this function does not disconnect
>    from the backend; be sure that a semanage_disconnect() was
>    previously called if the handle was connected. */
>    void semanage_handle_destroy(semanage_handle_t *);
> 
> But neither semanage_is_connected nor semanage_disconnect accept NULL.
> There is an assert in code but documentation does not say so.
> 
>    Disconnect from the manager given by the handle.  If already
>    disconnected then this function does nothing.  Return 0 if
>    disconnected properly or already disconnected, negative value on
>    error. */
>    int semanage_disconnect(semanage_handle_t *);
> 
> ACK
> http://sssd-ci.duckdns.org/logs/job/14/97/summary.html
> 
> LS

Thanks, the comment in semanage header is exactly why we had the comment
in the code that I removed with this patch..

Pushed upstream:
 * master: 589a8760b38d9e2dfa278764af12d59e1487fe07
 * sssd-1-12: 2ed3ac5190cd8f92d671e00837a360bd92cf150c 



More information about the sssd-devel mailing list