Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible mishandling of backslashes in CNs #110

Open
s100 opened this issue Jun 28, 2024 · 1 comment
Open

Possible mishandling of backslashes in CNs #110

s100 opened this issue Jun 28, 2024 · 1 comment

Comments

@s100
Copy link

s100 commented Jun 28, 2024

We want to support CNs with unusual characters. For testing purposes, we have a user with a CN which is the following string:

const cn = '<User>,,<With>,,<Special>++<Chars>\\'

Notice that the final character in this string is a single backslash.

I have been having trouble getting our unit test to work after upgrading from ldapauth-fork@5 to ldapauth-fork@6. After a long investigation I have discovered that under ldapauth-fork@5 this CN was being converted to the following DN:

const dn = 'cn=\\3CUser\\3E\\2C\\2C\\3CWith\\3E\\2C\\2C\\3CSpecial\\3E\\2B\\2B\\3CChars\\3E\\5C,ou=users,o=mqst'

whereas under ldapauth-fork@6 the CN is converted to:

const dn = 'cn=\\3cUser\\3e\\2c\\2c\\3cWith\\3e\\2c\\2c\\3cSpecial\\3e\\2b\\2b\\3cChars\\3e\\,ou=users,o=mqst'

See how under ldapauth-fork@5 the backslash has been escaped as '\\5C' - backslash, "5", "C", as one would expect. However, with ldapauth-fork@6 the backslash has been passed through unmodified and unescaped, as '\\'. This causes authentication to fail.

Ultimately I believe the issue is here, in the source code of @ldapjs/dn. Notice that the backslash is omitted from the embeddedReservedChars array, and a comment states "We will handle the reverse solidus ('\') on its own." I'm not entirely sure how this is meant to work, but I see no code in evidence which actually handles backslashes, whereas adding an entry 0x5c to the array causes a properly escaped DN to appear, and fixes the issue.

However, @ldapjs/dn is no longer maintained. Nor is ldapjs, which is what uses @ldapjs/dn internally. So unfortunately the bug comes to ldapauth-fork, which uses ldapjs internally. Is there something that we can do in ldapauth-fork to repair this issue? Do we need to fork ldapjs and the whole @ldapjs/* ecosystem to get this fixed? (And is this even a correct diagnosis of the problem, or would my suggested fix cause different problems?)

I would be interested to hear your thoughts on this. Thank you for an excellent project!

@BalzGuenat
Copy link

I just found a probably related issue with a DN containing umlauts. The string bär within a DN is passed into the groupSearchFilter function as b\c3\a4r, i.e. with escaped umlauts. This string is then sanitized and escaped again, resulting in the group search filter containing the string b\5cc3\5ca4r when it should be b\c3\a4r.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants