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

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

[Markus Frank]

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@proxmox.com>
---
There does not seem to be a regex for LDAP attributes in pbs.
Should a regex be added for this?

 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\-]+$/) {
 	return $attr;
     }
 
-- 
2.39.2

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

[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@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.

>  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.
>
>   Other characters may be escaped.

>  	return $attr;
>      }
>  

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

[Thomas Lamprecht]

Am 15/11/2023 um 13:23 schrieb Markus Frank:
> 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@proxmox.com>
> ---
> There does not seem to be a regex for LDAP attributes in pbs.
> Should a regex be added for this?
> 
>  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\-]+$/) {

Pre-existing, but shouldn't the regex actually be?

$attr =~ m/^[a-zA-Z][a-zA-Z0-9\-]*$/

I.e., start with a letter and then be any of letter, digit or hyphen (minus).

CCing Christoph, you did a bit more LDAP stuff recently - opinions?

Re: [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@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.

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

[Stefan Sterz]

On 15.11.23 14:28, Stefan Sterz wrote:
>>  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:
> 

sorry just noticed i forgot to add:

[1]: https://datatracker.ietf.org/doc/html/rfc4514#section-2.4

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

[Stefan Sterz]

On 15.11.23 15:49, Thomas Lamprecht wrote:
> Am 15/11/2023 um 14:28 schrieb Stefan Sterz:
>> On 15.11.23 13:23, Markus Frank wrote:

-- >8 snip 8< --

>>
>>>  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.

just to through this out there, my last attempt at validating this [1]
looked something like this:

```
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})?)!;
```

since things can also be escaped or in quotes, which makes them valid
again. could probably be improved here, though.

[1]: https://lists.proxmox.com/pipermail/pve-devel/2023-May/056840.html

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

[Thomas Lamprecht]

Am 15/11/2023 um 16:12 schrieb Stefan Sterz:
> just to through this out there, my last attempt at validating this [1]
> looked something like this:
> 
> ```
> 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})?)!;
> ```
> 
> since things can also be escaped or in quotes, which makes them valid
> again. could probably be improved here, though.
> 
> [1]: https://lists.proxmox.com/pipermail/pve-devel/2023-May/056840.html

Thanks for the pointer, memories are comming back now..

It seems like `$attr =~ /^(.*)$/ && return $1` (if even needed to avoid
tainting) seems the more practical solution here...

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

[Christoph Heiss]

On Wed, Nov 15, 2023 at 02:30:06PM +0100, Thomas Lamprecht wrote:
>
> Am 15/11/2023 um 13:23 schrieb Markus Frank:
> > Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/"
> > to allow hyphen in ldap attribute names for pve & pmg.
> > [..]
> > --- 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\-]+$/) {
>
> Pre-existing, but shouldn't the regex actually be?
>
> $attr =~ m/^[a-zA-Z][a-zA-Z0-9\-]*$/
>
> I.e., start with a letter and then be any of letter, digit or hyphen (minus).
>
> CCing Christoph, you did a bit more LDAP stuff recently - opinions?

Sorry for the late reply, just saw this now.

I'd definitely agree with Stefan here, that moving away from regex's for
validating LDAP DNs/attributes/etc is the right way, instead of
continuously having to fix them up.
Even if we try to follow the RFCs as closely as possible, that does
unfortunaly still not really guarantee that it is indeed valid and will
be _accepted by the server_.

Just doing a basic sanity check (e.g. not empty, no spaces) and then
querying the actual LDAP server whether that accepts it or not would be
IMHO preferable. Plus, it's less work on our side in the long run.

PBS does it no differently too, so I'd go the same way here too. Being
overly strict does not help anyone, especially with LDAP, which does a
lot of weird things.