[SSSD] [PATCH] LDAP: Skip dereferenced entries that we are not permitted to read

Jakub Hrozek jhrozek at redhat.com
Mon Sep 8 13:06:13 UTC 2014


On Fri, Sep 05, 2014 at 07:27:07PM +0200, Michal Židek wrote:
> On 09/05/2014 05:51 PM, Jakub Hrozek wrote:
> >On Tue, Sep 02, 2014 at 04:15:20PM +0200, Jakub Hrozek wrote:
> >>Hi,
> >>
> >>the attached patch fixes https://fedorahosted.org/sssd/ticket/2421
> >>
> >>To reproduce, it's enough to run "id admin" with an IPA 4.x server.
> >
> >Michal requested a change in the unit test off-list. A new patch is
> >attached.
> >
> 
> The patch works OK, but I would recommend putting the unit test
> part to a separate patch. The unit test part can not be applied
> on top of 1.11.
> 
> Michal

Sure, see attached patches. The first one applies on top of sssd-1-11
and master, the second is master only.
-------------- next part --------------
>From f44a47d7aa20cd265d87900aa23c049c34b8d765 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 8 Sep 2014 11:49:40 +0200
Subject: [PATCH 1/2] LDAP: Skip dereferenced entries that we are not permitted
 to read

https://fedorahosted.org/sssd/ticket/2421

In case we dereference an entry, for which we have /some/ permissions
for reading, but we only request attributes that we can't access, the
dereference control only returns the DN.

This is also the case with the current version of 389DS for cases where
no entries at all are readable. In this case, the server should not return
the DN at all, though. This DS bug was tracked as
https://fedorahosted.org/389/ticket/47885
---
 src/providers/ldap/sdap.c       | 7 ++++---
 src/providers/ldap/sdap_async.c | 8 +++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
index f2178dd0ad9a318e6c3ae820a1c7812b353780bc..ff50f8b5daf25d31c2eb57014a1a096833c628fa 100644
--- a/src/providers/ldap/sdap.c
+++ b/src/providers/ldap/sdap.c
@@ -580,10 +580,11 @@ errno_t sdap_parse_deref(TALLOC_CTX *mem_ctx,
           "Dereferenced DN: %s\n", orig_dn);
 
     if (!dref->attrVals) {
-        DEBUG(SSSDBG_MINOR_FAILURE,
-              "Dereferenced entry [%s] has no attributes\n",
+        DEBUG(SSSDBG_FUNC_DATA,
+              "Dereferenced entry [%s] has no attributes, skipping\n",
               orig_dn);
-        ret = EINVAL;
+        *_res = NULL;
+        ret = EOK;
         goto done;
     }
 
diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 87c125faa50f4150c5196759c0b689a0731fca84..a8ea53113fe67eed6f50d32f3dd520968b63b76e 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1061,6 +1061,12 @@ static errno_t add_to_deref_reply(TALLOC_CTX *mem_ctx,
 {
     int i;
 
+    if (res == NULL) {
+        /* Nothing to add, probably ACIs prevented us from dereferencing
+         * the attribute */
+        return EOK;
+    }
+
     for (i=0; i < num_maps; i++) {
         if (res[i]->attrs == NULL) continue; /* Nothing in this map */
 
@@ -1829,7 +1835,7 @@ static errno_t sdap_x_deref_parse_entry(struct sdap_handle *sh,
         }
 
         ret = add_to_deref_reply(state, state->num_maps,
-                                    &state->dreply, res);
+                                 &state->dreply, res);
         if (ret != EOK) {
             DEBUG(SSSDBG_OP_FAILURE, "add_to_deref_reply failed.\n");
             goto done;
-- 
1.9.3

-------------- next part --------------
>From 990896e9694090bddc755618d98969775d9b6ab9 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 8 Sep 2014 11:50:29 +0200
Subject: [PATCH 2/2] TESTS: Add a unit test for dereference parsing

---
 src/tests/cmocka/test_sdap.c | 170 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 169 insertions(+), 1 deletion(-)

diff --git a/src/tests/cmocka/test_sdap.c b/src/tests/cmocka/test_sdap.c
index 7c7eb34368b877e475804aff2c1c78a394afd2b6..a1f5188b745af9ede4cc54bc7e1ba5aadc4f07b4 100644
--- a/src/tests/cmocka/test_sdap.c
+++ b/src/tests/cmocka/test_sdap.c
@@ -56,6 +56,56 @@ void set_entry_parse(struct mock_ldap_entry *entry)
     will_return_always(mock_ldap_entry_get, entry);
 }
 
+LDAPDerefRes *mock_deref_res(TALLOC_CTX *mem_ctx,
+                             struct mock_ldap_entry *entry)
+{
+    LDAPDerefRes *dref;
+    LDAPDerefVal *dval, *dvaltail = NULL;
+    size_t nattr;
+    size_t nval;
+
+    dref = talloc_zero(mem_ctx, LDAPDerefRes);
+    assert_non_null(dref);
+
+    dref->derefVal.bv_val = talloc_strdup(dref, entry->dn);
+    assert_non_null(dref->derefVal.bv_val);
+    dref->derefVal.bv_len = strlen(entry->dn);
+
+    if (entry->attrs == NULL) {
+        /* no attributes, done */
+        return dref;
+    }
+
+    for (nattr = 0; entry->attrs[nattr].name; nattr++) {
+        dval = talloc_zero(dref, LDAPDerefVal);
+        assert_non_null(dval);
+
+        dval->type = talloc_strdup(dval, entry->attrs[nattr].name);
+        assert_non_null(dval->type);
+
+        for (nval = 0; entry->attrs[nattr].values[nval]; nval++);
+
+        dval->vals = talloc_zero_array(dval, struct berval, nval+1);
+        assert_non_null(dval->vals);
+        for (nval = 0; entry->attrs[nattr].values[nval]; nval++) {
+            dval->vals[nval].bv_val = talloc_strdup(dval->vals,
+                                             entry->attrs[nattr].values[nval]);
+            assert_non_null(dval->vals[nval].bv_val);
+            dval->vals[nval].bv_len = strlen(dval->vals[nval].bv_val);
+        }
+
+        if (dvaltail != NULL) {
+            dvaltail->next = dval;
+            dvaltail = dvaltail->next;
+        } else {
+            dvaltail = dval;
+            dref->attrVals = dval;
+        }
+    }
+
+    return dref;
+}
+
 /* libldap wrappers */
 int __wrap_ldap_set_option(LDAP *ld,
                            int option,
@@ -121,7 +171,6 @@ struct berval **__wrap_ldap_get_values_len(LDAP *ld,
             return NULL;
         }
         vals[i]->bv_len = strlen(attrvals[i]);
-        assert_non_null(vals[i]->bv_len);
     }
 
     return vals;
@@ -424,6 +473,116 @@ void test_parse_dups(void **state)
     talloc_free(attrs);
 }
 
+void test_parse_deref(void **state)
+{
+    errno_t ret;
+    struct sdap_attr_map_info minfo;
+    struct parse_test_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                      struct parse_test_ctx);
+    struct sdap_deref_attrs **res;
+    LDAPDerefRes *dref;
+
+    const char *oc_values[] = { "posixAccount", NULL };
+    const char *uid_values[] = { "tuser1", NULL };
+    const char *extra_values[] = { "extra", NULL };
+    struct mock_ldap_attr test_ipa_user_attrs[] = {
+        { .name = "objectClass", .values = oc_values },
+        { .name = "uid", .values = uid_values },
+        { .name = "extra", .values = extra_values },
+        { NULL, NULL }
+    };
+    struct mock_ldap_entry test_ipa_user;
+    test_ipa_user.dn = "cn=testuser,dc=example,dc=com";
+    test_ipa_user.attrs = test_ipa_user_attrs;
+
+    ret = sdap_copy_map(test_ctx, rfc2307_user_map, SDAP_OPTS_USER, &minfo.map);
+    minfo.num_attrs = SDAP_OPTS_USER;
+    assert_int_equal(ret, ERR_OK);
+
+    dref = mock_deref_res(test_ctx, &test_ipa_user);
+    assert_non_null(dref);
+
+    ret = sdap_parse_deref(test_ctx, &minfo, 1, dref, &res);
+    talloc_free(dref);
+    talloc_free(minfo.map);
+    assert_int_equal(ret, ERR_OK);
+    assert_non_null(res);
+
+    /* The extra attribute must not be downloaded, it's not present in map */
+    assert_non_null(res[0]);
+    assert_true(res[0]->map == minfo.map);
+
+    assert_entry_has_attr(res[0]->attrs, SYSDB_ORIG_DN,
+                          "cn=testuser,dc=example,dc=com");
+    assert_entry_has_attr(res[0]->attrs, SYSDB_NAME, "tuser1");
+    assert_entry_has_no_attr(res[0]->attrs, "extra");
+    talloc_free(res);
+}
+
+void test_parse_deref_no_attrs(void **state)
+{
+    errno_t ret;
+    struct sdap_attr_map_info minfo;
+    struct parse_test_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                      struct parse_test_ctx);
+    struct sdap_deref_attrs **res;
+    LDAPDerefRes *dref;
+
+    struct mock_ldap_entry test_ipa_user;
+    test_ipa_user.dn = "cn=testuser,dc=example,dc=com";
+    test_ipa_user.attrs = NULL;
+
+    ret = sdap_copy_map(test_ctx, rfc2307_user_map, SDAP_OPTS_USER, &minfo.map);
+    minfo.num_attrs = SDAP_OPTS_USER;
+    assert_int_equal(ret, ERR_OK);
+
+    dref = mock_deref_res(test_ctx, &test_ipa_user);
+    assert_non_null(dref);
+
+    ret = sdap_parse_deref(test_ctx, &minfo, 1, dref, &res);
+    talloc_free(dref);
+    talloc_free(minfo.map);
+    assert_int_equal(ret, ERR_OK);
+    assert_null(res); /* res must be NULL on receiving no attributes */
+}
+
+void test_parse_deref_map_mismatch(void **state)
+{
+    errno_t ret;
+    struct sdap_attr_map_info minfo;
+    struct parse_test_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                      struct parse_test_ctx);
+    struct sdap_deref_attrs **res;
+    LDAPDerefRes *dref;
+
+    const char *oc_values[] = { "posixAccount", NULL };
+    const char *uid_values[] = { "tuser1", NULL };
+    struct mock_ldap_attr test_ipa_user_attrs[] = {
+        { .name = "objectClass", .values = oc_values },
+        { .name = "uid", .values = uid_values },
+        { NULL, NULL }
+    };
+    struct mock_ldap_entry test_ipa_user;
+    test_ipa_user.dn = "cn=testuser,dc=example,dc=com";
+    test_ipa_user.attrs = test_ipa_user_attrs;
+
+    ret = sdap_copy_map(test_ctx, rfc2307_group_map, SDAP_OPTS_GROUP, &minfo.map);
+    minfo.num_attrs = SDAP_OPTS_GROUP;
+    assert_int_equal(ret, ERR_OK);
+
+    dref = mock_deref_res(test_ctx, &test_ipa_user);
+    assert_non_null(dref);
+
+    ret = sdap_parse_deref(test_ctx, &minfo, 1, dref, &res);
+    talloc_free(dref);
+    talloc_free(minfo.map);
+    assert_int_equal(ret, ERR_OK);
+    assert_non_null(res);
+    /* the group map didn't match, so no attrs will be parsed out of the map */
+    assert_null(res[0]->attrs);
+    talloc_free(res);
+}
+
 /* Negative test - objectclass doesn't match the map */
 void test_parse_bad_oc(void **state)
 {
@@ -548,6 +707,12 @@ int main(int argc, const char *argv[])
         unit_test_setup_teardown(test_parse_dups,
                                  parse_entry_test_setup,
                                  parse_entry_test_teardown),
+        unit_test_setup_teardown(test_parse_deref,
+                                 parse_entry_test_setup,
+                                 parse_entry_test_teardown),
+        unit_test_setup_teardown(test_parse_deref_no_attrs,
+                                 parse_entry_test_setup,
+                                 parse_entry_test_teardown),
         /* Negative tests */
         unit_test_setup_teardown(test_parse_no_oc,
                                  parse_entry_test_setup,
@@ -558,6 +723,9 @@ int main(int argc, const char *argv[])
         unit_test_setup_teardown(test_parse_no_dn,
                                  parse_entry_test_setup,
                                  parse_entry_test_teardown),
+        unit_test_setup_teardown(test_parse_deref_map_mismatch,
+                                 parse_entry_test_setup,
+                                 parse_entry_test_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
1.9.3



More information about the sssd-devel mailing list