fix(LDAP): escape DN on check-user

[blizzz]

Summary

the DN has to be escaped differently when used as a base and we were missing it here in the search method call in the check-user command.

Checklist

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

[blizzz]

/backport to stable28

[blizzz]

/backport to stable27

[max-nextcloud]

/backport! to stable28

[max-nextcloud]

backporting right away to have a patch the customer can test.

[come-nc]

So getDN does not return the raw dn but an escaped version?
Where is it escaped and how?

[blizzz]

So getDN does not return the raw dn but an escaped version? Where is it escaped and how?

It is stored in an escaped way in the database. But when you use a DN as base parameter, it has to be differently encoded. It's all about the backslash.

[blizzz]

cf. https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/Access.php#L252 and https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/Access.php#L1600

P.S.: putting it differently, when using it in the search filter, the backslash has to be escaped, what we default to, cf. https://www.rfc-editor.org/rfc/rfc2254#page-5

[come-nc]

@blizzz But I was sure we were using ldap_escape when using the dn (or anything really) in the search, with this system we may get double escaping.
It’s too late for changing this I suppose but it should be better documented.
https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L365 does not specify that it’s escaped in any way.

[blizzz]

@blizzz But I was sure we were using ldap_escape when using the dn (or anything really) in the search, with this system we may get double escaping. It’s too late for changing this I suppose but it should be better documented. https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L365 does not specify that it’s escaped in any way.

The DN is escaped in Helper::sanitizeDN() and this is used for queries/filters.

When the DN is used as base DN for operations though, the backlash must not be escaped as \5c.

That is the difference.

P.S.: sanitizeDN() follows RFC 2253

[come-nc]

I still think this should be better documented.

[blizzz]

@come-nc I added a commit with the doc. Not in the user model though, because it returns only what it was being fed with. Is this acceptable?

[blizzz]

/backport to stable29

[come-nc]

@come-nc I added a commit with the doc. Not in the user model though, because it returns only what it was being fed with. Is this acceptable?

Yeah it helps. It still bugs me that we do not escape upon building the filter instead. I think we avoid double-escaping only because we do not apply escape when searching for group members, and we got lucky that uids never need escaping.
To be safe

server/apps/user_ldap/lib/Group_LDAP.php

Line 808 in 1fb5486

$uid = $result[0];

should escape for instance, because it is later used raw in the filter, and the dn version is already escaped. Our internal code is not clear enough on what is escaped or not and that’s dangerous.

[blizzz]

@come-nc I added a commit with the doc. Not in the user model though, because it returns only what it was being fed with. Is this acceptable?

Yeah it helps. It still bugs me that we do not escape upon building the filter instead. I think we avoid double-escaping only because we do not apply escape when searching for group members, and we got lucky that uids never need escaping. To be safe

server/apps/user_ldap/lib/Group_LDAP.php

Line 808 in 1fb5486

$uid = $result[0];

should escape for instance, because it is later used raw in the filter, and the dn version is already escaped. Our internal code is not clear enough on what is escaped or not and that’s dangerous.

There is no double escape per se. It is only about the backslash that is expected in different forms when used in search filter compared to when used as base. As usage in filters is the common usage, that format was chosen to be saved in the DB.

[blizzz]

P.S.: When is the raw uid used in a search filter? (Apart from override as uuid attribute)

[come-nc]

P.S.: When is the raw uid used in a search filter? (Apart from override as uuid attribute)

server/apps/user_ldap/lib/Group_LDAP.php

Line 864 in 1fb5486

$filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn;

(following the line I posted earlier, $dn may be the uid there.)

[blizzz]

P.S.: When is the raw uid used in a search filter? (Apart from override as uuid attribute)

server/apps/user_ldap/lib/Group_LDAP.php

Line 864 in 1fb5486

$filter = $this->access->connection->ldapGroupMemberAssocAttr . '=' . $dn;

(following the line I posted earlier, $dn may be the uid there.)

Indeed. In those cases when a DN is not being used to reference members 😰 But a valid case nonetheless. This is not in scope for this PR though, and also it is nothing that can be crafted by a shady user.

[blizzz]

/backport! to stable28