[SSSD] [PATCHES] LDAP: Sanitize group dn before using in filter
Lukas Slebodnik
lslebodn at redhat.com
Mon Sep 21 17:45:26 UTC 2015
On (21/09/15 17:00), Jakub Hrozek wrote:
>On Fri, Sep 18, 2015 at 03:39:50PM +0200, Pavel Březina wrote:
>> On 09/01/2015 11:06 AM, Lukas Slebodnik wrote:
>> >ehlo,
>> >
>> >attached is alternative version for ticket 2744.
>> >The unit test passed but there is a performance improvement.
>> >
>> >If we compare 3rd patch to current master than
>> >there is a 10 less invocation of malloc (an also other functions) per group.
>> >and we saved allocation of 1091 per group.
>> >
>> >There was a comment[1] in old thread about a possibility of double sanitized dn.
>> >I was not able to find such case. I would be glad if someone
>> >can extend unit test which will pass with master and fail with attached patch.
>> >
>> >LS
>> >
>> >[1] https://lists.fedorahosted.org/pipermail/sssd-devel/2015-August/024346.html
>>
>> Looks like I misunderstood your original proposal. This looks indeed better.
>>
>> I see now why double-sanitization is not an issue.
>>
>> Ack.
>
>CI: http://sssd-ci.duckdns.org/logs/job/26/93/summary.html
>
>master:
> * 2cec08a3174bff951c048c57b4b0e4517ad6b7b1
> * 6cb5bad3c8e2f35ca9dce1800a506d626f90c079
> * 3d8b576bf49a79d5776574b96c6ef9535bbc46ac
> * 9c563db822758732b25a3c8c61ffac90a7deffc3
Attached are patches for 1.12 branch
LS
-------------- next part --------------
>From 6500b1a96319f87c5f1469069dcbe6eecd9c4a29 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 1 Sep 2015 06:58:50 +0200
Subject: [PATCH 1/2] LDAP: Sanitize group dn before using in filter
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Each string should be sanitized(rfc4515) before using ldbsearch.
A group dn was not sanitized in the function cleanup_groups.
Resolves:
https://fedorahosted.org/sssd/ticket/2744
Reviewed-by: Pavel Březina <pbrezina at redhat.com>
(cherry picked from commit 6cb5bad3c8e2f35ca9dce1800a506d626f90c079)
---
src/providers/ldap/ldap_id_cleanup.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/ldap_id_cleanup.c b/src/providers/ldap/ldap_id_cleanup.c
index 171c9b0aecd35f3ddabb00e5c260b66981a1d0f4..73e5e6fc509ba09b827cbd88eff3e1a6454918a0 100644
--- a/src/providers/ldap/ldap_id_cleanup.c
+++ b/src/providers/ldap/ldap_id_cleanup.c
@@ -359,6 +359,8 @@ static int cleanup_groups(TALLOC_CTX *memctx,
}
for (i = 0; i < count; i++) {
+ char *sanitized_dn;
+
dn = ldb_dn_get_linearized(msgs[i]->dn);
if (!dn) {
DEBUG(SSSDBG_CRIT_FAILURE, "Cannot linearize DN!\n");
@@ -366,6 +368,15 @@ static int cleanup_groups(TALLOC_CTX *memctx,
goto done;
}
+ /* sanitize dn */
+ ret = sss_filter_sanitize(tmpctx, dn, &sanitized_dn);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "sss_filter_sanitize failed: %s:[%d]\n",
+ sss_strerror(ret), ret);
+ goto done;
+ }
+
posix = ldb_msg_find_attr_as_string(msgs[i], SYSDB_POSIX, NULL);
if (!posix || strcmp(posix, "TRUE") == 0) {
/* Search for users that are members of this group, or
@@ -375,11 +386,14 @@ static int cleanup_groups(TALLOC_CTX *memctx,
gid = (gid_t) ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0);
subfilter = talloc_asprintf(tmpctx, "(&(%s=%s)(|(%s=%s)(%s=%lu)))",
SYSDB_OBJECTCLASS, SYSDB_USER_CLASS,
- SYSDB_MEMBEROF, dn,
+ SYSDB_MEMBEROF, sanitized_dn,
SYSDB_GIDNUM, (long unsigned) gid);
} else {
- subfilter = talloc_asprintf(tmpctx, "(%s=%s)", SYSDB_MEMBEROF, dn);
+ subfilter = talloc_asprintf(tmpctx, "(%s=%s)", SYSDB_MEMBEROF,
+ sanitized_dn);
}
+ talloc_zfree(sanitized_dn);
+
if (!subfilter) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
ret = ENOMEM;
--
2.5.0
-------------- next part --------------
>From 5245ffc751ce4a7ba4e72db9cfd69331d875d87f Mon Sep 17 00:00:00 2001
From: Pavel Reichl <preichl at redhat.com>
Date: Tue, 4 Aug 2015 09:25:08 -0400
Subject: [PATCH 2/2] tests: check special characters in cleanup_groups
Based on commits:
e2e334b2f51118cb14c7391c4e4e44ff247ef638
f02b62138466c876f6e8d6382769105f2e920d96
e0f2a783439fb7d3b85469f34ad6d672abf7e1fa
2cec08a3174bff951c048c57b4b0e4517ad6b7b1
---
Makefile.am | 22 +++
src/tests/cmocka/test_ldap_id_cleanup.c | 315 ++++++++++++++++++++++++++++++++
2 files changed, 337 insertions(+)
create mode 100644 src/tests/cmocka/test_ldap_id_cleanup.c
diff --git a/Makefile.am b/Makefile.am
index ac6a358ea14239781c26e6f2ac02bdeb3007659f..91ad413cfb5a8b4c27dc9414c43b84151dd8eb5b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -212,6 +212,7 @@ if HAVE_CMOCKA
sbus-internal-tests \
sss_sifp-tests \
test_search_bases \
+ test_ldap_id_cleanup \
sdap-tests \
test_sysdb_views \
test_sysdb_utils \
@@ -1969,6 +1970,27 @@ test_search_bases_LDADD = \
libsss_krb5_common.la \
libsss_test_common.la
+test_ldap_id_cleanup_SOURCES = \
+ $(sssd_be_SOURCES) \
+ src/tests/cmocka/test_ldap_id_cleanup.c \
+ src/providers/ldap/ldap_id_cleanup.c \
+ $(NULL)
+test_ldap_id_cleanup_CFLAGS = \
+ $(AM_CFLAGS) \
+ -DUNIT_TESTING
+ $(NULL)
+test_ldap_id_cleanup_LDADD = \
+ $(PAM_LIBS) \
+ $(CMOCKA_LIBS) \
+ $(POPT_LIBS) \
+ $(SSSD_LIBS) \
+ $(CARES_LIBS) \
+ $(KRB5_LIBS) \
+ $(SSSD_INTERNAL_LTLIBS) \
+ libsss_ldap_common.la \
+ libsss_test_common.la \
+ $(NULL)
+
ad_access_filter_tests_SOURCES = \
$(sssd_be_SOURCES) \
src/providers/ad/ad_common.c \
diff --git a/src/tests/cmocka/test_ldap_id_cleanup.c b/src/tests/cmocka/test_ldap_id_cleanup.c
new file mode 100644
index 0000000000000000000000000000000000000000..9578bb7d62d1a88b575ae03923fdfe532b1abf55
--- /dev/null
+++ b/src/tests/cmocka/test_ldap_id_cleanup.c
@@ -0,0 +1,315 @@
+/*
+ Authors:
+ Pavel Reichl <preichl at redhat.com>
+
+ Copyright (C) 2015 Red Hat
+
+ SSSD tests - id cleanup
+
+ 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 <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdarg.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <cmocka.h>
+#include <popt.h>
+
+#include "tests/cmocka/common_mock.h"
+#include "providers/ldap/ldap_auth.h"
+#include "providers/ldap/ldap_common.h"
+#include "providers/ldap/ldap_opts.h"
+#include "providers/ipa/ipa_opts.h"
+
+#define TESTS_PATH "tests_ldap_id_cleanup"
+#define TEST_CONF_FILE "tests_conf.ldb"
+
+struct sysdb_test_ctx {
+ struct sysdb_ctx *sysdb;
+ struct confdb_ctx *confdb;
+ struct tevent_context *ev;
+ struct sss_domain_info *domain;
+ struct sdap_options *opts;
+};
+
+static int _setup_sysdb_tests(struct sysdb_test_ctx **ctx, bool enumerate)
+{
+ struct sysdb_test_ctx *test_ctx;
+ char *conf_db;
+ int ret;
+
+ const char *val[2];
+ val[1] = NULL;
+
+ /* Create tests directory if it doesn't exist */
+ /* (relative to current dir) */
+ ret = mkdir(TESTS_PATH, 0775);
+ assert_true(ret == 0 || errno == EEXIST);
+
+ test_ctx = talloc_zero(global_talloc_context, struct sysdb_test_ctx);
+ assert_non_null(test_ctx);
+
+ /* Create an event context
+ * It will not be used except in confdb_init and sysdb_init
+ */
+ test_ctx->ev = tevent_context_init(test_ctx);
+ assert_non_null(test_ctx->ev);
+
+ conf_db = talloc_asprintf(test_ctx, "%s/%s", TESTS_PATH, TEST_CONF_FILE);
+ assert_non_null(conf_db);
+ DEBUG(SSSDBG_MINOR_FAILURE, "CONFDB: %s\n", conf_db);
+
+ /* Connect to the conf db */
+ ret = confdb_init(test_ctx, &test_ctx->confdb, conf_db);
+ assert_int_equal(ret, EOK);
+
+ val[0] = "LOCAL";
+ ret = confdb_add_param(test_ctx->confdb, true,
+ "config/sssd", "domains", val);
+ assert_int_equal(ret, EOK);
+
+ val[0] = "local";
+ ret = confdb_add_param(test_ctx->confdb, true,
+ "config/domain/LOCAL", "id_provider", val);
+ assert_int_equal(ret, EOK);
+
+ val[0] = enumerate ? "TRUE" : "FALSE";
+ ret = confdb_add_param(test_ctx->confdb, true,
+ "config/domain/LOCAL", "enumerate", val);
+ assert_int_equal(ret, EOK);
+
+ val[0] = "TRUE";
+ ret = confdb_add_param(test_ctx->confdb, true,
+ "config/domain/LOCAL", "cache_credentials", val);
+ assert_int_equal(ret, EOK);
+
+ ret = sssd_domain_init(test_ctx, test_ctx->confdb, "local",
+ TESTS_PATH, &test_ctx->domain);
+ assert_int_equal(ret, EOK);
+
+ test_ctx->domain->has_views = true;
+ test_ctx->sysdb = test_ctx->domain->sysdb;
+
+ *ctx = test_ctx;
+ return EOK;
+}
+
+#define setup_sysdb_tests(ctx) _setup_sysdb_tests((ctx), false)
+
+static int test_sysdb_setup(void **state)
+{
+ int ret;
+ struct sysdb_test_ctx *test_ctx;
+
+ assert_true(leak_check_setup());
+
+ ret = setup_sysdb_tests(&test_ctx);
+ assert_int_equal(ret, EOK);
+
+ test_ctx->domain->mpg = false;
+
+ /* set options */
+ test_ctx->opts = talloc_zero(test_ctx, struct sdap_options);
+ assert_non_null(test_ctx->opts);
+
+ ret = sdap_copy_map(test_ctx->opts, rfc2307_user_map,
+ SDAP_OPTS_USER, &test_ctx->opts->user_map);
+ assert_int_equal(ret, ERR_OK);
+
+ ret = dp_copy_defaults(test_ctx->opts, default_basic_opts,
+ SDAP_OPTS_BASIC, &test_ctx->opts->basic);
+ assert_int_equal(ret, ERR_OK);
+
+ dp_opt_set_int(test_ctx->opts->basic, SDAP_ACCOUNT_CACHE_EXPIRATION, 1);
+
+ *state = (void *) test_ctx;
+ return 0;
+}
+
+static int test_sysdb_teardown(void **state)
+{
+ struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
+ struct sysdb_test_ctx);
+
+ talloc_free(test_ctx);
+ assert_true(leak_check_teardown());
+ return 0;
+}
+
+static errno_t invalidate_group(TALLOC_CTX *ctx,
+ struct sss_domain_info *domain,
+ const char *name)
+{
+ struct sysdb_attrs *sys_attrs = NULL;
+ errno_t ret;
+
+ sys_attrs = sysdb_new_attrs(ctx);
+ if (sys_attrs) {
+ ret = sysdb_attrs_add_time_t(sys_attrs,
+ SYSDB_CACHE_EXPIRE, 1);
+ if (ret == EOK) {
+ ret = sysdb_set_group_attr(domain, name, sys_attrs,
+ SYSDB_MOD_REP);
+ } else {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Could not add expiration time to attributes\n");
+ }
+ talloc_zfree(sys_attrs);
+ } else {
+ DEBUG(SSSDBG_MINOR_FAILURE, "Could not create sysdb attributes\n");
+ ret = ENOMEM;
+ }
+ return ret;
+}
+
+static void test_id_cleanup_exp_group(void **state)
+{
+ errno_t ret;
+ struct ldb_message *msg;
+ struct sdap_domain sdom;
+ const char *special_grp = "special_gr*o/u\\p(2016)";
+ const char *empty_special_grp = "empty_gr*o/u\\p(2016)";
+ const char *empty_grp = "empty_grp";
+ const char *grp = "grp";
+ /* This timeout can be bigger because we will call invalidate_group
+ * to expire entries without waiting. */
+ const uint64_t CACHE_TIMEOUT = 30;
+ struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
+ struct sysdb_test_ctx);
+
+ ret = sysdb_store_group(test_ctx->domain, special_grp,
+ 10002, NULL, CACHE_TIMEOUT, 0);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_store_group(test_ctx->domain, empty_special_grp,
+ 10003, NULL, CACHE_TIMEOUT, 0);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_store_group(test_ctx->domain, grp,
+ 10004, NULL, CACHE_TIMEOUT, 0);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_store_group(test_ctx->domain, empty_grp,
+ 10005, NULL, CACHE_TIMEOUT, 0);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_store_user(test_ctx->domain, "test_user", NULL,
+ 10001, 10002, "Test user",
+ NULL, NULL, NULL, NULL, NULL,
+ 0, 0);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_store_user(test_ctx->domain, "test_user2", NULL,
+ 10002, 10004, "Test user",
+ NULL, NULL, NULL, NULL, NULL,
+ 0, 0);
+ assert_int_equal(ret, EOK);
+
+ sdom.dom = test_ctx->domain;
+
+ /* not expired */
+ ret = ldap_id_cleanup(test_ctx->opts, &sdom);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ special_grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ empty_special_grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ empty_grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ /* let records to expire */
+ invalidate_group(test_ctx, test_ctx->domain, special_grp);
+ invalidate_group(test_ctx, test_ctx->domain, empty_special_grp);
+ invalidate_group(test_ctx, test_ctx->domain, grp);
+ invalidate_group(test_ctx, test_ctx->domain, empty_grp);
+
+ ret = ldap_id_cleanup(test_ctx->opts, &sdom);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ special_grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ empty_special_grp, NULL, &msg);
+ assert_int_equal(ret, ENOENT);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ grp, NULL, &msg);
+ assert_int_equal(ret, EOK);
+
+ ret = sysdb_search_group_by_name(test_ctx, test_ctx->domain,
+ empty_grp, NULL, &msg);
+ assert_int_equal(ret, ENOENT);
+}
+
+int main(int argc, const char *argv[])
+{
+ int rv;
+ int no_cleanup = 0;
+ poptContext pc;
+ int opt;
+ struct poptOption long_options[] = {
+ POPT_AUTOHELP
+ SSSD_DEBUG_OPTS
+ { "no-cleanup", 'n', POPT_ARG_NONE, &no_cleanup, 0,
+ _("Do not delete the test database after a test run"), NULL },
+ POPT_TABLEEND
+ };
+
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test_setup_teardown(test_id_cleanup_exp_group,
+ test_sysdb_setup, test_sysdb_teardown),
+ };
+
+ /* Set debug level to invalid value so we can deside if -d 0 was used. */
+ debug_level = SSSDBG_INVALID;
+
+ pc = poptGetContext(argv[0], argc, argv, long_options, 0);
+ while ((opt = poptGetNextOpt(pc)) != -1) {
+ switch (opt) {
+ default:
+ fprintf(stderr, "\nInvalid option %s: %s\n\n",
+ poptBadOption(pc, 0), poptStrerror(opt));
+ poptPrintUsage(pc, stderr, 0);
+ return 1;
+ }
+ }
+ poptFreeContext(pc);
+
+ DEBUG_CLI_INIT(debug_level);
+
+ tests_set_cwd();
+ test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_FILE, LOCAL_SYSDB_FILE);
+ test_dom_suite_setup(TESTS_PATH);
+ rv = cmocka_run_group_tests(tests, NULL, NULL);
+
+ if (rv == 0 && no_cleanup == 0) {
+ test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_FILE, LOCAL_SYSDB_FILE);
+ }
+ return rv;
+}
--
2.5.0
More information about the sssd-devel
mailing list