[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