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

Lukas Slebodnik lslebodn at redhat.com
Wed Nov 4 08:40:47 UTC 2015


On (04/11/15 09:36), Lukas Slebodnik wrote:
>On (03/11/15 12:38), Sumit Bose wrote:
>>On Tue, Nov 03, 2015 at 11:46:42AM +0100, Lukas Slebodnik wrote:
>>> On (02/11/15 13:51), Sumit Bose wrote:
>>> >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
>>> 
>>> >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;
>>> Previously we used "goto done" for EOK.
>>> I tested with pysss_nss_idmap.getnamebysid('S-1-5-32-545') and
>>> I could not see any difference between "goto done" and "return EOK".
>>> So I'm fine with both version or did I miss something?
>>
>>No, I just think it is safer to immediately return here. nss_cmd_done()
>>just returns in the ret==EOK case as well, but since it expects cmdctx
>>as an argument I think it should be only called as long as cmdctx is
>>still valid.
>>
>>> 
>>> But it might be simpler to read the code if we transform nested if statements
>>> to swith case. Otherwise LGTM.
>>
>>good point, new version attached. I hope you don't mind the
>>'return-break' sequence. I see the break here as syntactical sugar to
>>underline that the case block is closed and since it is already used in
>>the code
>I fine with this version.
>
>>I hope static analysers won't complain about unreachable code
>>here.
>No complains :-)
>
>>From 2ceeaa87de7c2125def64cb5430f9acffbcccb72 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.
>>
>ACK
>
>http://sssd-ci.duckdns.org/logs/job/31/93/summary.html
>
master:
* 343b053bc61792023003d077ae81c05ff1676a89

LS


More information about the sssd-devel mailing list