Re: [SSSD] [PATCH] NSS: Fix use after free

2015-09-01 Thread Jakub Hrozek 
On Wed, Aug 19, 2015 at 11:15:16PM +0200, Jakub Hrozek wrote:
> On Thu, Aug 13, 2015 at 07:41:02AM +0200, Lukas Slebodnik wrote:
> > On (12/08/15 14:17), Jakub Hrozek wrote:
> > >On Mon, Aug 10, 2015 at 06:38:29AM +0200, Lukas Slebodnik wrote:
> > >> ehlo,
> > >> 
> > >> Use after free can happed if there are two domains and user is not found
> > >> in the first one.
> > >> 
> > >> LS
> > >
> > >Would it be possible to write a testcase in the NSS responder test?
> > It requires multi domain setup.
> > So I created different test.
> > My intention was to cover most test cases and not just initgroups,
> > But attached ins a POC patch which prove there is a use after free.
> > make check passes; you need to test with valgrind.
> > libtool --mode=execute valgrind -v ./nss-srv-multi-tests
> > 
> > Would you prefer to use current version of patch and add othter test cases
> > later? (it will take some time) or current version is enought for fix?
> 
> Ideally I think we should have only one NSS responder test, otherwise we
> would end up adding some testcases to one test and not the other...but I
> haven't tried, so I don't know how easy or hard that is.
> 
> ACK to your crash patch, I'll push it and apply to downstream.

Sorry, I forgot to send push-mail:
b9901fe3d6cfe05cd75a2440c0f9c7985aea36c6
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] NSS: Fix use after free

2015-08-19 Thread Jakub Hrozek 
On Thu, Aug 13, 2015 at 07:41:02AM +0200, Lukas Slebodnik wrote:
> On (12/08/15 14:17), Jakub Hrozek wrote:
> >On Mon, Aug 10, 2015 at 06:38:29AM +0200, Lukas Slebodnik wrote:
> >> ehlo,
> >> 
> >> Use after free can happed if there are two domains and user is not found
> >> in the first one.
> >> 
> >> LS
> >
> >Would it be possible to write a testcase in the NSS responder test?
> It requires multi domain setup.
> So I created different test.
> My intention was to cover most test cases and not just initgroups,
> But attached ins a POC patch which prove there is a use after free.
> make check passes; you need to test with valgrind.
> libtool --mode=execute valgrind -v ./nss-srv-multi-tests
> 
> Would you prefer to use current version of patch and add othter test cases
> later? (it will take some time) or current version is enought for fix?

Ideally I think we should have only one NSS responder test, otherwise we
would end up adding some testcases to one test and not the other...but I
haven't tried, so I don't know how easy or hard that is.

ACK to your crash patch, I'll push it and apply to downstream.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] NSS: Fix use after free

2015-08-12 Thread Lukas Slebodnik 
On (12/08/15 14:17), Jakub Hrozek wrote:
>On Mon, Aug 10, 2015 at 06:38:29AM +0200, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> Use after free can happed if there are two domains and user is not found
>> in the first one.
>> 
>> LS
>
>Would it be possible to write a testcase in the NSS responder test?
It requires multi domain setup.
So I created different test.
My intention was to cover most test cases and not just initgroups,
But attached ins a POC patch which prove there is a use after free.
make check passes; you need to test with valgrind.
libtool --mode=execute valgrind -v ./nss-srv-multi-tests

Would you prefer to use current version of patch and add othter test cases
later? (it will take some time) or current version is enought for fix?

LS
>From a0043e064cf60a2df53ff544fc9c88eb85af6853 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 10 Aug 2015 12:43:22 +0200
Subject: [PATCH] test_nss_srv_multi: Subject

Explanation

Resolves:
https://fedorahosted.org/sssd/ticket/
---
 Makefile.am   |  28 +++
 src/tests/cmocka/test_nss_srv_multi.c | 437 ++
 2 files changed, 465 insertions(+)
 create mode 100644 src/tests/cmocka/test_nss_srv_multi.c

diff --git a/Makefile.am b/Makefile.am
index 
8b64317d6dce9a1ee8614916395b9afd9f11f382..01e5c6c9c1af21d60a0d4922b327dada94f87704
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -201,6 +201,7 @@ endif # HAVE_CHECK
 if HAVE_CMOCKA
 non_interactive_cmocka_based_tests = \
 nss-srv-tests \
+nss-srv-multi-tests \
 test-find-uid \
 test-io \
 test-negcache \
@@ -1850,6 +1851,33 @@ TEST_MOCK_PROVIDER_OBJ = \
  src/tests/cmocka/common_mock_sdap.c \
  src/tests/cmocka/common_mock_sysdb_objects.c
 
+EXTRA_nss_srv_multi_tests_DEPENDENCIES = \
+ $(ldblib_LTLIBRARIES)
+nss_srv_multi_tests_SOURCES = \
+ $(TEST_MOCK_RESP_OBJ) \
+ src/tests/cmocka/test_nss_srv_multi.c \
+ src/responder/nss/nsssrv_cmd.c \
+ src/responder/nss/nsssrv_netgroup.c \
+ src/responder/nss/nsssrv_services.c \
+ src/responder/nss/nsssrv_mmap_cache.c
+nss_srv_multi_tests_CFLAGS = \
+$(AM_CFLAGS)
+nss_srv_multi_tests_LDFLAGS = \
+-Wl,-wrap,sss_dp_get_account_send \
+-Wl,-wrap,sss_ncache_check_user \
+-Wl,-wrap,sss_ncache_check_uid \
+-Wl,-wrap,sss_ncache_check_sid \
+-Wl,-wrap,sss_packet_get_body \
+-Wl,-wrap,sss_packet_get_cmd \
+-Wl,-wrap,sss_cmd_send_empty \
+-Wl,-wrap,sss_cmd_done
+nss_srv_multi_tests_LDADD = \
+$(CMOCKA_LIBS) \
+$(SSSD_LIBS) \
+$(SSSD_INTERNAL_LTLIBS) \
+libsss_test_common.la \
+libsss_idmap.la
+
 EXTRA_nss_srv_tests_DEPENDENCIES = \
  $(ldblib_LTLIBRARIES)
 nss_srv_tests_SOURCES = \
diff --git a/src/tests/cmocka/test_nss_srv_multi.c 
b/src/tests/cmocka/test_nss_srv_multi.c
new file mode 100644
index 
..6320255978b3f8056e0e63d502b9617477d4b042
--- /dev/null
+++ b/src/tests/cmocka/test_nss_srv_multi.c
@@ -0,0 +1,437 @@
+/*
+Authors:
+Jakub Hrozek 
+
+Copyright (C) 2013 Red Hat
+
+SSSD tests: NSS responder tests
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see .
+*/
+
+#include 
+#include 
+#include 
+#include 
+
+#include "tests/cmocka/common_mock.h"
+#include "tests/cmocka/common_mock_resp.h"
+#include "responder/common/negcache.h"
+#include "responder/nss/nsssrv.h"
+#include "responder/nss/nsssrv_private.h"
+#include "sss_client/idmap/sss_nss_idmap.h"
+#include "util/util_sss_idmap.h"
+#include "db/sysdb_private.h" /* new_subdomain() */
+
+#define TESTS_PATH "tests_nss_multi"
+#define TEST_CONF_DB "test_nss_multi_conf.ldb"
+#define TEST_DOM_NAME "nss_test"
+#define TEST_DOM_NAME1 "nss_test_a"
+#define TEST_DOM_NAME2 "nss_test_b"
+#define TEST_SUBDOM_NAME "test.subdomain"
+#define TEST_ID_PROVIDER "ldap"
+#define TEST_DOM_SID "S-1-5-21-444379608-1639770488-2995963434"
+
+static const char *domains[] = { TEST_DOM_NAME1, TEST_DOM_NAME2, NULL };
+
+struct nss_test_ctx {
+struct sss_test_ctx *tctx;
+struct sss_domain_info *subdom;
+
+struct resp_ctx *rctx;
+struct cli_ctx *cctx;
+struct sss_cmd_table *nss_cmds;
+struct nss_ctx *nctx;
+
+int ncache_hits;
+};
+
+const char *global_extra_attrs[] = { "phone", "mobile", NULL };
+
+struct nss_test_ctx *nss_test_ctx;
+
+/* Mock NSS structure */
+struct nss_ctx *
+m

Re: [SSSD] [PATCH] NSS: Fix use after free

2015-08-12 Thread Jakub Hrozek 
On Mon, Aug 10, 2015 at 06:38:29AM +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> Use after free can happed if there are two domains and user is not found
> in the first one.
> 
> LS

Would it be possible to write a testcase in the NSS responder test?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] NSS: Fix use after free

2015-08-09 Thread Lukas Slebodnik 
ehlo,

Use after free can happed if there are two domains and user is not found
in the first one.

LS
>From cbd388699c061ad644ead6c21e38dfb9bc0ca30c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Fri, 7 Aug 2015 14:29:45 +0200
Subject: [PATCH] NSS: Fix use after free

It can happed if there are two domains and user is not found
in the first one.

==29279== Invalid read of size 1
==29279==at 0x4C2CBA2: strlen (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29279==by 0x89A7AC4: talloc_strdup (in /usr/lib64/libtalloc.so.2.1.2)
==29279==by 0x11668A: nss_cmd_initgroups_search (nsssrv_cmd.c:4191)
==29279==by 0x118B27: nss_cmd_getby_dp_callback (nsssrv_cmd.c:1208)
==29279==by 0x10F2B4: nsssrv_dp_send_acct_req_done (nsssrv_cmd.c:759)
==29279==by 0x126AFB: sss_dp_internal_get_done (responder_dp.c:802)
==29279==by 0x56EA861: ??? (in /usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x56EDB50: dbus_connection_dispatch (in 
/usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x50721E1: sbus_dispatch (sssd_dbus_connection.c:96)
==29279==by 0x879B22E: tevent_common_loop_timer_delay (tevent_timed.c:341)
==29279==by 0x879C239: epoll_event_loop_once (tevent_epoll.c:911)
==29279==by 0x879A936: std_event_loop_once (tevent_standard.c:114)
==29279==  Address 0xbbad240 is 96 bytes inside a block of size 106 free'd
==29279==at 0x4C2AD17: free (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29279==by 0x89A46E3: _talloc_free (in /usr/lib64/libtalloc.so.2.1.2)
==29279==by 0x116679: nss_cmd_initgroups_search (nsssrv_cmd.c:4190)
==29279==by 0x118B27: nss_cmd_getby_dp_callback (nsssrv_cmd.c:1208)
==29279==by 0x10F2B4: nsssrv_dp_send_acct_req_done (nsssrv_cmd.c:759)
==29279==by 0x126AFB: sss_dp_internal_get_done (responder_dp.c:802)
==29279==by 0x56EA861: ??? (in /usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x56EDB50: dbus_connection_dispatch (in 
/usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x50721E1: sbus_dispatch (sssd_dbus_connection.c:96)
==29279==by 0x879B22E: tevent_common_loop_timer_delay (tevent_timed.c:341)
==29279==by 0x879C239: epoll_event_loop_once (tevent_epoll.c:911)
==29279==by 0x879A936: std_event_loop_once (tevent_standard.c:114)

Resolves:
https://fedorahosted.org/sssd/ticket/2749
---
 src/responder/nss/nsssrv_cmd.c | 6 +++---
 src/responder/nss/nsssrv_private.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 
e754245eac7200c2f667760540a1d04c9203843d..43cdb135c0933117743a97d6a2524326079bf72c
 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4143,7 +4143,7 @@ static int nss_cmd_initgr_send_reply(struct nss_dom_ctx 
*dctx)
 }
 
 ret = fill_initgr(cctx->creq->out, dctx->domain, dctx->res, nctx,
-  dctx->mc_name, cmdctx->name);
+  dctx->mc_name, cmdctx->normalized_name);
 if (ret) {
 return ret;
 }
@@ -4187,14 +4187,14 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx 
*dctx)
 /* make sure to update the dctx if we changed domain */
 dctx->domain = dom;
 
-talloc_free(name);
+talloc_zfree(cmdctx->normalized_name);
 name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
 if (!name) return ENOMEM;
 
 name = sss_reverse_replace_space(cmdctx, name,
  nctx->rctx->override_space);
 /* save name so it can be used in initgr reply */
-cmdctx->name = name;
+cmdctx->normalized_name = name;
 if (name == NULL) {
 DEBUG(SSSDBG_CRIT_FAILURE,
   "sss_reverse_replace_space failed\n");
diff --git a/src/responder/nss/nsssrv_private.h 
b/src/responder/nss/nsssrv_private.h
index 
e5a2486f1fb9a8de39ec90f802f596b2c2f6af7f..72f7b75604567f9b95937018e54ba2d60b771f9b
 100644
--- a/src/responder/nss/nsssrv_private.h
+++ b/src/responder/nss/nsssrv_private.h
@@ -31,6 +31,7 @@ struct nss_cmd_ctx {
 struct cli_ctx *cctx;
 enum sss_cli_command cmd;
 char *name;
+const char *normalized_name;
 bool name_is_upn;
 uint32_t id;
 char *secid;
-- 
2.5.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel