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 hope static analysers won't complain about unreachable code here. bye, Sumit > > LS > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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. ==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 | 17 ++++++++++------- src/tests/cmocka/test_nss_srv.c | 1 + 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index b8bd6425e2c937ce6008fd6663fe0312ad68f01e..d6ac9dc286764255b1a800efdb69b0af29520e32 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 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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel