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(a)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(a)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(a)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(a)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) ?