Fix: Escape group names for LDAP #37201

AaronDewes · 2023-03-13T19:07:15Z

Summary

Nextcloud's LDAP feature allows to automatically generate queries so only certain groups are available in Nextcloud.

However, I noticed that groups may contain special characters (Like "(" or ")") that should be escaped to ensure generated queries are correct. Currently, the generated queries will be invalid and lead to 0 groups found if any groups contain such characters.

I originally encountered this issue when trying to get Nextcloud running with a MNS+ system, a very popular school managment system in Rhineland-Palatinate.

TODO

Are tests needed for this? There are no tests for this function at all, so I'm not sure if/how to write new ones.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

come-nc

It would make sense to escape all the other values which are concatenated into ldap filters in the function.

AaronDewes · 2023-03-14T06:42:09Z

It would make sense to escape all the other values which are concatenated into ldap filters in the function.

Yes, I fixed this now. I think these are all values that need escaping, but please let me know if I missed any.

Also, if this gets merged, can this be backported to Nextcloud 26(.0.2)?

From https://github.com/nextcloud/desktop/wiki/Backport-policy-stable-branches:

If we are doing a X.Y.2 (usually one month after the X.Y.1 release) we will backport trivial fixes even if the bug is not necessary critical but we are very confident that the fix will not introduce problems ;

Because this fix is pretty simple, and it seems like there is still some time left until Nextcloud 27, it would be great to get this backported, because it affects every Nextcloud version.

AaronDewes · 2023-03-14T06:57:23Z

I did not intend to remove the other two requests, this seems to be a bug in GitHub (I just clicked "Re-request review from @come-nc").

come-nc · 2023-03-14T08:59:56Z

Also, if this gets merged, can this be backported to Nextcloud 26(.0.2)?

I’m afraid it will be for 26.0.1, we are too close to 26 release to backport in time most likely.

apps/user_ldap/lib/Wizard.php

@@ -925,7 +925,7 @@
 						}
 						$base = $this->configuration->ldapBase[0];
 						foreach ($cns as $cn) {
-							$rr = $this->ldap->search($cr, $base, 'cn=' . $cn, ['dn', 'primaryGroupToken']);
+							$rr = $this->ldap->search($cr, $base, 'cn=' . ldap_escape($cn, '', LDAP_ESCAPE_FILTER), ['dn', 'primaryGroupToken']);


blizzz · 2023-03-15T11:29:30Z

we also have Access::escapeFilterPart() (not sure why i introduced this in 2014 and not the escape method back then, maybe i simply was not aware of it, or we supported PHP < 5.6).

come-nc · 2023-03-15T14:37:43Z

we also have Access::escapeFilterPart() (not sure why i introduced this in 2014 and not the escape method back then, maybe i simply was not aware of it, or we supported PHP < 5.6).

escapeFilterPart also has a special handling of the asterisk as first character, if allowed.
But I do think it would work as well to do that:

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index 1cc0c62ff1d..658de8c0b83 100644
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -1411,9 +1411,7 @@ class Access extends LDAPUtility {
                        $asterisk = '*';
                        $input = mb_substr($input, 1, null, 'UTF-8');
                }
-               $search = ['*', '\\', '(', ')'];
-               $replace = ['\\*', '\\\\', '\\(', '\\)'];
-               return $asterisk . str_replace($search, $replace, $input);
+               return $asterisk . ldap_escape($input, '', LDAP_ESCAPE_FILTER);
        }
 
        /**

AaronDewes · 2023-03-20T18:37:08Z

escapeFilterPart also has a special handling of the asterisk as first character, if allowed.

For this purpose, ldap_escape should be the correct method though, because the user selects these groups from a list and does not manually enter them (This menu could be improved imho, but for now, I think this is the best fix).

AaronDewes · 2023-04-28T05:46:05Z

@blizzz @ArtificialOwl is there anything I can do to Help this PR get merged? I'd really like to have this in the next release to save school IT admins in Rhineland-Palatinate some work at the beginning of the next school year.

blizzz · 2023-04-28T07:14:53Z

@AaronDewes from my PoV also fix the other occurrences as described in #37201 (comment) f

AaronDewes · 2023-04-28T07:30:06Z

I did that in f1b6ddc, did I miss any?

blizzz · 2023-04-28T08:36:14Z

In the Access class as pointed out.

Signed-off-by: Aaron Dewes <[email protected]>

AaronDewes · 2023-09-20T13:38:48Z

@joshtrichards @ArtificialOwl @blizz Can someone have a look again? This bug is really annoying to me, and it would be great to have it fixed soon.

Altahrim

Nit: maybe add a test with parenthesis as it was the original bug

AaronDewes · 2023-09-29T07:35:38Z

Nit: maybe add a test with parenthesis as it was the original bug

Parenthesis are correctly escaped by escapeFilterPart. Originally, this PR was just to make some generated LDAP filters use escapeFilterPart, all of which do not have tests yet at all AFAIK and I don't know how to implement myself.

welcome · 2023-10-02T07:49:25Z

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

come-nc · 2023-10-02T07:50:25Z

/backport stable27

come-nc · 2023-10-02T07:50:38Z

/backport stable26

come-nc · 2023-10-02T07:50:51Z

/backport to stable27

come-nc · 2023-10-02T07:50:59Z

/backport to stable26

backportbot-nextcloud · 2023-10-02T08:28:58Z

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

backportbot-nextcloud · 2023-10-02T08:29:01Z

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

…neration Fix: Escape group names for LDAP

szaimen added the 3. to review Waiting for reviews label Mar 13, 2023

szaimen added this to the Nextcloud 27 milestone Mar 13, 2023

szaimen requested review from blizzz, come-nc, a team and ArtificialOwl and removed request for a team March 13, 2023 19:19

come-nc reviewed Mar 13, 2023

View reviewed changes

AaronDewes requested review from come-nc and removed request for blizzz and ArtificialOwl March 14, 2023 06:56

come-nc approved these changes Mar 14, 2023

View reviewed changes

github-advanced-security bot found potential problems Mar 14, 2023

View reviewed changes

come-nc requested review from blizzz, a team and ArtificialOwl and removed request for a team March 14, 2023 16:34

blizzz added the bug label Mar 15, 2023

come-nc approved these changes May 2, 2023

View reviewed changes

skjnldsv mentioned this pull request May 3, 2023

27.0.0 beta 1 #38038

Merged

AaronDewes added 3 commits August 13, 2023 15:43

Escape some more values

13d9494

Signed-off-by: Aaron Dewes <[email protected]>

Simplify escapeFilterPart

1690899

Signed-off-by: Aaron Dewes <[email protected]>

Fix tests

d79def5

Signed-off-by: Aaron Dewes <[email protected]>

AaronDewes force-pushed the fix/ldap-filter-generation branch from 09c2ccc to d79def5 Compare August 13, 2023 13:43

joshtrichards added the feature: ldap label Sep 8, 2023

Merge branch 'master' into fix/ldap-filter-generation

aefa366

come-nc requested review from a team, Altahrim and artonge September 21, 2023 08:03

artonge approved these changes Sep 21, 2023

View reviewed changes

Altahrim approved these changes Sep 22, 2023

View reviewed changes

come-nc merged commit 9ebcd28 into nextcloud:master Oct 2, 2023
33 of 37 checks passed

backportbot-nextcloud bot added the backport-request label Oct 2, 2023

AaronDewes pushed a commit to AaronDewes/nc-server that referenced this pull request Oct 2, 2023

Merge pull request nextcloud#37201 from AaronDewes/fix/ldap-filter-ge…

e2d3fef

…neration Fix: Escape group names for LDAP

AaronDewes mentioned this pull request Oct 2, 2023

[stable27] Fix: Escape group names for LDAP #40739

Merged

AaronDewes pushed a commit to AaronDewes/nc-server that referenced this pull request Oct 2, 2023

Merge pull request nextcloud#37201 from AaronDewes/fix/ldap-filter-ge…

e04cb81

…neration Fix: Escape group names for LDAP

AaronDewes mentioned this pull request Oct 2, 2023

[stable26] Fix: Escape group names for LDAP #40740

Merged

AaronDewes deleted the fix/ldap-filter-generation branch October 2, 2023 12:43

This was referenced Oct 9, 2023

27.1.3 RC1 #40840

Merged

26.0.8 RC1 #40841

Merged

skjnldsv mentioned this pull request Oct 27, 2023

fix(changelog): bot detection nextcloud/github_helper#90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Escape group names for LDAP #37201

Fix: Escape group names for LDAP #37201

AaronDewes commented Mar 13, 2023 •

edited by solracsf

Loading

come-nc left a comment

AaronDewes commented Mar 14, 2023 •

edited

Loading

AaronDewes commented Mar 14, 2023

come-nc commented Mar 14, 2023

blizzz commented Mar 15, 2023 •

edited

Loading

come-nc commented Mar 15, 2023

AaronDewes commented Mar 20, 2023

AaronDewes commented Apr 28, 2023

blizzz commented Apr 28, 2023

AaronDewes commented Apr 28, 2023

blizzz commented Apr 28, 2023

AaronDewes commented Sep 20, 2023

Altahrim left a comment

AaronDewes commented Sep 29, 2023

welcome bot commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

backportbot-nextcloud bot commented Oct 2, 2023

backportbot-nextcloud bot commented Oct 2, 2023

Fix: Escape group names for LDAP #37201

Fix: Escape group names for LDAP #37201

Conversation

AaronDewes commented Mar 13, 2023 • edited by solracsf Loading

Summary

TODO

Checklist

come-nc left a comment

Choose a reason for hiding this comment

AaronDewes commented Mar 14, 2023 • edited Loading

AaronDewes commented Mar 14, 2023

come-nc commented Mar 14, 2023

blizzz commented Mar 15, 2023 • edited Loading

come-nc commented Mar 15, 2023

AaronDewes commented Mar 20, 2023

AaronDewes commented Apr 28, 2023

blizzz commented Apr 28, 2023

AaronDewes commented Apr 28, 2023

blizzz commented Apr 28, 2023

AaronDewes commented Sep 20, 2023

Altahrim left a comment

Choose a reason for hiding this comment

AaronDewes commented Sep 29, 2023

welcome bot commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

come-nc commented Oct 2, 2023

backportbot-nextcloud bot commented Oct 2, 2023

backportbot-nextcloud bot commented Oct 2, 2023

AaronDewes commented Mar 13, 2023 •

edited by solracsf

Loading

AaronDewes commented Mar 14, 2023 •

edited

Loading

blizzz commented Mar 15, 2023 •

edited

Loading