[SSSD] [PATCH] NSS: fix a use-after-free issue

Sumit Bose sbose at redhat.com
Mon Nov 2 12:51:38 UTC 2015


On Mon, Nov 02, 2015 at 10:30:51AM +0100, Lukas Slebodnik wrote:
> On (02/11/15 10:05), Sumit Bose wrote:
> >On Mon, Nov 02, 2015 at 09:42:51AM +0100, Lukas Slebodnik wrote:
> >> On (30/10/15 17:35), Sumit Bose wrote:
> >> >Hi,
> >> >
> >> >I found this accidentally because I was still running SSSD with
> >> >MALLOC_PERTURB_ set which I used some time ago to hunt a glibc issue.
> >> >
> >> >This issue wasn't caught before by the unit-tests because
> >> >sss_cmd_done() which frees the context is overwritten in the tests and
> >> >so far didn't free the context. Additionally, since the use after free
> >> >was in a debug message, the unit tests must be run with debugging enable
> >> >to run over it. I think this is also the reason why Coverity or Clang
> >> >didn't found this issue before. Does anyone know if it is possible to
> >> >tell Converity to assume debug_level is set to 10?
> >> >
> >> >bye,
> >> >Sumit
> >> >
> >> 
> >> >From 309f59199c49d3d4dc4fa42229f6b2fa1e82e72a Mon Sep 17 00:00:00 2001
> >> >From: Sumit Bose <sbose at redhat.com>
> >> >Date: Fri, 30 Oct 2015 16:28:37 +0100
> >> >Subject: [PATCH] NSS: fix a use-after-free issue
> >> >
> >> >---
> >> > src/responder/nss/nsssrv_cmd.c  | 10 ++++++----
> >> > src/tests/cmocka/test_nss_srv.c |  1 +
> >> > 2 files changed, 7 insertions(+), 4 deletions(-)
> >> >
> >> >diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
> >> >index b8bd6425e2c937ce6008fd6663fe0312ad68f01e..58d20fab11eea6f419031f439083ca5f6d8c61bb 100644
> >> >--- a/src/responder/nss/nsssrv_cmd.c
> >> >+++ b/src/responder/nss/nsssrv_cmd.c
> >> >@@ -5455,11 +5455,13 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx)
> >> >     ret = nss_check_well_known_sid(cmdctx);
> >> >     if (ret != ENOENT) {
> >> >         if (ret == EOK) {
> >> >-            DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n",
> >> >-                                         cmdctx->secid);
> >> >-        } else {
> >> >-            DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
> >> >+            DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n", sid_str);
> >> >+            /* message is already send and cmdctx is freed,
> >> >+             * we can just return */
> >> >+            return EOK;
> >> >         }
> >> >+
> >> >+        DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
> >>                 it would be good to log error code here.
> >> >         goto done;
> >> 
> >> Was "use-after-free" caused only by logging sid or was it used elsewhere?
> >
> >ah, sorry. Yes, I made two changes here, replacing the variable in the
> >debug message and returning immediately and no go to done. The
> >use-after-free was in the debug message. cmdctx is not touched in the
> >following code path anymore but since we are all set I think it is
> >safer to just return here.
> >
> >> 
> >> Could you append valgrind error to commit mesage?
> >
> >I found the issue via a core dump. Do you prefer the valgrind message or
> >the first steps of a stack trace?
> >
> We can see when memory was released from valgrind message.
> A stack trace just shows when it crashed.

good point, new version attached.

bye,
Sumit
-------------- next part --------------
From 9561733a6ce32b5787acc5277e2d45026c5126db Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Fri, 30 Oct 2015 16:28:37 +0100
Subject: [PATCH] NSS: fix a use-after-free issue

While handling well-known SIDs a debug statement tries to access memory that is
already freed. This can be seen with the following output from valgrind.

==17600== Invalid read of size 4
==17600==    at 0x805ACC6: nss_cmd_getbysid (nsssrv_cmd.c:5458)
==17600==    by 0x805AF41: nss_cmd_getnamebysid (nsssrv_cmd.c:5509)
==17600==    by 0x80662F4: sss_cmd_execute (responder_cmd.c:161)
==17600==    by 0x8067015: client_cmd_execute (responder_common.c:249)
==17600==    by 0x80671F5: client_recv (responder_common.c:283)
==17600==    by 0x806741C: client_fd_handler (responder_common.c:335)
==17600==    by 0x45F5112: epoll_event_loop (tevent_epoll.c:728)
==17600==    by 0x45F5112: epoll_event_loop_once (tevent_epoll.c:926)
==17600==    by 0x45F32EE: std_event_loop_once (tevent_standard.c:114)
==17600==    by 0x45EF3BF: _tevent_loop_once (tevent.c:530)
==17600==    by 0x45EF5AB: tevent_common_loop_wait (tevent.c:634)
==17600==    by 0x45F326E: std_event_loop_wait (tevent_standard.c:140)
==17600==    by 0x45EF647: _tevent_loop_wait (tevent.c:653)
==17600==  Address 0x4b248a0 is 72 bytes inside a block of size 88 free'd
==17600==    at 0x402C26D: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==17600==    by 0x45FEC9E: _talloc_free_internal (talloc.c:1057)
==17600==    by 0x45FEC9E: _talloc_free (talloc.c:1581)
==17600==    by 0x8066085: sss_cmd_done (responder_cmd.c:93)
==17600==    by 0x805A9B0: nss_check_well_known_sid (nsssrv_cmd.c:5382)
==17600==    by 0x805AC86: nss_cmd_getbysid (nsssrv_cmd.c:5455)
==17600==    by 0x805AF41: nss_cmd_getnamebysid (nsssrv_cmd.c:5509)
==17600==    by 0x80662F4: sss_cmd_execute (responder_cmd.c:161)
==17600==    by 0x8067015: client_cmd_execute (responder_common.c:249)
==17600==    by 0x80671F5: client_recv (responder_common.c:283)
==17600==    by 0x806741C: client_fd_handler (responder_common.c:335)
==17600==    by 0x45F5112: epoll_event_loop (tevent_epoll.c:728)
==17600==    by 0x45F5112: epoll_event_loop_once (tevent_epoll.c:926)
==17600==    by 0x45F32EE: std_event_loop_once (tevent_standard.c:114)
==17600==

The patch contains a change to the unit tests which frees the memory in
the wrapper for sss_cmd_done() too. This allows to detect this kind of
issue in the unit tests as well.
---
 src/responder/nss/nsssrv_cmd.c  | 10 ++++++----
 src/tests/cmocka/test_nss_srv.c |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index b8bd6425e2c937ce6008fd6663fe0312ad68f01e..58d20fab11eea6f419031f439083ca5f6d8c61bb 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -5455,11 +5455,13 @@ static int nss_cmd_getbysid(enum sss_cli_command cmd, struct cli_ctx *cctx)
     ret = nss_check_well_known_sid(cmdctx);
     if (ret != ENOENT) {
         if (ret == EOK) {
-            DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n",
-                                         cmdctx->secid);
-        } else {
-            DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
+            DEBUG(SSSDBG_TRACE_ALL, "SID [%s] is a Well-Known SID.\n", sid_str);
+            /* message is already send and cmdctx is freed,
+             * we can just return */
+            return EOK;
         }
+
+        DEBUG(SSSDBG_OP_FAILURE, "nss_check_well_known_sid failed.\n");
         goto done;
     }
 
diff --git a/src/tests/cmocka/test_nss_srv.c b/src/tests/cmocka/test_nss_srv.c
index 6bfbd574a4bb73f932d7ce7e0275769e4c43aa24..f05b55e461d9a058a4894800b2efae3417c8dc01 100644
--- a/src/tests/cmocka/test_nss_srv.c
+++ b/src/tests/cmocka/test_nss_srv.c
@@ -136,6 +136,7 @@ void __wrap_sss_cmd_done(struct cli_ctx *cctx, void *freectx)
     nss_test_ctx->tctx->error = check_cb(sss_packet_get_status(packet),
                                          body, blen);
     nss_test_ctx->tctx->done = true;
+    talloc_free(freectx);
 }
 
 enum sss_cli_command __wrap_sss_packet_get_cmd(struct sss_packet *packet)
-- 
2.1.0



More information about the sssd-devel mailing list