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

[Stefan Sterz]

sorry just noticed i forgot: adding tests for this was

Suggested-by: Dominik Csapak <d.csapak at proxmox.com>

On 17.05.23 15:39, Stefan Sterz wrote:
> 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 at 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$/;
> +}