From 343b053bc61792023003d077ae81c05ff1676a89 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose@redhat.com>
Date: Nov 04 2015 08:37:57 +0000
Subject: 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.

Reviewed-by: Lukáš Slebodník <lslebodn@redhat.com>

---

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index b8bd642..d6ac9dc 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -5453,13 +5453,16 @@ 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");
-        }
+    switch (ret) {
+    case EOK:
+        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;
+        break;
+    case ENOENT:
+        break;
+    default:
+        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 6bfbd57..f05b55e 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)