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

[Christoph Heiss <c.heiss@proxmox.com>]

On Wed, May 17, 2023 at 03:39:31PM +0200, Stefan Sterz wrote:
> [..]
>
> 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.
That's a really good idea :^)

>
> 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})*!;
While reviewing that, I had a look at the `Net::LDAP` perl library
again, if it provides a way to _somehow_ validate DNs properly without
having to resort to very fragile regexes.

=> `Net::LDAP` provides a canonical_dn() function in `Net::LDAP::Util`
   https://metacpan.org/pod/Net::LDAP::Util#canonical_dn

I quickly hacked up your test script to use canonical_dn() instead of
the regex. Works pretty much the same, except that it considers empty
strings and attributes with empty values as valid DNs and allows
whitespaces at both the beginning and the end of attribute values. But
these edge-cases can be much more easily checked than the whole thing
(esp. with escaping and such), and should be more robust.
[ I've attached the diff for the test script below for reference. ]

So IHMO that should be the way forward.

@Stefan: I'd be willing to properly rewrite the DN checking using
canonical_dn() - or do you want to?

> [..]
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

diff --git a/src/test/dn-regex-test.pl b/src/test/dn-regex-test.pl
index a2410af..9a48483 100755
--- a/src/test/dn-regex-test.pl
+++ b/src/test/dn-regex-test.pl
@@ -3,6 +3,7 @@ use strict;
 use warnings;

 use PVE::Auth::LDAP;
+use Net::LDAP::Util qw(canonical_dn);

 # this test file attempts to check whether the ldap distinguished name regex
 # defined in `PVE::Auth::LDAP` complies with RFC 4514.
@@ -33,10 +34,10 @@ my @fail = (
     "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=",			# 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= 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
@@ -44,11 +45,12 @@ my @fail = (
 );

 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$/);
+    die "'$test_case' unexpectedly failed to parse as valid DN\n"
+	if !canonical_dn($test_case);
 }

 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$/;
+    my $dn = canonical_dn($test_case);
+    die "'$test_case' unexpectedly parsed as valid DN: '$dn'\n"
+	if $dn;
 }