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 <sb...@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 <sb...@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 <sb...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel