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(a)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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel