URL: https://github.com/SSSD/sssd/pull/5262 Author: elkoniu Title: #5262: DN with white spaces Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/5262/head:pr5262 git checkout pr5262
From 882307cdc1b596ba0cc346a0001f4fc014818d82 Mon Sep 17 00:00:00 2001 From: Tomas Halman <thal...@redhat.com> Date: Fri, 31 Jul 2020 11:12:02 +0200 Subject: [PATCH 1/4] UTIL: DN sanitization Some of the ldap servers returns DN in attributes such as isMemberOf with spaces like dc=example, dc=com. That should be fine and we should ignore them (cut them out) instead of escaping. Resolves: https://github.com/SSSD/sssd/issues/5261 --- src/tests/cmocka/test_utils.c | 70 +++++++++++++++++++ src/util/util.h | 20 ++++++ src/util/util_ext.c | 126 ++++++++++++++++++++++++++++++++++ 3 files changed, 216 insertions(+) diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c index c5eda4dd26..92ca783d9f 100644 --- a/src/tests/cmocka/test_utils.c +++ b/src/tests/cmocka/test_utils.c @@ -1955,6 +1955,73 @@ static void test_sss_get_domain_mappings_content(void **state) * capaths might not be as expected. */ } + +static void test_sss_filter_sanitize_dn(void **state) +{ + TALLOC_CTX *tmp_ctx; + char *trimmed; + int ret; + const char *DN = "cn=user,ou=people,dc=example,dc=com"; + + tmp_ctx = talloc_new(NULL); + assert_non_null(tmp_ctx); + + /* test that we remove spaces around '=' and ','*/ + ret = sss_filter_sanitize_dn(tmp_ctx, DN, &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, "cn=user,ou=people,dc=example,dc=com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, "cn= user,ou =people,dc = example,dc = com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, "cn=user, ou=people ,dc=example , dc=com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, "cn=user, ou=people ,dc=example , dc=com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, "cn= user, ou =people ,dc = example , dc = com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, " cn=user,ou=people,dc=example,dc=com ", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + ret = sss_filter_sanitize_dn(tmp_ctx, " cn=user, ou=people, dc=example, dc=com ", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal(DN, trimmed); + talloc_free(trimmed); + + /* test that we keep spaces inside a value */ + ret = sss_filter_sanitize_dn(tmp_ctx, "cn = user one, ou=people branch, dc=example, dc=com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal("cn=user\\20one,ou=people\\20\\20branch,dc=example,dc=com", trimmed); + talloc_free(trimmed); + + /* test that we keep escape special chars like () */ + ret = sss_filter_sanitize_dn(tmp_ctx, "cn = user one, ou=p(e)ople, dc=example, dc=com", &trimmed); + assert_int_equal(ret, EOK); + assert_string_equal("cn=user\\20one,ou=p\\28e\\29ople,dc=example,dc=com", trimmed); + talloc_free(trimmed); + + talloc_free(tmp_ctx); +} + int main(int argc, const char *argv[]) { poptContext pc; @@ -2064,6 +2131,9 @@ int main(int argc, const char *argv[]) cmocka_unit_test_setup_teardown(test_sss_ptr_hash_without_cb, setup_leak_tests, teardown_leak_tests), + cmocka_unit_test_setup_teardown(test_sss_filter_sanitize_dn, + setup_leak_tests, + teardown_leak_tests), }; /* Set debug level to invalid value so we can decide if -d 0 was used. */ diff --git a/src/util/util.h b/src/util/util.h index d538e0674d..aa9bf97d4c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -478,6 +478,26 @@ errno_t sss_filter_sanitize_for_dom(TALLOC_CTX *mem_ctx, char **sanitized, char **lc_sanitized); +/* Sanitize an input string (e.g. a DN) for use in + * an LDAP/LDB filter + * + * It is basically the same as sss_filter_sanitize(_ex), + * just extra spaces inside DN around '=' and ',' are removed + * before sanitizing other characters . According the documentation + * spaces in DN are allowed and some ldap servers can return them + * in isMemberOf or member attributes. + * + * (dc = my example, dc = com => dc=my\20example,dc=com) + * + * Returns a newly-constructed string attached to mem_ctx + * It will fail only on an out of memory condition, where it + * will return ENOMEM. + * + */ +errno_t sss_filter_sanitize_dn(TALLOC_CTX *mem_ctx, + const char *input, + char **sanitized); + char * sss_escape_ip_address(TALLOC_CTX *mem_ctx, int family, const char *addr); diff --git a/src/util/util_ext.c b/src/util/util_ext.c index a89b60f761..3ce0371235 100644 --- a/src/util/util_ext.c +++ b/src/util/util_ext.c @@ -240,3 +240,129 @@ errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, { return sss_filter_sanitize_ex(mem_ctx, input, sanitized, NULL); } + +/* There is similar function ldap_dn_normalize in openldap. + * To avoid dependecies across project we have this own func. + * Also ldb can do this but doesn't handle all the cases + */ +static errno_t sss_trim_dn(TALLOC_CTX *mem_ctx, + const char *input, + char **trimmed) +{ + int i = 0; + int o = 0; + int s; + char *output; + enum sss_trim_dn_state { + SSS_TRIM_DN_STATE_READING_NAME, + SSS_TRIM_DN_STATE_READING_VALUE + } state = SSS_TRIM_DN_STATE_READING_NAME; + + *trimmed = NULL; + + output = talloc_array(mem_ctx, char, strlen(input) + 1); + if (!output) { + return ENOMEM; + } + + /* skip leading spaces */ + while(isspace(input[i])) { + ++i; + } + + while(input[i] != '\0') { + if (!isspace(input[i])) { + switch (input[i]) { + case '=': + output[o++] = input[i++]; + if (state == SSS_TRIM_DN_STATE_READING_NAME) { + while (isspace(input[i])) { + ++i; + } + state = SSS_TRIM_DN_STATE_READING_VALUE; + } + break; + case ',': + output[o++] = input[i++]; + if (state == SSS_TRIM_DN_STATE_READING_VALUE) { + while (isspace(input[i])) { + ++i; + } + state = SSS_TRIM_DN_STATE_READING_NAME; + } + break; + case '\\': + output[o++] = input[i++]; + if (input[i] != '\0') { + output[o++] = input[i++]; + } + break; + default: + if (input[i] != '\0') { + output[o++] = input[i++]; + } + break; + } + + continue; + } + + /* non escaped space found */ + s = 1; + while (isspace(input[i + s])) { + ++s; + } + + switch (state) { + case SSS_TRIM_DN_STATE_READING_NAME: + if (input[i + s] != '=') { + /* this is not trailing space - should not be removed */ + while (isspace(input[i])) { + output[o++] = input[i++]; + } + } else { + i += s; + } + break; + case SSS_TRIM_DN_STATE_READING_VALUE: + if (input[i + s] != ',') { + /* this is not trailing space - should not be removed */ + while (isspace(input[i])) { + output[o++] = input[i++]; + } + } else { + i += s; + } + break; + } + } + + output[o--] = '\0'; + + /* trim trailing space */ + while (o >= 0 && isspace(output[o])) { + output[o--] = '\0'; + } + + *trimmed = output; + return EOK; +} + +errno_t sss_filter_sanitize_dn(TALLOC_CTX *mem_ctx, + const char *input, + char **sanitized) +{ + errno_t ret; + char *trimmed_dn = NULL; + + ret = sss_trim_dn(mem_ctx, input, &trimmed_dn); + if (ret != EOK) { + goto done; + } + + ret = sss_filter_sanitize_ex(mem_ctx, trimmed_dn, sanitized, NULL); + + done: + talloc_free(trimmed_dn); + return ret; +} From 2635e1538a1ef8c01a6587ef3f28ab3367e3459f Mon Sep 17 00:00:00 2001 From: Tomas Halman <thal...@redhat.com> Date: Fri, 31 Jul 2020 11:21:44 +0200 Subject: [PATCH 2/4] UTIL: Use sss_sanitize_dn where we deal with DN Resolves: https://github.com/SSSD/sssd/issues/5261 --- src/db/sysdb_ops.c | 2 +- src/providers/ipa/ipa_deskprofile_rules.c | 2 +- src/providers/ipa/ipa_hbac_rules.c | 2 +- src/providers/ipa/ipa_netgroups.c | 2 +- src/providers/ldap/sdap_async_groups.c | 2 +- src/providers/ldap/sdap_async_groups_ad.c | 2 +- src/providers/ldap/sdap_async_initgroups.c | 4 ++-- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index a00e022b88..c60c082397 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -3514,7 +3514,7 @@ errno_t sysdb_search_by_orig_dn(TALLOC_CTX *mem_ctx, return ENOMEM; } - ret = sss_filter_sanitize(tmp_ctx, member_dn, &sanitized_dn); + ret = sss_filter_sanitize_dn(tmp_ctx, member_dn, &sanitized_dn); if (ret != EOK) { goto done; } diff --git a/src/providers/ipa/ipa_deskprofile_rules.c b/src/providers/ipa/ipa_deskprofile_rules.c index 65994356e8..cce6184db6 100644 --- a/src/providers/ipa/ipa_deskprofile_rules.c +++ b/src/providers/ipa/ipa_deskprofile_rules.c @@ -91,7 +91,7 @@ ipa_deskprofile_rule_info_send(TALLOC_CTX *mem_ctx, goto immediate; } - ret = sss_filter_sanitize(state, host_dn, &host_dn_clean); + ret = sss_filter_sanitize_dn(state, host_dn, &host_dn_clean); if (ret != EOK) { goto immediate; } diff --git a/src/providers/ipa/ipa_hbac_rules.c b/src/providers/ipa/ipa_hbac_rules.c index 0634a277e2..e2c97ae3db 100644 --- a/src/providers/ipa/ipa_hbac_rules.c +++ b/src/providers/ipa/ipa_hbac_rules.c @@ -84,7 +84,7 @@ ipa_hbac_rule_info_send(TALLOC_CTX *mem_ctx, goto immediate; } - ret = sss_filter_sanitize(state, host_dn, &host_dn_clean); + ret = sss_filter_sanitize_dn(state, host_dn, &host_dn_clean); if (ret != EOK) goto immediate; state->ev = ev; diff --git a/src/providers/ipa/ipa_netgroups.c b/src/providers/ipa/ipa_netgroups.c index 360c803ed5..8d3ebf194f 100644 --- a/src/providers/ipa/ipa_netgroups.c +++ b/src/providers/ipa/ipa_netgroups.c @@ -375,7 +375,7 @@ static void ipa_get_netgroups_process(struct tevent_req *subreq) continue; } - ret = sss_filter_sanitize(state, orig_dn, &dn); + ret = sss_filter_sanitize_dn(state, orig_dn, &dn); if (ret != EOK) { goto done; } diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index d7ace9485d..c4443e9b3a 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -52,7 +52,7 @@ static int sdap_find_entry_by_origDN(TALLOC_CTX *memctx, return ENOMEM; } - ret = sss_filter_sanitize(tmpctx, orig_dn, &sanitized_dn); + ret = sss_filter_sanitize_dn(tmpctx, orig_dn, &sanitized_dn); if (ret != EOK) { ret = ENOMEM; goto done; diff --git a/src/providers/ldap/sdap_async_groups_ad.c b/src/providers/ldap/sdap_async_groups_ad.c index 3f842b26da..c954398bbc 100644 --- a/src/providers/ldap/sdap_async_groups_ad.c +++ b/src/providers/ldap/sdap_async_groups_ad.c @@ -91,7 +91,7 @@ sdap_get_ad_match_rule_members_send(TALLOC_CTX *mem_ctx, } /* Sanitize it in case we have special characters in DN */ - ret = sss_filter_sanitize(state, group_dn, &sanitized_group_dn); + ret = sss_filter_sanitize_dn(state, group_dn, &sanitized_group_dn); if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not sanitize group DN: %s\n", diff --git a/src/providers/ldap/sdap_async_initgroups.c b/src/providers/ldap/sdap_async_initgroups.c index b2f285690d..4b5b36403d 100644 --- a/src/providers/ldap/sdap_async_initgroups.c +++ b/src/providers/ldap/sdap_async_initgroups.c @@ -1647,7 +1647,7 @@ static struct tevent_req *sdap_initgr_rfc2307bis_send( attr_filter, &state->attrs, NULL); if (ret != EOK) goto done; - ret = sss_filter_sanitize(state, orig_dn, &clean_orig_dn); + ret = sss_filter_sanitize_dn(state, orig_dn, &clean_orig_dn); if (ret != EOK) goto done; use_id_mapping = sdap_idmap_domain_has_algorithmic_mapping( @@ -2429,7 +2429,7 @@ static errno_t rfc2307bis_nested_groups_step(struct tevent_req *req) goto done; } - ret = sss_filter_sanitize(tmp_ctx, state->orig_dn, &clean_orig_dn); + ret = sss_filter_sanitize_dn(tmp_ctx, state->orig_dn, &clean_orig_dn); if (ret != EOK) { goto done; } From 12bbd26e6c551d59793ba9a02a1d7cae4062f189 Mon Sep 17 00:00:00 2001 From: Tomas Halman <thal...@redhat.com> Date: Wed, 19 Aug 2020 15:17:44 +0200 Subject: [PATCH 3/4] UTIL: Use sss_sanitize_dn where we deal with DN 2 Tests show that also ldb_dn_get_linearized can return DN with extra spaces. We have to trim that too. Resolves: https://github.com/SSSD/sssd/issues/5261 --- src/ldb_modules/memberof.c | 6 +++--- src/providers/ldap/ldap_id_cleanup.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c index dae51938b7..5de3b7c3b5 100644 --- a/src/ldb_modules/memberof.c +++ b/src/ldb_modules/memberof.c @@ -1364,7 +1364,7 @@ static int memberof_del(struct ldb_module *module, struct ldb_request *req) return LDB_ERR_OPERATIONS_ERROR; } - sret = sss_filter_sanitize(del_ctx, dn, &clean_dn); + sret = sss_filter_sanitize_dn(del_ctx, dn, &clean_dn); if (sret != 0) { talloc_free(ctx); return LDB_ERR_OPERATIONS_ERROR; @@ -1781,7 +1781,7 @@ static int mbof_del_execute_op(struct mbof_del_operation *delop) return LDB_ERR_OPERATIONS_ERROR; } - ret = sss_filter_sanitize(del_ctx, dn, &clean_dn); + ret = sss_filter_sanitize_dn(del_ctx, dn, &clean_dn); if (ret != 0) { return LDB_ERR_OPERATIONS_ERROR; } @@ -3054,7 +3054,7 @@ static int mbof_get_ghost_from_parent(struct mbof_mod_del_op *igh) return LDB_ERR_OPERATIONS_ERROR; } - ret = sss_filter_sanitize(igh, dn, &clean_dn); + ret = sss_filter_sanitize_dn(igh, dn, &clean_dn); if (ret != 0) { return LDB_ERR_OPERATIONS_ERROR; } diff --git a/src/providers/ldap/ldap_id_cleanup.c b/src/providers/ldap/ldap_id_cleanup.c index b6aece9989..ce7cf7f162 100644 --- a/src/providers/ldap/ldap_id_cleanup.c +++ b/src/providers/ldap/ldap_id_cleanup.c @@ -443,7 +443,7 @@ static int cleanup_groups(TALLOC_CTX *memctx, } /* sanitize dn */ - ret = sss_filter_sanitize(tmpctx, dn, &sanitized_dn); + ret = sss_filter_sanitize_dn(tmpctx, dn, &sanitized_dn); if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "sss_filter_sanitize failed: %s:[%d]\n", From 619a8889cb46696ce6ba6c23f49ac9e8935760ad Mon Sep 17 00:00:00 2001 From: Tomas Halman <thal...@redhat.com> Date: Thu, 20 Aug 2020 16:26:36 +0200 Subject: [PATCH 4/4] CACHE: trim spaces in DN before hash lookup DN must be trimmed, before it is used as hash key Resolves: https://github.com/SSSD/sssd/issues/5261 --- src/providers/ldap/sdap_async_groups.c | 15 +++++++++++++-- src/util/util.h | 13 +++++++++++++ src/util/util_ext.c | 6 +++--- 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/providers/ldap/sdap_async_groups.c b/src/providers/ldap/sdap_async_groups.c index c4443e9b3a..e7c7d4aef5 100644 --- a/src/providers/ldap/sdap_async_groups.c +++ b/src/providers/ldap/sdap_async_groups.c @@ -275,8 +275,14 @@ static int sdap_fill_memberships(struct sdap_options *opts, hret = HASH_ERROR_KEY_NOT_FOUND; } else { key.type = HASH_KEY_STRING; - key.str = (char *)values[i].data; + ret = sss_trim_dn(ctx, (char *)values[i].data, &key.str); + if (ret != EOK) { + ret = ENOMEM; + goto done; + } + hret = hash_lookup(ghosts, &key, &value); + talloc_free(key.str); } if (hret == HASH_ERROR_KEY_NOT_FOUND) { @@ -465,8 +471,13 @@ sdap_process_ghost_members(struct sysdb_attrs *attrs, for (i = 0; i < memberel->num_values; i++) { key.type = HASH_KEY_STRING; - key.str = (char *) memberel->values[i].data; + ret = sss_trim_dn(sysdb_attrs, (char *) memberel->values[i].data, &key.str); + if (ret != EOK) { + return ENOMEM; + } + hret = hash_lookup(ghosts, &key, &value); + talloc_free(key.str); if (hret == HASH_ERROR_KEY_NOT_FOUND) { continue; } else if (hret != HASH_SUCCESS) { diff --git a/src/util/util.h b/src/util/util.h index aa9bf97d4c..ad074d09f7 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -478,6 +478,19 @@ errno_t sss_filter_sanitize_for_dom(TALLOC_CTX *mem_ctx, char **sanitized, char **lc_sanitized); + +/* trim extra spaces from DN + * + * Extra spaces inside DN around '=' and ',' are removed + * + * (dc = my example, dc = com => dc=my example,dc=com) + * + * Returns EOK or ENOMEM. + */ +errno_t sss_trim_dn(TALLOC_CTX *mem_ctx, + const char *input, + char **trimmed); + /* Sanitize an input string (e.g. a DN) for use in * an LDAP/LDB filter * diff --git a/src/util/util_ext.c b/src/util/util_ext.c index 3ce0371235..91f5f82752 100644 --- a/src/util/util_ext.c +++ b/src/util/util_ext.c @@ -245,9 +245,9 @@ errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, * To avoid dependecies across project we have this own func. * Also ldb can do this but doesn't handle all the cases */ -static errno_t sss_trim_dn(TALLOC_CTX *mem_ctx, - const char *input, - char **trimmed) +errno_t sss_trim_dn(TALLOC_CTX *mem_ctx, + const char *input, + char **trimmed) { int i = 0; int o = 0;
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org
