389-ds-bot
commented
4 years ago Comment from tbordaz (@tbordaz) at 2015-09-10 22:23:19
Related freeipa ticket is https://fedorahosted.org/freeipa/ticket/5168
Closed 389-ds-bot closed 4 years ago
389-ds-bot
commented
4 years ago Comment from tbordaz (@tbordaz) at 2015-09-10 22:23:19
Related freeipa ticket is https://fedorahosted.org/freeipa/ticket/5168
389-ds-bot
commented
4 years ago Comment from tbordaz (@tbordaz) at 2015-09-11 12:35:13
Additional thoughts. The fix should take care to the following use case:
ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(!(telephonenumber=1*))(cn=bar))" dn
In that case the telephonenumber starts with a '1', but the component (!(telephonenumber=1*)) will get evaluated to TRUE because telephonenumber is not readable. So the entry may get returned although it does not match the filter.
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2015-09-14 14:55:07
yes, I think we need to be careful, the approach to just setting a filtercomponent without access to false Is not sufficient
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2015-09-16 15:49:27
setting a filtercomponent without access to false leads to problems as in comment 2
maybe we can use a three valued logic, like it is done in the bind rule evaluation in libaccess and commented on in acllas.c
So a component without access would be set to "undefined" and the following rules could be applied
(!(undefined)) --> undefined (|(undefined)(true)) --> true (|(undefined)(false)) --> false (&(undefined)(true)) --> false (&(undefined)(false)) --> false
in the above example we would evaluate "(|(!(telephonenumber=1*))(cn=bar))" ==> "(|(!(undefined))(cn=bar))" ==> "(|(undefined)(cn=bar))" and the result woul ddepend only on cn=bar
389-ds-bot
commented
4 years ago Comment from tbordaz (@tbordaz) at 2015-09-16 18:44:03
That looks a great idea. Although it is difficult to anticipate all impact of such change. A question, do we need to add those rules ? (|(undefined)(undefined)) -> false (&(undefined)(undefined)) -> false
389-ds-bot
commented
4 years ago Comment from pj101 at 2015-10-29 19:48:51
There is another side effect if this bug that i've just stumbled upon (using v1.3.3.x).
The LDAP filters in devices like printers/scanners/multi-function printers are often hard-coded and impossible to change. Since they are supposed to work with AD, they often use the attribute sAMAccountName not present in 389ds schema.
If a (e.g., anonymous in this case) user makes a search using the filter containing an attribute that is not present in the schema (ex sAMAccountName), the search returns 0 results when it should return this entry - all other search filter attributes are allowed to this anonymous user:
ldapsearch -x -h ldap-model.polytechnique.fr -b "ou=Utilisateurs,dc=id,dc=polytechnique,dc=edu" '(&(objectClass=person)(|(|(uid=ivanov*)(cn=ivanov*)(sAMAccountName=ivanov*)(sn=ivanov*)(givenName=ivanov*)(mail=ivanov*)))(mail=*))' uid
# extended LDIF
#
# LDAPv3
# base <ou=Utilisateurs,dc=id,dc=polytechnique,dc=edu> with scope subtree
# filter: (&(objectClass=person)(|(|(uid=ivanov*)(cn=ivanov*)(sAMAccountName=ivanov*)(sn=ivanov*)(givenName=ivanov*)(mail=ivanov*)))(mail=*))
# requesting: uid
#
# search result
search: 2
result: 0 Success
# numResponses: 1
ldapsearch -x -h ldap-model.polytechnique.fr -b "ou=Utilisateurs,dc=id,dc=polytechnique,dc=edu" '(&(objectClass=person)(|(|(uid=ivanov*)(cn=ivanov*)(sn=ivanov*)(givenName=ivanov*)(mail=ivanov*)))(mail=*))' uid
# extended LDIF
#
# LDAPv3
# base <ou=Utilisateurs,dc=id,dc=polytechnique,dc=edu> with scope subtree
# filter: (&(objectClass=person)(|(|(uid=ivanov*)(cn=ivanov*)(sn=ivanov*)(givenName=ivanov*)(mail=ivanov*)))(mail=*))
# requesting: uid
#
# andrey.ivanov, Personnel, Utilisateurs, id.polytechnique.edu
dn: uid=andrey.ivanov,ou=Personnel,ou=Utilisateurs,dc=id,dc=polytechnique,dc=e
du
uid: andrey.ivanov
# search result
search: 2
result: 0 Success
# numResponses: 2
# numEntries: 1
Comment from pvoborni at 2016-02-16 23:08:53
Hello, I'd like to ask to give this ticket a priority, i.e., implement in 1.3.5.
This issue already caused several other issues:
It also prevents to use IPA permission system effectively for non-admin users when admins want to restrict access to certain attributes - IPA uses certain LDAP search filter and it doesn't know how to change it because it doesn't know what are the user's rights so then the effect is the same as in tickets 5167, 5055, 5130.
389-ds-bot
commented
4 years ago Comment from nkinder (@nkinder) at 2016-02-16 23:19:40
Moving back to NEEDS_TRIAGE so we can re-evaluate the priority as requested in comment9.
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-02-17 20:52:12
I will write a design doc based on the ideas in comment 4.
General rule the addition or removal of a component in an OR filter where there is no search access does have no impact on the search result, independent of the fact that this componenent matches or not.
The coded change seems not too be as much effort as to design the test suite, to ensure
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-02-18 19:55:21
found a new problem, the slapi plugin api exposes a function:
int slapi_vattr_filter_test_ext( Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Filter *f,
int verify_access , int only_test_access);where only_test_access will anly check access to the filter, in this case it is not known which components contribute to the filter matching and we have still to evaluate access to ALL attributes used in the filter.
In DS itself this is only used if filter test can be bypassed (can_skip_filter_test() ). This is now only true for search filters with exactly one simple filter.
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-03-08 13:59:50
I think the rules as defined in comment 4 (!(undefined)) --> undefined (|(undefined)(true)) --> true (|(undefined)(false)) --> false (&(undefined)(true)) --> false (&(undefined)(false)) --> false
are not fully correct. and filters in combination with not,could invert the result even if not access to all and component is granted. it should better be: (!(undefined)) --> undefined (|(undefined)(true)) --> true (|(undefined)(false)) --> false (&(undefined)(true)) --> undefined (&(undefined)(false)) --> undefined
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-03-08 15:48:56
well, it probably was still not correct for and case:
if we have: (&(undefined)(false))
the and will always be false independent of the undefined part, so:
(&(undefined)(false)) --> false
if we have: (&(undefined)(true))
the result of the and is unknown, because the undefined part cannot be evaluated, so it should be:
(&(undefined)(true)) --> undefined
389-ds-bot
commented
4 years ago Comment from nhosoi (@nhosoi) at 2016-03-10 07:02:14
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1316328
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2016-05-20 07:45:41
I think that there are two cases here.
One is where OR and there is no attribute to satisfy the filter. I'm happy for that to be allowed.
As for the second issue, where a value exists for the attribute, but is denied. I can see the rules as proposed by Ludwig are very sound, but they may add complexity in diagnosing an issue for administrators. It would be very subtle to detect and determine why a query is failing if we are denying / allowing based on parts of an entry.
I'm assuming here also that when a filter component moves to undefined, it matches nothing.
So either we'll end up allowing some subtle queries and aci changes to work because they have partially matching attribute sets based on their acis. But at the same time, this may cause queries in other cases to suddenly start working on an upgrade which could create subtle application failures when before the application relied on the filter result failing rather than a partial result set.
I can see some benefits to this change, but I worry that it could add a complexity and subtle issues.
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-05-25 14:19:24
William, if you say: "this may cause queries in other cases to suddenly start working " that's what the fix is about, the ticket is logged as a defect and fixing it has to change behaviour. We did this with the last change when fixing the CVE (in the easiest way) and searches no longer worked. Many clients were running into this and have been fixed, but this fix should prevent new clients to run into it again. I think there should be no client relying on doing a search and expecting no results.
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-05-25 15:43:26
and William is right, it is complex, I found some new combinations of nested AND/OR where it does not work :-(
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-05-25 19:28:41
attachment 0001-Ticket-48275-correctly-handle-or-filters-with-compon.patch
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-05-25 19:28:52
attachment 0002-testcase-for-ticket-48275.patch
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-05-25 20:22:47
it looks like the attached patch handles this, it requires also for AND filter lists to do matching and access testing for each component together
389-ds-bot
commented
4 years ago Comment from nhosoi (@nhosoi) at 2016-05-25 23:44:52
The patch 0001-Ticket-48275-correctly-handle-or-filters-with-compon.patch looks good to me.
It'd be nice if you could add comments about the return values from slapi_vattr_filter_test_ext_internal (nomatch: < 0, success: 0, and error including undefined: > 0, are they?) as well as the rules described in #comment:14 and #comment:15.
Thanks!
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-06-07 19:41:40
attachment 0001-Ticket-48275-search-returns-no-entry-when-OR-filter-.patch
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2016-06-07 19:48:40
Added comment as suggested by Noriko, attached revised patch and committed to master
ldap/servers/slapd/filterentry.c | 208 +++++++++++++++++++++++++-------------- 1 file changed, 138 insertions(+), 70 deletions(-)
New commits: commit 169d0ab5e8d2d61d39585c3a8981a3707578cae8
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2017-02-11 23:00:02
Metadata Update from @elkris:
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2019-05-22 12:15:30
test case is no longer working in current lib389, here is slightly modified version which can be used, but needs to be reworked before committing
389-ds-bot
commented
4 years ago Comment from lkrispen (@elkris) at 2019-05-22 12:15:30
Metadata Update from @elkris:
Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/48275
Problem description access control requires that a user has read access to all attributes in OR filter components. Else no entry is returned, even if the filter matches some entries. This is to prevent guessing of attribute values using OR filter. The problem is that this requirement prevents to use non readable attribute in filter. If we make sure that component, with non readable attributes, do not match the selected entry. then guessing would be prevented and it will allow non readable attributes in the filter.
For example, 'user' has read access on 'cn' but no read access over 'telephonenumber' attribute
Without this access control guessing could be done this way
With the current access control, last 5 searches return (preventing guessing)
But also
ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=*)(cn=foo))" dn