#48275 search returns no entry when OR filter component contains non readable attribute
Closed: Fixed None Opened 3 years ago by tbordaz.

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


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

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

}}}

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

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

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!

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

2 years ago

Login to comment on this ticket.

Metadata