[SSSD] [PATCHES] Sanitize search filters in the sysdb
Sumit Bose
sbose at redhat.com
Wed Nov 10 15:07:14 UTC 2010
On Fri, Nov 05, 2010 at 11:35:52AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/28/2010 09:11 PM, Stephen Gallagher wrote:
> > On 10/28/2010 08:50 PM, Stephen Gallagher wrote:
> >> This is the first part of a fix for https://fedorahosted.org/sssd/ticket/639
> >
> >> These patches address the various search filters we're using in the
> >> sysdb. Patches for LDAP search filters will be coming later. I wanted to
> >> get these reviewed first.
> >
>
>
> Patch 0001: Add utility function to sanitize LDAP/LDB filters. Contains
> a unit test.
>
> Patch 0002: Add sysdb utility function for sanitizing DN. No unit test,
> since we're relying on an opaque function from libldb.
>
> Patch 0003: Use sss_filter_sanitize() for all ldb searches internal to
> the sysdb
>
> Patch 0004: Use sss_filter_sanitize() for all ldb searches in the IPA
> provider
>
> Patch 0005: Use sss_filter_sanitize() for all ldb searches in the LDAP
> provider
>
> Patch 0006: Use sysdb_dn_sanitize() in the DN helpers
>
> Patch 0007: Sanitize search filters in the memberOf plugin
>
> Patch 0008: Sanitize a DN lookup for memberOf in initgroups processing
>
> Patch 0009: Add unit tests for users/groups/netgroups with odd characters
>
I have only minor comment, please find them below ...
bye,
Sumit
>
> - --
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkzUJFcACgkQeiVVYja6o6OqzACePcPef36XeMXYojTwqKAnnBKj
> dFQAnR87P/W/wCoaMk9C4RtKiyE0vxuS
> =ndLg
> -----END PGP SIGNATURE-----
> diff --git a/src/util/util.c b/src/util/util.c
> index 06eea283770601997e3ff90df73db58a4fb25bba..20b8c78a72611335be77d36e7b3dc420f26989d2 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -515,3 +515,55 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx,
...
> + }
> + output[j] = '\0';
> + *sanitized = talloc_realloc(mem_ctx, output, char, strlen(output)+1);
I think it is not necessary to call talloc_realloc() and if you really
want to call it you can use j instead of strlen(output).
...
> From 1516feab46b282ada2f3ed1d808f259474b942dc Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Thu, 28 Oct 2010 20:28:59 -0400
> Subject: [PATCH 4/9] Sanitize sysdb search filters in the IPA provider
>
> ---
> src/providers/ipa/ipa_access.c | 17 +++++++++++++++--
> 1 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
> index 979959fb12f6383edd86ce2405de217462b9a5a6..c22f75efb0509c11f0073b6a5925730c1e42ee8d 100644
> --- a/src/providers/ipa/ipa_access.c
> +++ b/src/providers/ipa/ipa_access.c
> @@ -1084,12 +1092,17 @@ static struct tevent_req *hbac_get_rules_send(TALLOC_CTX *memctx,
> state->hbac_attrs[16] = SYSDB_ORIG_DN;
> state->hbac_attrs[17] = NULL;
>
> + ret = sss_filter_sanitize(state, host_dn, &host_dn_clean);
> + if (ret != EOK) {
> + goto fail;
> + }
> +
> state->hbac_filter = talloc_asprintf(state,
> "(&(objectclass=ipaHBACRule)"
> "(%s=%s)(|(%s=%s)(%s=%s)",
> IPA_ENABLED_FLAG, IPA_TRUE_VALUE,
> IPA_HOST_CATEGORY, "all",
> - IPA_MEMBER_HOST, host_dn);
> + IPA_MEMBER_HOST, host_dn_clean);
> if (state->hbac_filter == NULL) {
> ret = ENOMEM;
> goto fail;
talloc_free(host_dn_clean) ?
...
>
> diff --git a/src/providers/ldap/sdap_async_accounts.c b/src/providers/ldap/sdap_async_accounts.c
> index 6b14161cefb8a272edea34ac117550e5d53139a0..426bdf44e5072c19eb97aee03d3731fbf1dfebc9 100644
> --- a/src/providers/ldap/sdap_async_accounts.c
> +++ b/src/providers/ldap/sdap_async_accounts.c
> @@ -2590,6 +2590,7 @@ static errno_t sdap_nested_group_process_step(struct tevent_req *req)
> errno_t ret;
> struct sdap_nested_group_ctx *state =
> tevent_req_data(req, struct sdap_nested_group_ctx);
> + char *member_dn;
> char *filter;
> static const char *attrs[] = SYSDB_PW_ATTRS;
> size_t count;
> @@ -2636,10 +2637,15 @@ static errno_t sdap_nested_group_process_step(struct tevent_req *req)
>
> } while (has_key);
>
> + ret = sss_filter_sanitize(state, state->member_dn, &member_dn);
> + if (ret != EOK) {
> + goto error;
> + }
> +
> /* Check for the specified origDN in the sysdb */
> filter = talloc_asprintf(NULL, "(%s=%s)",
> SYSDB_ORIG_DN,
> - state->member_dn);
> + member_dn);
> if (!filter) {
> ret = ENOMEM;
> goto error;
> @@ -2657,7 +2663,7 @@ static errno_t sdap_nested_group_process_step(struct tevent_req *req)
>
> filter = talloc_asprintf(NULL, "(%s=%s)",
> SYSDB_ORIG_DN,
> - state->member_dn);
> + member_dn);
> if (!filter) {
> ret = ENOMEM;
> goto error;
talloc_free(member_dn) ?
...
> From 52934f59cdef9a9d90b253e323a2c914e22fb1f7 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Fri, 5 Nov 2010 11:15:42 -0400
> Subject: [PATCH 6/9] Sanitize sysdb DN helpers
>
> ---
> src/db/sysdb.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 90 insertions(+), 7 deletions(-)
>
> diff --git a/src/db/sysdb.c b/src/db/sysdb.c
> index b2691526a4a02874f9784c2c41a63c8be237bc0c..71d4f8490a4cf3d68a85ae0a71836ff8e8d50b1d 100644
> --- a/src/db/sysdb.c
> +++ b/src/db/sysdb.c
> @@ -54,33 +54,116 @@ struct ldb_dn *sysdb_custom_subtree_dn(struct sysdb_ctx *ctx, void *memctx,
> const char *domain,
> const char *subtree_name)
> {
> - return ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_CUSTOM_SUBTREE,
> - subtree_name, domain);
> + errno_t ret;
> + TALLOC_CTX *tmp_ctx;
> + char *clean_subtree;
> + struct ldb_dn *dn = NULL;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (!tmp_ctx) {
> + return NULL;
> + }
> +
> + ret = sysdb_dn_sanitize(tmp_ctx, subtree_name, &clean_subtree);
> + if (ret != EOK) {
> + goto done;
> + }
> +
> + dn = ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_CUSTOM_SUBTREE,
> + clean_subtree, domain);
> +
> +done:
> + talloc_free(tmp_ctx);
> + return dn;
> }
> struct ldb_dn *sysdb_custom_dn(struct sysdb_ctx *ctx, void *memctx,
> const char *domain, const char *object_name,
> const char *subtree_name)
> {
> - return ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_CUSTOM, object_name,
> - subtree_name, domain);
> + errno_t ret;
> + TALLOC_CTX *tmp_ctx;
> + char *clean_name;
> + char *clean_subtree;
> + struct ldb_dn *dn = NULL;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (!tmp_ctx) {
> + return NULL;
> + }
> +
> + ret = sysdb_dn_sanitize(tmp_ctx, object_name, &clean_name);
> + if (ret != EOK) {
> + goto done;
> + }
> +
> + ret = sysdb_dn_sanitize(tmp_ctx, subtree_name, &clean_subtree);
> + if (ret != EOK) {
> + goto done;
> + }
> +
> + dn = ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_CUSTOM, clean_name,
> + clean_subtree, domain);
> +
> +done:
> + talloc_free(tmp_ctx);
> + return dn;
> }
>
Why do you create a tmp_ctx on the two cases above and just use NULL in
the following three cases? I think using NULL is sufficient in all
cases.
> struct ldb_dn *sysdb_user_dn(struct sysdb_ctx *ctx, void *memctx,
> const char *domain, const char *name)
> {
> - return ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_USER, name, domain);
> + errno_t ret;
> + char *clean_name;
> + struct ldb_dn *dn;
> +
> + ret = sysdb_dn_sanitize(NULL, name, &clean_name);
> + if (ret != EOK) {
> + return NULL;
> + }
> +
> + dn = ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_USER,
> + clean_name, domain);
> + talloc_free(clean_name);
> +
> + return dn;
> }
>
> struct ldb_dn *sysdb_group_dn(struct sysdb_ctx *ctx, void *memctx,
> const char *domain, const char *name)
> {
> - return ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_GROUP, name, domain);
> + errno_t ret;
> + char *clean_name;
> + struct ldb_dn *dn;
> +
> + ret = sysdb_dn_sanitize(NULL, name, &clean_name);
> + if (ret != EOK) {
> + return NULL;
> + }
> +
> + dn = ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_GROUP,
> + clean_name, domain);
> + talloc_free(clean_name);
> +
> + return dn;
> }
>
> struct ldb_dn *sysdb_netgroup_dn(struct sysdb_ctx *ctx, void *memctx,
> const char *domain, const char *name)
> {
> - return ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_NETGROUP, name, domain);
> + errno_t ret;
> + char *clean_name;
> + struct ldb_dn *dn;
> +
> + ret = sysdb_dn_sanitize(NULL, name, &clean_name);
> + if (ret != EOK) {
> + return NULL;
> + }
> +
> + dn = ldb_dn_new_fmt(memctx, ctx->ldb, SYSDB_TMPL_NETGROUP,
> + clean_name, domain);
> + talloc_free(clean_name);
> +
> + return dn;
> }
>
> struct ldb_dn *sysdb_netgroup_base_dn(struct sysdb_ctx *ctx, void *memctx,
> --
> 1.7.3.2
>
...
> From 170fba6e14eeea0ff65917a8d7e5b753b260ef73 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Mon, 1 Nov 2010 14:47:09 -0400
> Subject: [PATCH 7/9] Sanitize search filters in memberOf plugin
>
> ---
> Makefile.am | 4 +++-
> src/ldb_modules/memberof.c | 20 ++++++++++++++++++--
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
> index 1e28593dd1c8d1522c9aefa1434d544e91412737..e135cf8d0a0144447f91846e8cc134f2e895e81e 100644
> --- a/src/ldb_modules/memberof.c
> +++ b/src/ldb_modules/memberof.c
> @@ -1206,9 +1208,16 @@ static int memberof_del(struct ldb_module *module, struct ldb_request *req)
> talloc_free(ctx);
> return LDB_ERR_OPERATIONS_ERROR;
> }
> +
> + sret = sss_filter_sanitize(del_ctx, dn, &clean_dn);
> + if (sret != 0) {
> + talloc_free(ctx);
> + return LDB_ERR_OPERATIONS_ERROR;
> + }
> +
> expression = talloc_asprintf(del_ctx,
> "(|(distinguishedName=%s)(%s=%s))",
> - dn, DB_MEMBER, dn);
> + clean_dn, DB_MEMBER, clean_dn);
> if (!expression) {
> talloc_free(ctx);
> return LDB_ERR_OPERATIONS_ERROR;
talloc_free(clean_dn) ?
> @@ -1586,6 +1595,7 @@ static int mbof_del_execute_op(struct mbof_del_operation *delop)
> struct ldb_request *search;
> char *expression;
> const char *dn;
> + char *clean_dn;
> static const char *attrs[] = { DB_OC, DB_NAME,
> DB_MEMBER, DB_MEMBEROF, NULL };
> int ret;
> @@ -1599,9 +1609,15 @@ static int mbof_del_execute_op(struct mbof_del_operation *delop)
> if (!dn) {
> return LDB_ERR_OPERATIONS_ERROR;
> }
> +
> + ret = sss_filter_sanitize(del_ctx, dn, &clean_dn);
> + if (ret != 0) {
> + return LDB_ERR_OPERATIONS_ERROR;
> + }
> +
> expression = talloc_asprintf(del_ctx,
> "(|(distinguishedName=%s)(%s=%s))",
> - dn, DB_MEMBER, dn);
> + clean_dn, DB_MEMBER, clean_dn);
talloc_free(clean_dn) ?
> if (!expression) {
> return LDB_ERR_OPERATIONS_ERROR;
> }
> --
> 1.7.3.2
>
> From d1644133be265567c81482ae755f6d1a4b7e5260 Mon Sep 17 00:00:00 2001
> From: Stephen Gallagher <sgallagh at redhat.com>
> Date: Fri, 5 Nov 2010 10:51:39 -0400
> Subject: [PATCH 8/9] Sanitize sysdb dn for memberof lookup
>
> ---
> src/providers/ldap/sdap_async_accounts.c | 11 ++++++++++-
> 1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_accounts.c b/src/providers/ldap/sdap_async_accounts.c
> index 426bdf44e5072c19eb97aee03d3731fbf1dfebc9..6f4532eea31ff4a027c221983bf91ef06ea66f58 100644
> --- a/src/providers/ldap/sdap_async_accounts.c
> +++ b/src/providers/ldap/sdap_async_accounts.c
> @@ -1915,6 +1915,7 @@ static void sdap_initgr_rfc2307_process(struct tevent_req *subreq)
> struct ldb_message_element *groups;
> size_t count;
> const char *attrs[2];
> + char *clean_dn;
> int ret;
> int i;
>
> @@ -1967,9 +1968,17 @@ static void sdap_initgr_rfc2307_process(struct tevent_req *subreq)
>
> /* Get a list of the groups by groupname only */
> for (i=0; i < groups->num_values; i++) {
> + ret = sysdb_dn_sanitize(state,
> + (const char *)groups->values[i].data,
> + &clean_dn);
> + if (ret != EOK) {
> + tevent_req_error(req, ret);
> + return;
> + }
> +
> ret = sysdb_group_dn_name(state->sysdb,
> sysdb_grouplist,
> - (const char *)groups->values[i].data,
> + clean_dn,
> &sysdb_grouplist[i]);
> if (ret != EOK) {
> tevent_req_error(req, ENOMEM);
talloc_free(clean_dn) ?
More information about the sssd-devel
mailing list