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

Jakub Hrozek jhrozek at redhat.com
Tue Sep 2 14:15:20 UTC 2014


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.
-------------- next part --------------
>From 1e4b512c1054709a9f684d85a21e970d12539798 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 27 Aug 2014 15:49:26 +0200
Subject: [PATCH] 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 +-
 src/tests/cmocka/test_sdap.c    | 168 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 178 insertions(+), 5 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 ed20b26fdee63a3a4c749b7941f6d781ae1793ec..3231d92809e24abc2507974cbcab75155e5d2f9f 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 */
 
@@ -1818,7 +1824,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;
diff --git a/src/tests/cmocka/test_sdap.c b/src/tests/cmocka/test_sdap.c
index 7c7eb34368b877e475804aff2c1c78a394afd2b6..7ae7f4a6f2b7959036404f22b52166fe6241dbb4 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,114 @@ 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);
+    talloc_free(res);
+}
+
 /* Negative test - objectclass doesn't match the map */
 void test_parse_bad_oc(void **state)
 {
@@ -548,6 +705,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 +721,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