Re: [SSSD] [PATCHSET] LDAP: sanitize group name when used in filter
On (07/08/15 13:58), Pavel Březina wrote:
>On 08/07/2015 06:28 AM, Lukas Slebodnik wrote:
>>On (06/08/15 15:04), Pavel Reichl wrote:
>>>
>>>
>>>On 08/06/2015 02:55 PM, Lukas Slebodnik wrote:
On (06/08/15 14:31), Pavel Reichl wrote:
>On 08/05/2015 02:44 PM, Pavel Březina wrote:
>>On 08/05/2015 12:11 PM, Pavel Reichl wrote:
>>>
>>>On 08/05/2015 11:34 AM, Pavel Březina wrote:
On 08/04/2015 03:52 PM, Pavel Reichl wrote:
>Hello,
>
>please see 2 simple patches attached.
>
>I could not find function to sanitize DN so it could be used as part
>of
>filter (sanitize ()*/\...) so I had to write one.
>
> sysdb_dn_sanitize is not the right choice,
>
>sysdb_dn_sanitize("name=expired-group(2016),cn=groups,cn=LOCAL,cn=sysdb")
>
>->
>"name\\3Dexpired-group(2016)\\,cn\\3Dgroups\\,cn\\3DLOCAL\\,cn\\3Dsysdb"
>
>
>Thanks!
Hi, I did just a quick read of your patches... can you take one more
step with creating a sanitized dn and create a more generic function
for that?
Have you considered to modify sysdb_dn_sanitize to also escape
parentheses (that's what is misssing, isn't it)?
>>>no because sysdb_dn_sanitize escapes also ',' and '=' and I need them to
>>>stat as they are
>>>
>>>This is what I have:
>>>"name=expired-group(2016),cn=groups,cn=LOCAL,cn=sysdb"
>>>This is what I need:
>>>"name=expired-group\282016\29,cn=groups,cn=LOCAL,cn=sysdb" // just
>>>escape '(' and ')'
>>>This is what sysdb_dn_sanitize returns:
>>>"name\\3Dexpired-group(2016)\\,cn\\3Dgroups\\,cn\\3DLOCAL\\,cn\\3Dsysdb"
>>>
>>>Failing filter:
>>>(&(objectClass=user)(|(memberOf=name=VDI-US02_Corporate-Environment(2013),cn=groups,cn=qut.edu.au,cn=sysdb)
>>>
>>>
>>>Corrent filter
>>>(&(objectClass=user)(|(memberOf=name=VDI-US02_Corporate-Environment\282013\29,cn=groups,cn=qut.edu.au,cn=sysdb)
>>>
>>>
>>>
>>>I hope it's clearer now.
>>Of course... sysdb_dn_sanitize is not supposed to be called on the whole
>>dn. Just on the name part. It mean "sanitize value so it can be used in
>>dn". But changing it to also escape parentheses would require sysdb and
>>code update, so it is not worth it.
>>
>>>+static errno_t
>>>+get_group_dn_with_filter_sanitized_name(TALLOC_CTX *mem_ctx,
>>>+struct sss_domain_info *domain,
>>>+const char *grp_name,
>>>+const char **_grp_dn);
>>Can you use group_name and _group_dn? Two characters won't kill anybody
>>:-)
>>Otherwise we can keep the code as is. I have just one recommendation for
>>tests:
>Sure, done.
>>>+/* let records to expire */
>>>+usleep(110);
>>It will be better to expire the records manually by setting expiration
>>time
>>to zero. I'm not sure if we have already a function for that, if not,
>>please write one. It may be quite useful for tests.
>I agree with you and I know that you would prefer the function to be
>generic
>and part of sysdb. But I am afraid that It would take too much time to do
>it
>properly and we should also handle code duplication that would be
>introduced
>to sss_cache.c. Would static function in this test be sufficient temporal
>solution for now? I would also file a ticket for proper solution. Is this
>OK
>with you?
>
I didn't try but I have an idea.
sysdb_group_dn calls sysdb_dn_sanitize to sanitize name and then
it creates "struct ldb_dn".
It might be goot to try use sysdb_group_dn + convert dn to string.
I hope it should be properly escaped.
>>>Lukas, sysdb_dn_sanitize() does not escape '(',')','*',... as they are valid
>>>characters in DN AFAIK. But they have a special meaning when used as a part
>>>of the filter.
>>Yes, that's true. I checked RFC4514 and RFC2253
>>
>>However, I do not understand why full dn cannot be escaped and just rdn part
>>of
>>dn.
>>
>>You can use hexadecimal representatin even for normal letters.
>>[root@host db]# ldbsearch -H cache_example.com.ldb -b
>>name=pcp,cn=groups,cn=example.com,cn=sysdb '(name=\70\63\70)'
>># record 1
>>dn: name=pcp,cn=groups,cn=example.com,cn=sysdb
>>createTimestamp: 1438338481
>>gidNumber: 967
>>name: pcp
>>objectClass: group
>>isPosix: TRUE
>>lastUpdate: 1438673281
>>dataExpireTimestamp: 1438678681
>>distinguishedName: name=pcp,cn=groups,cn=example.com,cn=sysdb
>>
>># returned 1 records
>># 1 entries
>># 0 referrals
>>[root@host db]# ldbsearch -H cache_example.com.ldb -b
>>name=pcp,cn=groups,cn=example.com,cn=sysdb '(name=pcp)'
>># record 1
>>dn: name=pcp,cn=groups,cn=example.com,cn=sysdb
>>createTimestamp: 1438338481
>>gidNumber: 967
>>name: pcp
>>objectClas
Re: [SSSD] [PATCHES] cleanup task: Expire all memberof targets when removing user
On (07/08/15 20:54), Michal Židek wrote:
>Hi,
>
>see the attached patches for ticket
>https://fedorahosted.org/sssd/ticket/2676
>
>Removing the user during cleanup task completely
>removes that user from local database, so that
>user is no longer part of any group in sysdb.
>
>Such user may, or may not have been removed from
>LDAP so the only way to figure out is check
>on the next refresh otherwise we may get
>inconsistent results.
>
>The third patch adds integration test for this
>Apply and run the intgcheck without the first 2
>patches to see the failed test (you will also see that
>all the tests after this failed one will end with
>errors as well, that is probably due to incomplete
>clean up after the ldap tests and I do not intend to
>solve this as part of this patchset but will
>investigate it later).
>
>From 9930382acae9eb708c609355131207a0be42d1e9 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Michal=20=C5=BDidek?=
>Date: Thu, 6 Aug 2015 09:16:03 +0200
>Subject: [PATCH 1/3] SYSDB: Add function to expire entry
>
>Ticket:
>https://fedorahosted.org/sssd/ticket/2676
>
>Added function to expire entry in sysdb using
>its DN.
>---
> src/db/sysdb.h | 10 ++-
> src/db/sysdb_ops.c | 51
> src/tests/sysdb-tests.c | 69 +
> 3 files changed, 129 insertions(+), 1 deletion(-)
>
>diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>index 9e28b5c..a48e22d 100644
>--- a/src/db/sysdb.h
>+++ b/src/db/sysdb.h
>@@ -387,6 +387,11 @@ errno_t sysdb_msg2attrs(TALLOC_CTX *mem_ctx, size_t count,
> /* convert an ldb error into an errno error */
> int sysdb_error_to_errno(int ldberr);
>
>+/* Convert ldb value to ldb dn */
>+struct ldb_dn *sysdb_ldb_val_to_ldb_dn(TALLOC_CTX *tctx,
>+ struct sss_domain_info *dom,
>+ struct ldb_val *ldbval);
>+
> /* DNs related helper functions */
> errno_t sysdb_get_rdn(struct sysdb_ctx *sysdb, TALLOC_CTX *mem_ctx,
> const char *dn, char **_name, char **_val);
>@@ -717,11 +722,14 @@ int sysdb_delete_entry(struct sysdb_ctx *sysdb,
>struct ldb_dn *dn,
>bool ignore_not_found);
>
>-
> int sysdb_delete_recursive(struct sysdb_ctx *sysdb,
>struct ldb_dn *dn,
>bool ignore_not_found);
>
>+/* Mark entry as expired (permissive) */
>+int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom,
>+ struct ldb_dn *ldbdn);
>+
> /* Search Entry */
> int sysdb_search_entry(TALLOC_CTX *mem_ctx,
>struct sysdb_ctx *sysdb,
>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>index d1d43eb..c582578 100644
>--- a/src/db/sysdb_ops.c
>+++ b/src/db/sysdb_ops.c
>@@ -3875,3 +3875,54 @@ errno_t sysdb_handle_original_uuid(const char
>*orig_name,
>
> return EOK;
> }
>+
>+struct ldb_dn *sysdb_ldb_val_to_ldb_dn(TALLOC_CTX *tctx,
>+ struct sss_domain_info *dom,
>+ struct ldb_val *ldbval)
>+{
>+return ldb_dn_from_ldb_val(tctx, dom->sysdb->ldb, ldbval);
>+}
I would prefer to do not expose such simple functions as sysdb functions.
I do not see a reason why should convert ldb_val -> ldb_dn
in other modules.
If wee need two versions of sysdb_mark_entry_as_expired e can still have two
functions:
sysdb_mark_entry_as_expired_ldb_dn
sysdb_mark_entry_as_expired_ldb_val
The core logic can be in one and second will be wrapper with
ldb_dn_from_ldb_val.
>+
>+/* Mark entry as expired if that entry exists in database. */
>+int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom,
>+ struct ldb_dn *ldbdn)
>+{
>+struct ldb_message *msg;
>+int ret;
>+TALLOC_CTX *tmp_ctx;
>+
>+tmp_ctx = talloc_new(NULL);
>+if (tmp_ctx == NULL) {
>+return ENOMEM;
>+}
>+
>+msg = ldb_msg_new(tmp_ctx);
>+if (msg == NULL) {
>+ret = ENOMEM;
>+goto done;
>+}
>+
>+msg->dn = ldbdn;
>+
>+ret = ldb_msg_add_empty(msg, SYSDB_CACHE_EXPIRE,
>+LDB_FLAG_MOD_REPLACE, NULL);
>+if (ret != LDB_SUCCESS) {
>+goto done;
>+}
>+
>+ret = ldb_msg_add_string(msg, SYSDB_CACHE_EXPIRE, "1");
>+if (ret != LDB_SUCCESS) {
>+goto done;
>+}
>+
>+ret = sss_ldb_modify_permissive(dom->sysdb->ldb, msg);
We should avoid using sss_ldb_modify_permissive.
It should be used in very rare cases.
Could you explain why do we need to it here?
>+if (ret != LDB_SUCCESS) {
>+goto done;
>+}
>+
>+ret = EOK;
>+done:
>+talloc_free(tmp_ctx);
>+return ret;
>+}
>+
>diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
>index 24d1527..3bd03b1 100644
>--- a/src/tests/sysdb-tests.c
>+++ b/src/tests/sysdb-tests.c
>@@ -6212,6 +6212,74 @@ START_TEST(test_confdb_l
Re: [SSSD] [PATCH] krb5_utils-tests: Remove unused variables
On (07/08/15 15:02), Pavel Reichl wrote: > > >On 08/07/2015 02:43 PM, Lukas Slebodnik wrote: >>ehlo, >> >>there was a warning/error >>src/tests/krb5_utils-tests.c: In function ‘test_sss_krb5_realm_has_proxy’: >>src/tests/krb5_utils-tests.c:690:10: error: unused variable ‘perr’ >>[-Werror=unused-variable] >> long perr; >> ^ >>src/tests/krb5_utils-tests.c:689:21: error: unused variable ‘kerr’ >>[-Werror=unused-variable] >> krb5_error_code kerr; >> ^ >>cc1: all warnings being treated as errors >> >>LS >> >ACK, Thank you for review. It would be better if you could let review of easy patches for new developers. >ci passed (as we define it these days) ^^ These words are not appropriate. We are all responsible for the current state. 1. we can catch problematic tests during review [1] 2. we can send patches for existing issues [2] 3. we can help other developers to fix problematic test (pcech is working on problematic test more than 3 days) I would like to encourage you to be proactive. We have many tickets in "SSSD Continuous integration" bucket[3] It's much better strategy than complaints to the curent state of CI. LS [1] https://lists.fedorahosted.org/pipermail/sssd-devel/2015-May/023594.html [2] https://lists.fedorahosted.org/pipermail/sssd-devel/2015-May/023556.html [3] https://fedorahosted.org/sssd/report/3 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] NSS: Fix use after free
ehlo,
Use after free can happed if there are two domains and user is not found
in the first one.
LS
>From cbd388699c061ad644ead6c21e38dfb9bc0ca30c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik
Date: Fri, 7 Aug 2015 14:29:45 +0200
Subject: [PATCH] NSS: Fix use after free
It can happed if there are two domains and user is not found
in the first one.
==29279== Invalid read of size 1
==29279==at 0x4C2CBA2: strlen (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29279==by 0x89A7AC4: talloc_strdup (in /usr/lib64/libtalloc.so.2.1.2)
==29279==by 0x11668A: nss_cmd_initgroups_search (nsssrv_cmd.c:4191)
==29279==by 0x118B27: nss_cmd_getby_dp_callback (nsssrv_cmd.c:1208)
==29279==by 0x10F2B4: nsssrv_dp_send_acct_req_done (nsssrv_cmd.c:759)
==29279==by 0x126AFB: sss_dp_internal_get_done (responder_dp.c:802)
==29279==by 0x56EA861: ??? (in /usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x56EDB50: dbus_connection_dispatch (in
/usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x50721E1: sbus_dispatch (sssd_dbus_connection.c:96)
==29279==by 0x879B22E: tevent_common_loop_timer_delay (tevent_timed.c:341)
==29279==by 0x879C239: epoll_event_loop_once (tevent_epoll.c:911)
==29279==by 0x879A936: std_event_loop_once (tevent_standard.c:114)
==29279== Address 0xbbad240 is 96 bytes inside a block of size 106 free'd
==29279==at 0x4C2AD17: free (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==29279==by 0x89A46E3: _talloc_free (in /usr/lib64/libtalloc.so.2.1.2)
==29279==by 0x116679: nss_cmd_initgroups_search (nsssrv_cmd.c:4190)
==29279==by 0x118B27: nss_cmd_getby_dp_callback (nsssrv_cmd.c:1208)
==29279==by 0x10F2B4: nsssrv_dp_send_acct_req_done (nsssrv_cmd.c:759)
==29279==by 0x126AFB: sss_dp_internal_get_done (responder_dp.c:802)
==29279==by 0x56EA861: ??? (in /usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x56EDB50: dbus_connection_dispatch (in
/usr/lib64/libdbus-1.so.3.7.4)
==29279==by 0x50721E1: sbus_dispatch (sssd_dbus_connection.c:96)
==29279==by 0x879B22E: tevent_common_loop_timer_delay (tevent_timed.c:341)
==29279==by 0x879C239: epoll_event_loop_once (tevent_epoll.c:911)
==29279==by 0x879A936: std_event_loop_once (tevent_standard.c:114)
Resolves:
https://fedorahosted.org/sssd/ticket/2749
---
src/responder/nss/nsssrv_cmd.c | 6 +++---
src/responder/nss/nsssrv_private.h | 1 +
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index
e754245eac7200c2f667760540a1d04c9203843d..43cdb135c0933117743a97d6a2524326079bf72c
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4143,7 +4143,7 @@ static int nss_cmd_initgr_send_reply(struct nss_dom_ctx
*dctx)
}
ret = fill_initgr(cctx->creq->out, dctx->domain, dctx->res, nctx,
- dctx->mc_name, cmdctx->name);
+ dctx->mc_name, cmdctx->normalized_name);
if (ret) {
return ret;
}
@@ -4187,14 +4187,14 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx
*dctx)
/* make sure to update the dctx if we changed domain */
dctx->domain = dom;
-talloc_free(name);
+talloc_zfree(cmdctx->normalized_name);
name = sss_get_cased_name(dctx, cmdctx->name, dom->case_sensitive);
if (!name) return ENOMEM;
name = sss_reverse_replace_space(cmdctx, name,
nctx->rctx->override_space);
/* save name so it can be used in initgr reply */
-cmdctx->name = name;
+cmdctx->normalized_name = name;
if (name == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE,
"sss_reverse_replace_space failed\n");
diff --git a/src/responder/nss/nsssrv_private.h
b/src/responder/nss/nsssrv_private.h
index
e5a2486f1fb9a8de39ec90f802f596b2c2f6af7f..72f7b75604567f9b95937018e54ba2d60b771f9b
100644
--- a/src/responder/nss/nsssrv_private.h
+++ b/src/responder/nss/nsssrv_private.h
@@ -31,6 +31,7 @@ struct nss_cmd_ctx {
struct cli_ctx *cctx;
enum sss_cli_command cmd;
char *name;
+const char *normalized_name;
bool name_is_upn;
uint32_t id;
char *secid;
--
2.5.0
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
