Skip to content

Delete character escaping for passwords#253

Merged
whikernel merged 1 commit into
dfir-iris:developfrom
juadde:ldap_password
May 30, 2023
Merged

Delete character escaping for passwords#253
whikernel merged 1 commit into
dfir-iris:developfrom
juadde:ldap_password

Conversation

@juadde

@juadde juadde commented May 24, 2023

Copy link
Copy Markdown
Contributor

Hello,

Escaping character will cause passwords check failure in case of characters like * (I would tend to think that no escape_filter_chars() is needed at all in this file).

This is a pull request so passwords with "strange" characters can work.

Kind regards.

@juadde

juadde commented May 25, 2023

Copy link
Copy Markdown
Contributor Author

To test this issue, setup a LDAP with a user that has a password containing *, and try to connect. It will not work.

@whikernel

Copy link
Copy Markdown
Contributor

Hi @juadde
Before making this change we need to dig in the code of ldap3 because we're not able to ensure 100% that the user password can't be used for LDAP injection through the Connection function.

Let us know if you have any insight about this!

@juadde

juadde commented May 25, 2023

Copy link
Copy Markdown
Contributor Author

As far as I understand it, escaping (as done in escape_filter_chars) is performed to ensure that when a request to the LDAP is performed there will be no possibility to have malicious content in a filter (like (&(objectClass=person)(objectClass=user)) for example). The password should never be part of LDAP filters (the passwords are not stored in pain text anyway - so making a filter with the password value would be useless). Escaping it would only change the password value if some characters are present. It is also possible to unescape the password with unescape_filter_chars() when you use it as a password, but I don't really see the point here.

In connection.py it is always used for authentication with SASL, NTLM etc.

I am unsure that if we were trying to prevent injections in the authentication protocols we are using the right function (and we would need unescaping the password before giving it to the authentication protocols anyway, as they need the "right" password).

I don't know if that helps a little bit?

@whikernel

Copy link
Copy Markdown
Contributor

Hi @juadde

Yes indeed, that was an total oversight from us. It makes sense that the pwd are not passed in the LDAP filters so there's not point of filtering the pwd. Which as you said has to be sent as is anyway.
Thanks for taking the time and the links 👍

We'll merge and push in the next patch.

@whikernel
whikernel self-requested a review May 27, 2023 07:14
@whikernel whikernel self-assigned this May 27, 2023
@whikernel whikernel added the enhancement New feature or request label May 27, 2023
@whikernel whikernel added this to the v2.2.2 milestone May 27, 2023
@whikernel
whikernel merged commit dd6424a into dfir-iris:develop May 30, 2023
@juadde
juadde deleted the ldap_password branch May 30, 2023 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants