[pve-devel] [PATCH access-control] ldap: fix ldap distinguished names regex

[Stefan Sterz <s.sterz@proxmox.com>]

according to the current specification of the string representation of
ldap distinguished names (DN) presented by RFC 4514 [1] the current
regex checking ldap DNs still prevents users from entering valid
DNs.

for example we do not allow multi-valued RelativeDistinguishedNames as
these are separated by a '+'. this was already reported as an issue in
the subscription support.

escaped sequences in unquoted AttributeValues are also not allowed.
this includes commas (',') and quotes ('"'). for example, the
following does not work even though the RFC explicitly mentions it as
a valid example ([1], section 4):

CN=James \"Jim\" Smith\, III,DC=example,DC=net

while this may not be usable as a valid username, as argued
previously [2], it seems arbitrary to allow this for quoted
AttributeValues but not for unquoted ones.

this commit also adds a test file that tests the regex against a
number of common pitfalls. including distinguished names that are
structurally similar to those reported as erroneously forbidden
earlier. these tests should help avoid regressions in the future.

[1]: https://datatracker.ietf.org/doc/html/rfc4514
[2]: https://git.proxmox.com/?p=pve-access-control.git;h=1aa2355a

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
would really appreciate another pair of eyes here. given the recent
churn related to this regex. it's very likely i missed something too.

 src/PVE/Auth/LDAP.pm      |  9 +++++--
 src/test/Makefile         |  1 +
 src/test/dn-regex-test.pl | 54 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)
 create mode 100755 src/test/dn-regex-test.pl

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index fc82a17..ccc6864 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -10,8 +10,13 @@ use PVE::Tools;
 
 use base qw(PVE::Auth::Plugin);
 
-my  $dn_part_regex = qr!("[^"]+"|[^ ,+"/<>;=#][^,+"/<>;=]*[^ ,+"/<>;=]|[^ ,+"/<>;=#])!;
-our $dn_regex = qr!\w+=${dn_part_regex}(,\s*\w+=${dn_part_regex})*!;
+my $escaped  = qr!\\(?:[ "#+,;<=>\\]|[0-9a-fA-F]{2})!;
+my $start    = qr!(?:${escaped}|[^"+,;<>\\\0 #])!;
+my $middle   = qr!(?:${escaped}|[^"+,;<>\\\0])!;
+my $end      = qr!(?:${escaped}|[^"+,;<>\\\0 ])!;
+my $attr_val = qr!("[^"]+"|${start}(?:${middle}*${end})?)!;
+
+our $dn_regex = qr!\w+=${attr_val}([,\+]\s*\w+=${attr_val})*!;
 
 sub type {
     return 'ldap';
diff --git a/src/test/Makefile b/src/test/Makefile
index 859a84b..b4c05eb 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -13,3 +13,4 @@ check:
 	perl -I.. perm-test7.pl
 	perl -I.. perm-test8.pl
 	perl -I.. realm_sync_test.pl
+	perl -I.. dn-regex-test.pl
diff --git a/src/test/dn-regex-test.pl b/src/test/dn-regex-test.pl
new file mode 100755
index 0000000..a2410af
--- /dev/null
+++ b/src/test/dn-regex-test.pl
@@ -0,0 +1,54 @@
+
+use strict;
+use warnings;
+
+use PVE::Auth::LDAP;
+
+# this test file attempts to check whether the ldap distinguished name regex
+# defined in `PVE::Auth::LDAP` complies with RFC 4514.
+# 
+# see: https://datatracker.ietf.org/doc/html/rfc4514
+
+my @pass = (
+    "ou=a",			# single AttributeTypeValue 
+    "ou=orga,dc=com,cn=name",	# multiple RelativeDistinguishedNames
+    "STREET=a,cn=a,C=c",	# single character AttributeValues
+    "UID=tt,cn=\"#+,;<>\\ \"",	# forbidden characters are allowed when quoted
+    "c=\\\"\\#\\+\\;\\<\\=\\>",	# specific characters allowed when escaped
+    "a=\\\\",			# escaped backslashes are allowed
+    "ST=a,cn=\"Test, User\"",	# allow un-escaped commas in quoted AttributeValues
+    "o2u=bc,cn=Test\\, User",	# allow escaped commas
+    "T2=a #b",			# spaces (' ') and '#' are allowed in the middle of AttributeValues
+    "word4word=ab#",		# allow '#' at the end of an AttributeValue
+    "ou=orga+sub=ab",		# allow '+' as separators for multi-valued RelativeDistinguishedName
+    "dc=\\f0\\Ac\\93",		# allow escaping hex values in unquoted AttributeValues
+
+    # regression tests
+    "ou=adf-bd,dc=abcd+efOuId=BL:BL:sldkf:704004,dc=or,dc=com",
+    "gvGid=DE:8A:wordCaps,ou=Service,dc=alsdkj+abOuId=UK:A8:137100,dc=edu,dc=de",
+);
+
+my @fail = (
+    "",				# no empty distinguished name
+    "ou=a,",			# no empty AttributeTypeAndValue
+    "ou=a+",			# no multi-valued RelativeDistinguishedName with empty second part
+    "ou",			# missing separator and AttributeValue
+    "ou=",			# no 'empty' AttributeValue
+    "ou=+",			# forbidden character '+' in AttributeValue
+    "ou= or",			# no space at the beginning of an AttributeValue
+    "ou=orgs ",			# no space at the end of an AttributeValue
+    "ou=#value",		# no '#' at the beginning an AttributeValue
+    "ou=\"+,;<>\\\0",		# no un-escaped forbidden characters in unquoted AttributeValues
+    "ou=name\0",		# no null value in AttributeValue
+    "ou=zy\\xw\\v"		# no unescaped backslashes that are not escaping specific characters
+);
+
+for my $test_case (@pass) {
+    die "\'" . $test_case . "\' unexpectedly did not match the ldap dn_regex\n"
+	if !($test_case =~ m/^$PVE::Auth::LDAP::dn_regex$/);
+}
+
+for my $test_case (@fail) {
+    die "\'" . $test_case . "\' unexpectedly did match the the ldap dn_regex\n"
+	if $test_case =~ m/^$PVE::Auth::LDAP::dn_regex$/;
+}
-- 
2.30.2