Univention Bugzilla – Bug 32086
LDAP Filter / DN's aren't escaped in S4 Connector
Last modified: 2017-04-13 16:12:04 CEST
Ticket#2013072521001368 I have seen the following traceback (+ reject) after ad-takeover, the search filter was: filter: u'(&(objectclass=posixGroup)(cn=TEAMLEITER (Mailverteiler))) --- 25.07.2013 18:32:21,622 LDAP (ERROR ): unexpected Error during s4.resync_rejected 25.07.2013 18:32:21,623 LDAP (ERROR ): Traceback (most recent call last): File "/usr/lib/pymodules/python2.6/univention/s4connector/s4/__init__.py", line 1951, in resync_rejected mapped_object = self._object_mapping(property_key,object) File "/usr/lib/pymodules/python2.6/univention/s4connector/__init__.py", line 1575, in _object_mapping object=function(self, object, dn_mapping_stored, isUCSobject=(object_type == 'ucs')) File "/usr/lib/pymodules/python2.6/univention/s4connector/s4/__init__.py", line 341, in group_dn_mapping return samaccountname_dn_mapping(s4connector, given_object, dn_mapping_stored, isUCSobject, 'group', u'cn', u'posixGroup', 'cn', u'group') File "/usr/lib/pymodules/python2.6/univention/s4connector/s4/__init__.py", line 303, in samaccountname_dn_mapping base=s4connector.lo.base, scope='sub', attr=['objectClass']) File "/usr/lib/pymodules/python2.6/univention/s4connector/__init__.py", line 445, in search_ucs result = self.lo.search( filter = filter, base = base, scope = scope, attr = attr, unique = unique, required = required, timeout = timeout, sizelimit = sizelimit ) File "/usr/lib/pymodules/python2.6/univention/admin/uldap.py", line 348, in search raise univention.admin.uexceptions.ldapError, _err2str(msg) ldapError: Bad search filter --- After escaping the attribute (line 303 in /usr/share/pyshared/univention/s4connector/s4/__init__.py) a error message (invalid syntax for group name) was shown). - ucsdn_result = s4connector.search_ucs(filter=unicode(u'(&(objectclass=%s)(%s=%s))' % (ocucs, ucsattrib, samaccountname)) + ucsdn_result = s4connector.search_ucs(filter=unicode(u'(&(objectclass=%s)(%s=%s))' % (ocucs, ucsattrib, ldap.filter.escape_filter_chars(samaccountname)))
services/univention-s4-connector/modules/univention/s4connector/s4/__init__.py: 388 » » » » » » newdn = dn_attr + '=' + dn_attr_val + dn[pos2:] # guess the old dn 389 » » » » » else: 390 » » » » » » newdn = ucsattrib + '=' + samaccountname + dn[pos2:] # guess the old dn → dn_attr_val and samaccountname needs ldap.dn.escape_dn_chars().
Created attachment 8259 [details] Patch-series to fix LDAP (filter/DN) escaping in the s4connector The attached patch-series unifies and fixes the LDAP-filter and -DN escaping in the s4connector code. All LDAP-filter strings are escaped via `ldap.filter.escape_filter_chars` (indirectly using a new function `format_escaped()`. All LDAP-DN are de-/constructed via `ldap.dn.{str2dn,dn2str}()`. This also fixes https://forge.univention.org/bugzilla/show_bug.cgi?id=42716. This renders https://forge.univention.org/bugzilla/show_bug.cgi?id=39484 obsolete and would allow to delete the function `s4.explode_unicode_dn()` in combination with the cleanup in https://forge.univention.org/bugzilla/show_bug.cgi?id=43068. All s4connector Tests are passing on a single UCS 4.1-4 errate 332.
> 08_bug_#32086_s4connector_unify_ldap_operations_(dns.py).patch FAIL: Hunk 1: s4connector.lo_s4 is an instance of univention.admin.uldap.access. lo_s4.search() will never raise ldap.NO_SUCH_OBJECT but univention.admin.uexceptions.noObject. Please use lo_s4.get() as the search scope is base. Otherwise lo_s4.lo have to be used. HINT: Hunk 3 and 4: You can use getAttr() because 'dnsRecord' is the only interesting attribute > 10_bug_#32086#42716_s4connector_unify_ldap_dn_escaping_(dns.py).patch @@ -292,8 +287,12 @@ def dns_dn_mapping(s4connector, given_object, dn_mapping_stored, isUCSobject): The following lines were added: 104 +» » » » » new_rdn = [('DC', relativeDomainName, ldap.AVA_STRING)] 105 +» » » » » newdn = unicode(ldap.dn.dn2str([new_rdn] + ldap.dn.str2dn(zone_dn))) → I didn't check the source code if this is okay. > 14_bug_#32086_s4connector_unify_ldap_filter_escaping_(s4__init__.py).patch HUNK 3: You can use lo.getAttr here. All of the rest looks very very good!
Additionally I would also add the following patch. This will reveal places where the encoding is still broken: diff --git a/services/univention-s4-connector/modules/univention/s4connector/__init__.py b/services/univention-s4-connector/modules/univention/s4connector/__init__.py index b1ce6be..6f87fc5 100644 --- a/services/univention-s4-connector/modules/univention/s4connector/__init__.py +++ b/services/univention-s4-connector/modules/univention/s4connector/__init__.py @@ -877,8 +877,6 @@ def get_ucs_ldap_object_dn(self, dn): » » » » return self.lo.lo.lo.search_s(searchdn, ldap.SCOPE_BASE, '(objectClass=*)', ('dn',))[0][0] » » » except ldap.NO_SUCH_OBJECT: » » » » return None -» » » except ldap.INVALID_DN_SYNTAX: -» » » » return None » » » except ldap.INVALID_SYNTAX: » » » » return None » » » except (ldap.SERVER_DOWN, SystemExit): @@ -897,8 +895,6 @@ def get_ucs_ldap_object(self, dn): » » » » return self.lo.get(searchdn, required=1) » » » except ldap.NO_SUCH_OBJECT: » » » » return None -» » » except ldap.INVALID_DN_SYNTAX: -» » » » return None » » » except ldap.INVALID_SYNTAX: » » » » return None » » » except (ldap.SERVER_DOWN, SystemExit):
(In reply to Florian Best from comment #3) > s4connector.lo_s4 is an instance of univention.admin.uldap.access. s4connector.lo_s4 is actually an instance of univention.uldap.access, so this works. > The following lines were added: > 104 +» » » » » new_rdn = [('DC', relativeDomainName, > ldap.AVA_STRING)] > 105 +» » » » » newdn = unicode(ldap.dn.dn2str([new_rdn] + > ldap.dn.str2dn(zone_dn))) You might have been confused by the indentation. The following line existed on that indentation level before. newdn = 'DC=%s,%s' % (relativeDomainName, zone_dn) I applied the changes and will rerun the tests with modification proposed in https://forge.univention.org/bugzilla/show_bug.cgi?id=32086#c4. > All of the rest looks very very good! Thank you!
(In reply to Lukas Oyen from comment #5) > (In reply to Florian Best from comment #3) > > > s4connector.lo_s4 is an instance of univention.admin.uldap.access. > > s4connector.lo_s4 is actually an instance of univention.uldap.access, so > this works. Ah, yes. > You might have been confused by the indentation. The following line existed > on that indentation level before. Oh, yes. It might be good to add some tests with objects which have '='/'+' in their DN and ')' in their value.
*** Bug 42716 has been marked as a duplicate of this bug. ***
Created attachment 8271 [details] Patch-series to fix LDAP (filter/DN) escaping in the s4connector The patches reworked as proposed in comment 3. All s4connector tests are passing on a single UCS 4.1-4 errata 332 (also with the changes as proposed in comment 4). The changes in comment 4 are not part of the patch-series, as they were already commited as part of bug 31771.
Commited in 4.2 as r75098.
mapping.py is still unfixed.
(In reply to Florian Best from comment #10) > mapping.py is still unfixed. True. I omitted it, as most filters are constructed with `{conenctor/s4/}ldap/base`, but there are some, where filter expressions are constructed from other UCR variables.
Created attachment 8372 [details] Patch to fix LDAP filters in mapping.py Additional patch to fix escaping in LDAP filters in `mapping.py`. At this point this is a proof-of-concept, as a resolution for #43407 is needed. With this patch, the exact same mapping-file is generated as before.
34 -» print "»» » ignore_filter='(|%s)'," % ignore_filter 35 +» print "»» » ignore_filter='%s'," % ignore_filter → I would suggest here to use the following: print "»» » ignore_filter=%r," % (ignore_filter,) Otherwise if the variable contains a ' single-quote the whole file is broken / syntax error or one can inject python logic into it.
(In reply to Florian Best from comment #13) > 34 -» print "»» » ignore_filter='(|%s)'," % ignore_filter > 35 +» print "»» » ignore_filter='%s'," % ignore_filter > → I would suggest here to use the following: > print "»» » ignore_filter=%r," % (ignore_filter,) > > Otherwise if the variable contains a ' single-quote the whole file is broken > / syntax error or one can inject python logic into it. Right, I will update that with the other changes, after #43397 is fixed.
REOPEN: svn r75089 contains a mistake: » try: -» » searchResult = s4connector.lo_s4.lo.search_s(zone_dn, ldap.SCOPE_BASE, '(objectClass=*)', ['dn']) +» » _ = s4connector.lo_s4.getAttr(zone_dn, 'dn', required=True) » except ldap.NO_SUCH_OBJECT: » » __create_s4_forward_zone(s4connector, zone_dn) 'dn' is not a attribute and therefore ldap.NO_SUCH_OBJECT is raised which causes that the zone is going to be added again, causing the following traceback: 24.01.2017 18:14:04,875 LDAP (PROCESS): sync from ucs: [ dns] [ modify] DC=@,DC=autotest091.local,CN=MicrosoftDNS,DC=DomainDnsZones,DC=autotest091,DC=local 24.01.2017 18:14:04,888 LDAP (WARNING): sync failed, saved as rejected /var/lib/univention-connector/s4/1485299084.869911 24.01.2017 18:14:04,888 LDAP (WARNING): Traceback (most recent call last): File "/usr/lib/pymodules/python2.7/univention/s4connector/__init__.py", line 843, in __sync_file_from_ucs if ((old_dn and not self.sync_from_ucs(key, object, premapped_ucs_dn, unicode(old_dn, 'utf8'), old, new)) or (not old_dn and not self.sync_from_ucs(key, object, premapped_ucs_dn, old_dn, old, new))): File "/usr/lib/pymodules/python2.7/univention/s4connector/s4/__init__.py", line 2528, in sync_from_ucs self.property[property_type].con_sync_function(self, property_type, object) File "/usr/lib/pymodules/python2.7/univention/s4connector/s4/dns.py", line 1578, in ucs2con s4_zone_create_wrapper(s4connector, object) File "/usr/lib/pymodules/python2.7/univention/s4connector/s4/dns.py", line 854, in s4_zone_create_wrapper result = s4_zone_create(s4connector, object) File "/usr/lib/pymodules/python2.7/univention/s4connector/s4/dns.py", line 754, in s4_zone_create __create_s4_forward_zone(s4connector, zone_dn) File "/usr/lib/pymodules/python2.7/univention/s4connector/s4/dns.py", line 485, in __create_s4_forward_zone s4connector.lo_s4.lo.add_s(zone_dn, al) File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 202, in add_s return self.result(msgid,all=1,timeout=self.timeout) File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 465, in result resp_type, resp_data, resp_msgid = self.result2(msgid,all,timeout) File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 469, in result2 resp_type, resp_data, resp_msgid, resp_ctrls = self.result3(msgid,all,timeout) File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 476, in result3 resp_ctrl_classes=resp_ctrl_classes File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 483, in result4 ldap_result = self._ldap_call(self._l.result4,msgid,all,timeout,add_ctrls,add_intermediates,add_extop) File "/usr/lib/python2.7/dist-packages/ldap/ldapobject.py", line 106, in _ldap_call result = func(*args,**kwargs) ALREADY_EXISTS: {'info': 'Entry DC=autotest091.local,CN=MicrosoftDNS,DC=DomainDnsZones,DC=autotest091,DC=local already exists', 'desc': 'Already exists'} This causes a lot of rejects: http://jenkins.knut.univention.de:8080/job/UCS-4.2/job/UCS-4.2-0/job/AutotestJoin/21/SambaVersion=s4,Systemrolle=master/testReport/00_checks/10_s4_connector_rejects/test/
I added a hotfix by using entryDN instead. univention-s4-connector (11.0.5-1): r76079 | Bug #32086: dn is no attribute, search for entryDN attribute instead
Felix just assumed that you could also just ask for attributes=[] instead? Anyway, nice catch. Any idea why this didn't occur in UCS 4.1?
(In reply to Arvid Requate from comment #17) > Felix just assumed that you could also just ask for attributes=[] instead? > Anyway, nice catch. Any idea why this didn't occur in UCS 4.1? because we didn't commit the changes in UCS 4.1 :) Yes, probably just get(dn, attr=[]) is better, i just wanted the ucs-tests tomorrow to be green(er).
The replacement of lo.search_s() with getAttr() has side effects: getAttr() also raises NO_SUCH_OBJECT if the object exists but the attribute is not set at the object.
(In reply to Florian Best from comment #3) > > 14_bug_#32086_s4connector_unify_ldap_filter_escaping_(s4__init__.py).patch > HUNK 3: You can use lo.getAttr here. This was my bad suggestion, so I fixed it: univention-s4-connector (11.0.6-1): r76123 | Bug #32086: fix getAttr change in svn r75089
Created attachment 8401 [details] Patch to fix LDAP filters in mapping.py Updated version of the `mapping.py` ldap-escaping fixes. This is based on the fix for bug #43397.
Commited in r76453.
I fixed all the broken places in ucs-test s4connector.py: r76562 | Bug #32086: fix broken ldap dn escaping in s4 connector tests
REOPEN: services/univention-s4-connector/modules/univention/s4connector/s4/dns.py: 449 def __split_s4_dnsNode_dn(dn): 460 def __split_ol_dNSZone_dn(dn, objectclasses): services/univention-s4-connector/modules/univention/s4connector/__init__.py: 1434 » » » » # parent_dn = string.join(ldap.explode_dn(object_dn_mapped_to_ucr_ldap_base_case)[1:], ",") 1435 » » » » parent_dn = string.join(ldap.explode_dn(object['dn'])[1:], ",") → Still unfixed.
And some more: modules/univention/s4connector/__init__.py:» » return dn.split(',', 1)[0] modules/univention/s4connector/__init__.py:» » return dn.split(',', 1)[1] modules/univention/s4connector/s4/__init__.py:» while dn.find(',', last + 1) > 0: modules/univention/s4connector/s4/__init__.py:» » last = dn.find(',', last + 1) modules/univention/s4connector/s4/__init__.py:» » » pos = string.find(dn, '=') modules/univention/s4connector/s4/__init__.py:» » » » pos = string.find(dn, '=') modules/univention/s4connector/s4/__init__.py:» » » » last_found = dn.find('=', last_found) + 1 modules/univention/s4connector/s4/dns.py:» zoneName = string.join(dn[1].split('=')[1:], '=') modules/univention/s4connector/s4/dns.py:» relativeDomainName = string.join(dn[0].split('=')[1:], '=') modules/univention/s4connector/s4/dns.py:» » zoneName = string.join(dn[0].split('=')[1:], '=') modules/univention/s4connector/s4/dns.py:» » zoneName = string.join(dn[1].split('=')[1:], '=') modules/univention/s4connector/s4/dns.py:» » relativeDomainName = string.join(dn[0].split('=')[1:], '=')
(In reply to Florian Best from comment #24) > REOPEN: > > services/univention-s4-connector/modules/univention/s4connector/s4/dns.py: > 449 def __split_s4_dnsNode_dn(dn): > 460 def __split_ol_dNSZone_dn(dn, objectclasses): > > services/univention-s4-connector/modules/univention/s4connector/__init__.py: > 1434 » » » » # parent_dn = > string.join(ldap.explode_dn(object_dn_mapped_to_ucr_ldap_base_case)[1:], ",") > 1435 » » » » parent_dn = > string.join(ldap.explode_dn(object['dn'])[1:], ",") > > → Still unfixed. Fixed in r76628. Two other uses on `ldap.explode_dn` and subsequent split()/join() operations where also fixed. The other cases of find()/split()/join() where left untouched, as they are within dead code.
OK: Code review OK: manual tests with some objects OK: Changelog For ucs-test I cloned this to Bug #43598 as I had no time to convert every test case.
UCS 4.2 has been released: https://docs.software-univention.de/release-notes-4.2-0-en.html https://docs.software-univention.de/release-notes-4.2-0-de.html If this error occurs again, please use "Clone This Bug".