[pve-devel] [PATCH common] fix #5034 ldap attribute regex

[Thomas Lamprecht]

Am 15/11/2023 um 14:28 schrieb Stefan Sterz:
> On 15.11.23 13:23, Markus Frank wrote:
>> Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/"
>> to allow hyphen in ldap attribute names for pve & pmg.
>>
>> Signed-off-by: Markus Frank <m.frank at proxmox.com>
>> ---
>> There does not seem to be a regex for LDAP attributes in pbs.
>> Should a regex be added for this?
>>
> 
> we recently moved away from using regex for validating a LDAP
> configuration, for two reasons:
> 
> 1. turns out finding a regex that validates all possible valid LDAP DNs
>    is pretty hard and fixing this often comes along with breaking older
>    setups
> 2. even a valid *looking* DN may not actual work against a real LDAP
>    server.
> 
> so instead we now actually try to query an LDAP server with the provided
> config and see if that returns anything. thus, users are more likely to
> get what they want, and we don't have to validate for every possible case.
> 
> i guess there could be an argument why a regex here makes sense.
> however, i'd imagine it's a little odd for users that we are stricter
> here than we are with DNs.

thanks for your input (didn't see your reply before sending mine)

> 
>>  src/PVE/JSONSchema.pm | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>> index 49e0d7a..ef58b62 100644
>> --- a/src/PVE/JSONSchema.pm
>> +++ b/src/PVE/JSONSchema.pm
>> @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr', \&verify_ldap_simple_attr);
>>  sub verify_ldap_simple_attr {
>>      my ($attr, $noerr) = @_;
>>  
>> -    if ($attr =~ m/^[a-zA-Z0-9]+$/) {
>> +    if ($attr =~ m/^[a-zA-Z0-9\-]+$/) {
> 
> if i'm not mistaken, this regex should try to filter an `AttributeValue`
> [1]. in case we do stick with this regex approach here, you may want to
> relax this even further, as per the standard:
> 
>>  If that UTF-8-encoded Unicode string does not have any of the
>>  following characters that need escaping, then that string can be used
>>  as the string representation
>>  of the value.
>>
>>      - a space (' ' U+0020) or number sign ('#' U+0023) occurring at
>>        the beginning of the string;
>>
>>      - a space (' ' U+0020) character occurring at the end of the
>>        string;
>>
>>      - one of the characters '"', '+', ',', ';', '<', '>',  or '\'
>>        (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C,
>>        respectively);
>>
>>      - the null (U+0000) character.
>>

Ack, so I was wrong, the format might still make sense albeit checking
for above cases would then indeed better, something along the lines of:

if ($attr !~ /(?:^(?:\s|#))|["+,;<>\0\\]|(?:\s$)/) {
    return $attr;
}

If we leave that regex out completely we should ensure that we don't get
any tainting issues.

The format could move to PVE::Auth::LDAP too, FWIW, but that's a different
story.