#51245 Ticket 51244 - On ADD replication URP issue internal searches with filter containing unescaped chars
Closed 2 years ago by spichugi. Opened 2 years ago by tbordaz.
tbordaz/389-ds-base ticket_51244  into  master

@@ -7,6 +7,7 @@ 

  # --- END COPYRIGHT BLOCK ---

  #

  import pytest

+ import logging

  from lib389.replica import Replicas

  from lib389.tasks import *

  from lib389.utils import *
@@ -578,6 +579,68 @@ 

      for i in range(21,25):

          test_user.add('description', 'value {}'.format(str(i)))

  

+ @pytest.mark.ds51244

+ def test_urp_trigger_substring_search(topo_m2):

+     """Test that a ADD of a entry with a '*' in its DN, triggers

+     an internal search with a escaped DN

+ 

+     :id: 9869bb39-419f-42c3-a44b-c93eb0b77667

+     :setup: MMR with 2 masters

+     :steps:

+         1. enable internal operation loggging for plugins

+         2. Create on M1 a test_user with a '*' in its DN

+         3. Check the test_user is replicated

+         4. Check in access logs that the internal search does not contain '*'

+     :expectedresults:

+         1. Should succeeds

+         2. Should succeeds

+         3. Should succeeds

+         4. Should succeeds

+     """

+     m1 = topo_m2.ms["master1"]

+     m2 = topo_m2.ms["master2"]

+ 

+     # Enable loggging of internal operation logging to capture URP intop

+     log.info('Set nsslapd-plugin-logging to on')

+     for inst in (m1, m2):

+         inst.config.loglevel([AccessLog.DEFAULT, AccessLog.INTERNAL], service='access')

+         inst.config.set('nsslapd-plugin-logging', 'on')

+         inst.restart()

+ 

+     # add a user with a DN containing '*'

+     test_asterisk_uid = 'asterisk_*_in_value'

+     test_asterisk_dn = 'uid={},{}'.format(test_asterisk_uid, DEFAULT_SUFFIX)

+ 

+     test_user = UserAccount(m1, test_asterisk_dn)

+     if test_user.exists():

+         log.info('Deleting entry {}'.format(test_asterisk_dn))

+         test_user.delete()

+     test_user.create(properties={

+         'uid': test_asterisk_uid,

+         'cn': test_asterisk_uid,

+         'sn': test_asterisk_uid,

+         'userPassword': test_asterisk_uid,

+         'uidNumber' : '1000',

+         'gidNumber' : '2000',

+         'homeDirectory' : '/home/asterisk',

+     })

+ 

+     # check that the ADD was replicated on M2

+     test_user_m2 = UserAccount(m2, test_asterisk_dn)

+     for i in range(1,5):

+         if test_user_m2.exists():

+             break

+         else:

+             log.info('Entry not yet replicated on M2, wait a bit')

+             time.sleep(2)

+ 

+     # check that M2 access logs does not "(&(objectclass=nstombstone)(nscpentrydn=uid=asterisk_*_in_value,dc=example,dc=com))"

+     log.info('Check that on M2, URP as not triggered such internal search')

+     pattern = ".*\(Internal\).*SRCH.*\(&\(objectclass=nstombstone\)\(nscpentrydn=uid=asterisk_\*_in_value,dc=example,dc=com.*"

+     found = m2.ds_access_log.match(pattern)

+     log.info("found line: %s" % found)

+     assert not found

+ 

  

  if __name__ == '__main__':

      # Run isolated

@@ -1411,9 +1411,12 @@ 

      Slapi_Entry **entries = NULL;

      Slapi_PBlock *newpb;

      char *basedn = slapi_entry_get_ndn(entry);

+     char *escaped_basedn;

      const Slapi_DN *suffix = slapi_get_suffix_by_dn(slapi_entry_get_sdn (entry));

+     escaped_basedn = slapi_filter_escape_filter_value("nscpentrydn", basedn);

  

-     char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", basedn);

+     char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", escaped_basedn);

+     slapi_ch_free((void **)&escaped_basedn);

please use slapi_ch_free_string() here too! Thanks!

      newpb = slapi_pblock_new();

      slapi_search_internal_set_pb(newpb,

                                   slapi_sdn_get_dn(suffix), /* Base DN */
@@ -1602,12 +1605,15 @@ 

      Slapi_Entry **entries = NULL;

      Slapi_PBlock *newpb;

      const char *basedn = slapi_sdn_get_dn(parentdn);

+     char *escaped_basedn;

+     escaped_basedn = slapi_filter_escape_filter_value("nscpentrydn", basedn);

  

      char *conflict_csnstr = (char*)slapi_entry_attr_get_ref((Slapi_Entry *)entry, "conflictcsn");

      CSN *conflict_csn = csn_new_by_string(conflict_csnstr);

      CSN *tombstone_csn = NULL;

  

-     char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", basedn);

+     char *filter = slapi_filter_sprintf("(&(objectclass=nstombstone)(nscpentrydn=%s))", escaped_basedn);

+     slapi_ch_free((void **)&escaped_basedn);

please use slapi_ch_free_string()

      newpb = slapi_pblock_new();

      char *parent_dn = slapi_dn_parent (basedn);

      slapi_search_internal_set_pb(newpb,

@@ -130,6 +130,27 @@ 

      return ptr;

  }

  

+ /* Escaped an equality filter value (assertionValue) of a given attribute

+  * Caller must free allocated escaped filter value

+  */

+ char *

+ slapi_filter_escape_filter_value(char* filter_attr, char *filter_value)

+ {

+     char *result;

+     struct slapi_filter *f;

+ 

+     if ((filter_attr == NULL) || (filter_value == NULL)) {

+         return NULL;

+     }

+     f = (struct slapi_filter *)slapi_ch_calloc(1, sizeof(struct slapi_filter));

+     f->f_choice = LDAP_FILTER_EQUALITY;

+     f->f_un.f_un_ava.ava_type = filter_attr;

+     f->f_un.f_un_ava.ava_value.bv_len = strlen(filter_value);

+     f->f_un.f_un_ava.ava_value.bv_val = filter_value;

+     result = filter_escape_filter_value(f, FILTER_EQ_FMT, FILTER_EQ_LEN);

+     slapi_ch_free((void**) &f);

+     return result;

+ }

  

  /*

   * get_filter_internal(): extract an LDAP filter from a BerElement and create

@@ -5264,6 +5264,7 @@ 

  int slapi_filter_compare(struct slapi_filter *f1, struct slapi_filter *f2);

  Slapi_Filter *slapi_filter_dup(Slapi_Filter *f);

  int slapi_filter_changetype(Slapi_Filter *f, const char *newtype);

+ char *slapi_filter_escape_filter_value(char* filter_attr, char *filter_value);

  

  int slapi_attr_is_last_mod(char *attr);

  

Bug description:
In MMR a consumer receiving a ADD has to do some checking based on basedn.
It checks if the entry was a tombstone or if the conflicting parent entry was a tombstone.

To do this checking, URP does internal searches using basedn.
A '*' (ASTERISK) is valid in a RDN and in a DN. But using a DN in an assertionvalue of a filter, the ASTERISK needs to be escaped else the server will interprete the filtertype to be a substring. (see
https://tools.ietf.org/html/rfc4515#section-3)

The problem is that if a added entry contains an ASTERISK in the DN, it will not be escaped in internal search and trigger substring search (likely unindexed).

Fix description:
escape the DN before doing internal search in URP

https://pagure.io/389-ds-base/issue/51244

Reviewed by: ?

Platforms tested: F31

Code looks good, but I thought you could have used: filter_escape_filter_value() Maybe not?

rebased onto d7c5cc5

2 years ago

@mreynolds thanks for reviewing.

Thanks for the tip. It took me some time to make it work using filter_escape_filter_value but I think it is the right approach. I updated the patch.

rebased onto fa025b8

2 years ago

@mreynolds, I rebase the PR. do you mind to review it again :)

This seems clearer, good job. I'll leave it to @mreynolds to finish review though :)

please use slapi_ch_free_string() here too! Thanks!

Two minor issues, but ack!! Yeah this code looks a lot better using the filter escape functions, thanks!

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4298

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

2 years ago