[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