389 should assert that filters conform to schema

[389-ds-bot]

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

• Created at 2019-05-03 03:49:19 by firstyear (@Firstyear)
• Closed at 2019-08-29 01:54:39 as fixed
• Assigned to firstyear (@Firstyear)
• Associated bugzillas
  • https://bugzilla.redhat.com/show_bug.cgi?id=1721425

---

Issue Description

389 Should assert that all attributes in a filter are present and valid in schema. If there are attributes in a filter that are not in schema, this can lead to DOS due to fall-back to un-indexed scans, and it also can mask and cover-up application and development issues with queries. For example, the referenced case was caused by IPA mistakenly searching an attribute that can never be satisfied by ACI/filter.

This should optionally be allowed to be disabled, due to some sites that use things like extensibleObject that by nature, bypass and violate schema.

https://pagure.io/389-ds-base/pull-request/50252#comment-85208

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-09 05:05:09

So I'm thinking about this and I think that we should check filters against the schema - if they do not have an appropriate attributeType, we flag on the filter element that it is NOT_SCHEMA_PRESENT, similar to the flags used currently to make AND/OR as tombstone filters.

On detection of the the NOT_SCHEMA_PRESENT flag, the IDL code could then determine this flag is present, and return a 0 length IDL, as well as setting a new notes= flag, such as F or something yet to determine. This way, we could have notes=F meaning 'filter requested a term that could not be satisfied".

To support some installs that may be using this for extensibleObject or other, that they do index, a cn=config flag such as filter-validate-schema-enable should be added. We could have this as on/off to toggle these states, or we could have it set to something like "strict/warning/off", where strict would return ldap error's on invalid filter, warning is the notes=F behaviour, and off is the extensible object behaviour.

The default should initially be "warning", but I could see value in tests in lib389 defaulting to strict, or even later defaulting to strict for all installs.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-09 05:05:10

Metadata Update from @Firstyear:

• Custom field origin adjusted to None
• Custom field reviewstatus adjusted to None

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-10 04:41:00

Would be keen to hear your thoughts on this one @elkris

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-14 05:46:07

So I think that the way to do this would be to make a function similar to slapi_entry_schema_check, like slapi_filter_schema_check. It would take an argument such as "policy" that defines if we should ignore the check, warning to flag the filter item with NOT_SCHEMA_PRESENT, and strict which will cause a failure return.

On failure return, we cancel the operation with an appropriate error code.

If NOT_SCHEMA_PRESENT is flagged on a filter, then in the backend, we generate an empty IDL.

I think we'd default policy to warning, aiming to change to strict in the future.

Anyway, I think that having the check functions in schema.c is the correct place to match how we check entries are schema compliant.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-05-14 14:13:42

Would be keen to hear your thoughts on this one @elkris

yes, it could be good to have an option to check if attributes used are defined in the schema. But we need to be able to turn this off, some applications have turned schema checking off, some have clients querying different ldap servers (eg AD+389) and I have seen using attrs in an OR filter which are defined in one instance and not the other, but the search should succeed and the unknown attr ignored.

The question is how to respond to the usage of an undefined attr. One option is to reject the search completely, the other to use a NULL idl for the cpmponent, but this could give incorrect or unexpected results

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-05-14 14:32:07

Currently DS retrieves candidates and is able to evaluate a filter even when it contains unknown attributes

ldapsearch -LLL -h localhost -p 38901 -b "dc=example,dc=com" "(&(dc=example)(tofu=simple))"
dn: dc=example,dc=com
objectClass: top
objectClass: domain
objectClass: extensibleobject
dc: example
tofu: simple
roomNumber: 123
[14/May/2019:14:22:32.664197954 +0200] conn=11 fd=64 slot=64 connection from 127.0.0.1 to 127.0.0.1                             
[14/May/2019:14:22:32.664556326 +0200] conn=11 op=0 BIND dn="cn=directory manager" method=128 version=3
[14/May/2019:14:22:32.693435802 +0200] conn=11 op=0 RESULT err=0 tag=97 nentries=0 etime=0.0029173711 dn="cn=directory manager"
[14/May/2019:14:22:32.693642812 +0200] conn=11 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(&(dc=example)(tofu=simple))" attrs=ALL
[14/May/2019:14:22:32.694794465 +0200] conn=11 op=1 RESULT err=0 tag=101 nentries=1 etime=0.0001318754 notes=U
[14/May/2019:14:22:32.695012480 +0200] conn=11 op=2 UNBIND
[14/May/2019:14:22:32.695029716 +0200] conn=11 op=2 fd=64 closed - U1

ldapsearch -LLL -h localhost -p 38901 -b "dc=example,dc=com" "(&(objectclass=extensibleobject)(tofu=simple))"
dn: dc=example,dc=com
objectClass: top
objectClass: domain
objectClass: extensibleobject
dc: example
tofu: simple
roomNumber: 123
[14/May/2019:14:22:54.640545081 +0200] conn=12 fd=64 slot=64 connection from 127.0.0.1 to 127.0.0.1
[14/May/2019:14:22:54.640815170 +0200] conn=12 op=0 BIND dn="cn=directory manager" method=128 version=3
[14/May/2019:14:22:54.667989100 +0200] conn=12 op=0 RESULT err=0 tag=97 nentries=0 etime=0.0027372701 dn="cn=directory manager"
[14/May/2019:14:22:54.668230759 +0200] conn=12 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(&(objectClass=extensibleobject)(tofu=simple))" attrs=ALL
[14/May/2019:14:22:54.668604674 +0200] conn=12 op=1 RESULT err=0 tag=101 nentries=1 etime=0.0000528564
[14/May/2019:14:22:54.668779805 +0200] conn=12 op=2 UNBIND
[14/May/2019:14:22:54.668815054 +0200] conn=12 op=2 fd=64 closed - U1

What is the benefit of changing this behavior ?

A concern could be for filter evaluation that relies on a "default" matching rule that could give unexpected result

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-05-14 14:48:58

I think Williams concern is that you can have search filters like: "(unknown=xxx)" where unknown is not in the schema. It will not have an index and so it will be allids and have to evaluate all entries. Since it is not in the schema we know that there can be no entry matching the filter (assuming schema check enabled) and reject the search earlier.

but @tbordaz I think you have point, extensible objects should allow attributes which are not in the objectclass, but currently we allow any attribute, even undefined attrs. So, if we reject these, we will need to do some cleanup (I think especially in plugins) or we will hurt ourselves when rejecting them

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-05-14 15:12:00

Isnt't all this what nsslapd-require-index is for?

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-05-14 15:28:00

yes, there is require-index or lookthroughlimit which can prevent a bad impact. And if you allow unindexed searches the kind of "DOS" can be done with any unindexed attr which is in the schema.

So I do not see a real danger of DOS, but on the other hand I would recommend to have a database where all used attributes should be in the schema and all entries should confirm to the schema. In that case we can detect "impossible" searches, accidently or with bad intent, early and avoid search processing.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-05-14 15:38:50

yes, there is require-index or lookthroughlimit which can prevent a bad impact. And if you allow unindexed searches the kind of "DOS" can be done with any unindexed attr which is in the schema. So I do not see a real danger of DOS, but on the other hand I would recommend to have a database where all used attributes should be in the schema and all entries should confirm to the schema. In that case we can detect "impossible" searches, accidently or with bad intent, early and avoid search processing.

Yeah, that sounds good and I do get the point of this ticket, but my main concern is the performance impact. We need to take some kind of lock to read the schema, and we need to do it for each filter component for every search. Seems like a potential bottle neck to me. So I only request that we get search performance numbers from before and after this fix to make sure there are no significant performance regressions.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-15 02:31:10

So to summarise:

@elkris's first comment: I think all your cases are covered by having policy of "off, warning, strict", where "off" is current behaviour, warning is "if the attr is not in the schema, we return a zero IDL" and strict is "outright reject the filter". This covers the extensibleObject case (off), the AD attr case ("warning").

I have actually seen this cause a DOS at UofA in the past - which did in fact become a RH support case. An appliance was searching attributes that didn't exist, and causing all threads to be blocked and exhausted. Eventually, we added a fake index for the non-existing attribute. This is because a search of (a=a) will become a full table scan. You correctly point out the idlscanlimit will kick in, however, the problem with lowering idlscanlimit is that then legitimate searches like (&(objectClass=person)(memberOf=student)) would be limited and also become full table scans when we should have been using the indexes - which we were unable to use as a mechanism to resolve the issue either (it basically broke mail routing due to the need to do distribution groups to large sets of users in the university). I've asserted before that I think the idlscanlimit is an attempt to resolve a problem which is actually solved by query validation and filter optimisation.

Finally, an important point of this ticket is that IPA was emitting an invalid query, for an attribute that never existed. We should never have let this go silent, and at least, should have warned that this behaviour was going to result in things you probably don't want or expect to happen.

So to address @mreynolds389 concern - the schema is behind a RWlock, not a mutex, which means provided the schema is not changing, then this is a very low cost operation to perform. Certainly we can check to see if there is a major regression in performance though. We could additionally change this to a pthread rwlock, which has shown better performance characteristics in the past (but we will need to be careful to potentially change the policy to writer favouring to avoid starvation in write situations like replication).

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-05-22 02:43:10

Metadata Update from @Firstyear:

• Issue assigned to Firstyear

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-06-18 11:29:50

Metadata Update from @tbordaz:

• Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1721425

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-06-18 11:29:50

Issue linked to Bugzilla: Bug 1721425

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-06-20 17:46:53

Metadata Update from @mreynolds389:

• Issue set to the milestone: 1.4.1

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-08-29 01:54:39

Metadata Update from @Firstyear:

• Issue close_status updated to: fixed
• Issue status updated to: Closed (was: Open)

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-10-09 15:07:57

There is a regression with tis patch, see: BZ https://bugzilla.redhat.com/show_bug.cgi?id=1759709

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-11-08 19:59:57

regression fixed upstream b7d11180552b495ec34079a5578054c4d5e89473

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-01-23 11:49:40

regression fixed 1.4.1 9dc085433..0adfacd47 389-ds-base-1.4.1

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2020-06-03 08:10:49

Just for recording, original PR was #3438