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 371A89A494 for ; Wed, 17 May 2023 15:40:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0EE9628484 for ; Wed, 17 May 2023 15:39:44 +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:39:43 +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 F218C45545 for ; Wed, 17 May 2023 15:39:42 +0200 (CEST) From: Stefan Sterz To: pve-devel@lists.proxmox.com Date: Wed, 17 May 2023 15:39:31 +0200 Message-Id: <20230517133931.148634-1-s.sterz@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.096 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 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [dn-regex-test.pl, perm-test8.pl, perm-test7.pl, proxmox.com, ldap.pm, ietf.org] Subject: [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:40:14 -0000 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$/; +} -- 2.30.2