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
dn: cn=foo,dc=example,dc=com objectClass: top objectClass: person sn: foo cn: foo telephoneNumber: 123
Without this access control guessing could be done this way
ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "dc=example, dc=com" "(cn=foo)" dn cn dn: cn=foo,dc=example,dc=com cn: foo ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=0*)(cn=bar))" dn <no entry> ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=1*)(cn=bar))" dn dn: cn=foo,dc=example,dc=com ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=10*)(cn=bar))" dn <no entry> ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=11*)(cn=bar))" dn <no entry> ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=12*)(cn=bar))" dn dn: cn=foo,dc=example,dc=com ...
With the current access control, last 5 searches return <no entry> (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 <no entry>
Now if access control allows non readable attribute ('telephonenumber') but systematically reject matching with it the last 5 searches also return <no entry> But the following searches would be successfull ldapsearch -D "cn=user,dc=example,dc=com" -w xxx -b "cn=foo,dc=example, dc=com" "(|(telephonenumber=*)(cn=foo))" dn telephonenumber cn dn: cn=foo,dc=example,dc=com cn: foo
Related freeipa ticket is https://fedorahosted.org/freeipa/ticket/5168
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.
yes, I think we need to be careful, the approach to just setting a filtercomponent without access to false Is not sufficient
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
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
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
search: 2 result: 0 Success
}}}
{{{ 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
dn: uid=andrey.ivanov,ou=Personnel,ou=Utilisateurs,dc=id,dc=polytechnique,dc=e du uid: andrey.ivanov
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: https://fedorahosted.org/freeipa/ticket/5167 https://fedorahosted.org/freeipa/ticket/5055 https://fedorahosted.org/freeipa/ticket/5130 https://fedorahosted.org/freeipa/ticket/5168 * https://bugzilla.redhat.com/show_bug.cgi?id=1238190
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.
Moving back to NEEDS_TRIAGE so we can re-evaluate the priority as requested in comment#9.
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 - there is no regression - the scenario in the CVE is handled properly - the test cases in this ticket and the related ipa tickets are addressed
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.
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
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
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1316328
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.
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.
and William is right, it is complex, I found some new combinations of nested AND/OR where it does not work :-(
attachment 0001-Ticket-48275-correctly-handle-or-filters-with-compon.patch
attachment 0002-testcase-for-ticket-48275.patch
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
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!
attachment 0001-Ticket-48275-search-returns-no-entry-when-OR-filter-.patch
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 169d0ab
Metadata Update from @lkrispen: - Issue assigned to lkrispen - Issue set to the milestone: 1.3.5.5
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 <img alt="ticket48275_test.py" src="/389-ds-base/issue/raw/files/bc37e10f9784667655d4a83930abcc3ccb7de272fc326fe2ed95d01a82e04cb7-ticket48275_test.py" />
Metadata Update from @lkrispen: - Issue close_status updated to: None (was: Fixed) - Issue set to the milestone: None (was: 1.3.5.5)
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1606
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix
Login to comment on this ticket.