From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7AE4E9A4A0 for ; Wed, 17 May 2023 15:42:20 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5EAE4284B8 for ; Wed, 17 May 2023 15:42:20 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 17 May 2023 15:42:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5B7B645543 for ; Wed, 17 May 2023 15:42:19 +0200 (CEST) Message-ID: Date: Wed, 17 May 2023 15:42:18 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 To: pve-devel@lists.proxmox.com References: <20230517133931.148634-1-s.sterz@proxmox.com> Content-Language: en-US From: Stefan Sterz In-Reply-To: <20230517133931.148634-1-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.238 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -2.666 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH access-control] ldap: fix ldap distinguished names regex X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 May 2023 13:42:20 -0000 sorry just noticed i forgot: adding tests for this was Suggested-by: Dominik Csapak 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 > --- > 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$/; > +}