Univention Bugzilla – Bug 41005
zoneName=../../../../../etc/shadow,cn=dns,dc=foo
Last modified: 2021-06-23 07:29:07 CEST
I just created a DNS forward zone in LDAP which triggered the listener to overwrite /etc/shadow. dn: zoneName=../../../../../etc/shadow,cn=dns,dc=foo objectClass: top objectClass: dNSZone objectClass: univentionObject univentionObjectType: dns/forward_zone dNSTTL: 10800 relativeDomainName: @ zoneName: ../../../../../etc/shadow aRecord: 192.168.0.113 nSRecord: xen3.school.local. nSRecord: slave114.school.local. sOARecord: xen3.school.local. root.school.local. 157 28800 7200 604800 10800
If I remove the zone from LDAP the /etc/shadow file will be removed by the listener, too.
See also Bug #36775 - maybe this can be prevented here, too.
Advisory: univention-bind.yaml
Created attachment 7788 [details] patch The fix looks good. I would have used urlencoding instead of this injective-implementation. There are some race conditions in the code when changing owner or permissions of the file. Attached a patch which resolves them, too.
(In reply to Florian Best from comment #4) > Created attachment 7788 [details] > patch The patch contains a little typo: bind.py:79: undefined name 'filename'
(In reply to Janek Walkenhorst from comment #3) > Advisory: univention-bind.yaml Why do you run a 'univention-directory-listener-ctrl resync bind' in postinst? Is it necessary? The sanitize_filename code looks complicated. Wouldn't it be better simply to check the realpath of the filename and to ignore everything what is not in the base folder? For example: >>> import os >>> BASE='/var/cache/bind' >>> path_a = 'foo.bar' >>> path_b = '../../foo2.bar2' >>> full_path_a = os.path.join(BASE, path_a) >>> full_path_b = os.path.join(BASE, path_b) >>> os.path.realpath(full_path_a) '/var/cache/bind/foo.bar' >>> os.path.realpath(full_path_b) '/var/foo2.bar2' >>> Otherwise we may use a standard function like Florian mentioned?
Reverted changes in UCS 4.1-2.
If you use filename = urlencode(zone_dn) instead of only the zone name which may exists twice this would also fix Bug #26683.
For Bug 33846 I simply used this: zoneFile = "%s.zone" % (zone,) zoneFile = os.path.normpath('/' + zoneFile).lstrip('/') zoneFile = os.path.join(PROXY_CACHE_DIR, zoneFile)
I think there are 3 distinct properties we should strive to achieve: a) Readability: "vier-eins-qa.test" should map to "vier-eins-qa.test.zone" instead of e.g. "766965722d65696e732d71612e74657374.zone" to not unnecessarily confuse the human operator. a1) Compatibility: The current file names for (ideally all) zone files should stay the same. This, I think, is not completely achievable (after all, fixing the bug requires changing the name of at least some files) a2) Usability: The files names should not contain shell-funny-characters, e.g. "!" or "$" or " " and the like. b) Injectivity: <https://en.wikipedia.org/wiki/Injective_function> The mapping should be injective, i.e. all distinct zone names should be mapped to distinct file names; Overwriting other zone files would be bad. c) Totality: <https://en.wikipedia.org/wiki/Partial_function> The mapping from zone name to file name should be total, i.e. all zone names in the LDAP directory should lead to configuration files in the BIND file system directory. All that of course in addition to the properties required to actually fix the problem: P) file name must not contain "/" That byte is invalid in file names (and makes it a path). Q) file name must not contain "\x00" That byte is invalid in file name. R) file name must not be "." That name is reserved. S) file name must not be ".." That name is reserved. T) file name should not start with "." Hidden files confuse humans. U) file name must only use bytes safe for the BIND .proxy file This is a difficult one as I could not find any documentation specifying the syntax of the BIND configuration files, thus we need to be conservative. U1) file name must not contain '"' That byte delimits the file name in the BIND configuration files. X) file name must not be empty At least one byte is required for a valid file name. (In reply to Stefan Gohmann from comment #6) > The sanitize_filename code looks complicated. I hope that impression is not because I made the code longer (44 lines) because in an effort to make the code understandable. E.g. one could remove 9 lines of asserts, but they are there to document the context/requirements for which the different sets of bytes where chosen. (reduction to 35 lines) One could remove 10 lines of comments. (reduction to 26 lines) All in all, one could reformat the code to reduce it to 19 lines, but I think this would just reduce maintainability (In reply to Janek Walkenhorst from comment #3) > Advisory: univention-bind.yaml Has: a a1? a2 b c P Q R S T U? U1 X Missing: ∅ (In reply to Florian Best from comment #4) > The fix looks good. I would have used urlencoding instead of this > injective-implementation. Has: a a1? a2? b c P Q U? U1 X Missing: R? S? T? Using percent encoding seems like a possible solution; I originally discarded similar solutions like base64 and hex due to them missing property a, I did not consider percent encoding. I do not quite like having three bytes per escaped byte, after all a file name can only be 255 bytes long; But this is probably only cosmetic. (In reply to Stefan Gohmann from comment #6) > The sanitize_filename code looks complicated. Wouldn't it be better simply > to check the realpath of the filename and to ignore everything what is not > in the base folder? Has: P Q R? S? a a1 b X Missing: T U U1 a2 c (In reply to Arvid Requate from comment #9) > For Bug 33846 I simply used this: > > zoneFile = "%s.zone" % (zone,) > zoneFile = os.path.normpath('/' + zoneFile).lstrip('/') > zoneFile = os.path.join(PROXY_CACHE_DIR, zoneFile) Has: a P R S X Missing: a1 a2 b c? Q T U U1 (In reply to Stefan Gohmann from comment #6) > Why do you run a 'univention-directory-listener-ctrl resync bind' in > postinst? Is it necessary? Yes, due to property a1 not being completely fulfill-able some file names will change and thus need to be rewritten. One could of course look for code to migrate only the files that need it.
Created attachment 9117 [details] sanitizing.patch A patch to solve the problem by sanitizing (using urlencoding)
Created attachment 9118 [details] rejection.patch A patch to solve the problem by rejecting "bad" inputs.
The good news: # udm dns/forward_zone create --set nameserver="$(hostname -f)" --set contact="root@$(dnsdomainname)" --set zone=../../../../proc/sysrq-trigger E: Invalid Syntax: Zone name: Labels must be between 1 and 63 characters long! The bad news: Using the low-level LDAP tools directly it is still possible # eval "$(ucr shell hostname domainname ldap/base)" # ldapadd -Y EXTERNAL -H ldapi:/// <<__LDIF__ dn: zoneName=../../../../var/lib/univention-ldap/ldap/data.mdb,cn=dns,${ldap_base} nSRecord: ${hostname}.${domainname}. objectClass: dNSZone objectClass: top dNSTTL: 10800 relativeDomainName: @ zoneName: ../../../../var/lib/univention-ldap/ldap/data.mdb sOARecord: ${hostname}.${domainname}. root.${domainname}. 9 28800 7200 604800 10800 __LDIF__ But you already need LDAP admin credentials, so you can do arbitrary things in LDAP already. This is still an security issue if you delegate DNS administration to a user via UMC. Looking at the code we could do something like this: try: _validated_zonename = univention.admin.syntax.dnsZone.parse(zonename) except univention.admin.uexceptions.valueError: return # refuse invalid DNS zone names PS: The UDL modules postrun() logic breaks if you create a zone in TLD '.proxy': 298 »···»···»···»···if not f.endswith('.proxy'):
This issue has been filled against UCS 4.1. The maintenance with bug and security fixes for UCS 4.1 has ended on 5st of April 2018. Customers still on UCS 4.1 are encouraged to update to UCS 4.3. Please contact your partner or Univention for any questions. If this issue still occurs in newer UCS versions, please use "Clone this bug" or simply reopen the issue. In this case please provide detailed information on how this issue is affecting you.
(In reply to Philipp Hahn from comment #13) > Looking at the code we could do something like this: > try: > _validated_zonename = univention.admin.syntax.dnsZone.parse(zonename) > except univention.admin.uexceptions.valueError: > return # refuse invalid DNS zone names It's not restrictive enough and it prevents overwriting the syntax via UCR: >>> univention.admin.syntax.dnsZone.parse('foo"\x00.zone') 'foo"\x00.zone' Also importing UDM might not be good. > PS: The UDL modules postrun() logic breaks if you create a zone in TLD '.proxy': > 298 »···»···»···»···if not f.endswith('.proxy'): I am forbidding zone names which end with .proxy and .zone now. Following fixes have been done: * added base dir limitation to the specific directories. zones are ignored if they are outside. * validate the zone name, so that invalid names cannot crash named/bind: disallowed: ** null-byte ** " ** {0,127,256}.in-add.arpa ** . ** .. anywhere in the name ** zones ending with .proxy or .zone ** control characters * fix urlencoding of the password and ldap hostdn when inserting into the ldap-uri (e.g. password containing "," or password file ending with \n breaks) * fix zone name like "foo.zone.bar" univention-bind (13.0.1-3) dec0e02a4ed2 | Bug #41005: Merge branch 'fbest/41005-zone-name-path-injection' into 4.4-0 42e446896dcc | Bug #41005: fix path injection vulnerable in bind listener acfaaf28cab0 | Bug #41005: prevent {0,127,255}.in-addr.arpa zones crashing bind/named 5b31c6bb2fd0 | Bug #41005: validate zone name 53e285eeb91e | Bug #41005: add basedir restriction 0c9e38e45bfc | Bug #41005: fix urlencoding of password and password files ending with \n 61592edb1282 | Bug #41005: fix zone names containing ".zone"
Ohh, and I commmited a test case (67_udm-dns/110_dns_forward_zone_name_validation.py) which does various things which break bind. Therefor I did not enable it yet, will do it when the erratum is released.
Created attachment 9996 [details] qa.diff Ok, looks good, two small suggestions attached. Also, I guess the character '"' could be allowed if validate_zonename would zonemame.replace('"','\\"'). At least named-checkconf seemed to be ok with zone "\"". But I don't insist on that.
Thanks, patch plus additional quoting of " added.
Verified: * Code review * Functional test * Advisory
I also removed two useless lines: /var/cache/univention-bind-proxy/ only contains files which end with ".zone".
Ok.
<http://errata.software-univention.de/ucs/4.4/68.html>