search returns no entry when OR filter component contains non readable attribute

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/48275

• Created at 2015-09-10 21:43:41 by tbordaz (@tbordaz)
• Closed as None
• Assigned to lkrispen (@elkris)
• Associated bugzillas
  • https://bugzilla.redhat.com/show_bug.cgi?id=1316328

---

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 (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

Now if access control allows non readable attribute ('telephonenumber') but systematically reject matching with it
the last 5 searches also return
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

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2015-09-10 22:23:19

Related freeipa ticket is https://fedorahosted.org/freeipa/ticket/5168

[389-ds-bot]

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]

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]

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]

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]

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

[389-ds-bot]

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:

• 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.

[389-ds-bot]

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]

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

• 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

[389-ds-bot]

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]

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]

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]

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]

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]

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]

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]

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]

Comment from lkrispen (@elkris) at 2016-05-25 19:28:52

attachment
0002-testcase-for-ticket-48275.patch

[389-ds-bot]

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]

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]

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]

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 169d0ab

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-02-11 23:00:02

Metadata Update from @elkris:

• Issue assigned to elkris
• Issue set to the milestone: 1.3.5.5

[389-ds-bot]

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]

Comment from lkrispen (@elkris) at 2019-05-22 12:15:30

Metadata Update from @elkris:

• Issue close_status updated to: None (was: Fixed)
• Issue set to the milestone: None (was: 1.3.5.5)