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

Update: LDAP_Injection_Prevention_Cheat_Sheet #1372

Open
einhirn opened this issue Apr 8, 2024 · 3 comments
Open

Update: LDAP_Injection_Prevention_Cheat_Sheet #1372

einhirn opened this issue Apr 8, 2024 · 3 comments
Labels
ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet.

Comments

@einhirn
Copy link

einhirn commented Apr 8, 2024

What is missing or needs to be updated?

After a casual reading, the Cheat Sheet seems to suggests that even passwords should always be escaped. When this is taken at face value, it can cause all kinds of issues, mainly stuff not working as expected any more (see dexidp/dex#3433).

The Safe Java Escaping Example wrongly assumes that a password check against LDAP is done via LDAP filter and escapes the password, when actually password checking against an ldap server is (or at least should be) exclusively done via BIND request which safely accepts the password in unfiltered plaintext.

How should this be resolved?

The usual methods for password authentication against LDAP should be mentioned:

  • Single-Step Auth:
    • BIND-Request with DN constructed from template like uid=${USERID},ou=people,dc=example,dc=com. Whatever is inserted in that template should be escaped as needed. Password from user input is given to BIND call as is.
  • Three-Step-Auth:
    • BIND with credentials limited to searching - these would be configured in the app.
    • SEARCH LDAP for users DN like (&(uid=${USERID})(deactivated=0)) or something. Again, anything inserted into that query should be escaped as needed
    • BIND with DN from search result above, if any. Password from user input is given to BIND call as is.

These two both assume the LDAP server to require (non-anonymous) BIND before allowing access to user data, which I think is (or should be) default or at least best practice. So Enabling Bind Authentication should be moved up in the document's focus, i.e. "To prevent LDAP Injection, first the LDAP server needs to be configured properly...". Even if it's out of scope of this document, proper safety precautions should be mentioned - "Simple BIND transmits plain text passwords to the server, so it should only be used with an encrypted connection" or "If you really need to use unencrypted connection, make sure to only use SASL BIND with challenge protocols etc. to prevent password sniffing."

The Safe Java Escaping Example should be updated to reflect either one of the above methods and no longer use the password in a search filter.

@einhirn einhirn added ACK_WAITING Issue waiting acknowledgement from core team before to start the work to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet. labels Apr 8, 2024
@jmanico
Copy link
Member

jmanico commented Apr 8, 2024

This cheatsheet can use a lot of changes. I like your ideas so far. Would you care to submit PR's?

@einhirn
Copy link
Author

einhirn commented Apr 11, 2024

I'll look into it and see when I might find some time to do that... I tend to overthink what I write on a word for word level, so it takes a lot of time to write something, especially something public in the field of security 😬

@mackowski mackowski added ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. and removed ACK_WAITING Issue waiting acknowledgement from core team before to start the work to fix it. labels Apr 22, 2024
@Jeymz
Copy link
Contributor

Jeymz commented Aug 7, 2024

What is missing or needs to be updated?

After a casual reading, the Cheat Sheet seems to suggests that even passwords should always be escaped. When this is taken at face value, it can cause all kinds of issues, mainly stuff not working as expected any more (see dexidp/dex#3433).

The Safe Java Escaping Example wrongly assumes that a password check against LDAP is done via LDAP filter and escapes the password, when actually password checking against an ldap server is (or at least should be) exclusively done via BIND request which safely accepts the password in unfiltered plaintext.

How should this be resolved?

The usual methods for password authentication against LDAP should be mentioned:

  • Single-Step Auth:

    • BIND-Request with DN constructed from template like uid=${USERID},ou=people,dc=example,dc=com. Whatever is inserted in that template should be escaped as needed. Password from user input is given to BIND call as is.
  • Three-Step-Auth:

    • BIND with credentials limited to searching - these would be configured in the app.
    • SEARCH LDAP for users DN like (&(uid=${USERID})(deactivated=0)) or something. Again, anything inserted into that query should be escaped as needed
    • BIND with DN from search result above, if any. Password from user input is given to BIND call as is.

These two both assume the LDAP server to require (non-anonymous) BIND before allowing access to user data, which I think is (or should be) default or at least best practice. So Enabling Bind Authentication should be moved up in the document's focus, i.e. "To prevent LDAP Injection, first the LDAP server needs to be configured properly...". Even if it's out of scope of this document, proper safety precautions should be mentioned - "Simple BIND transmits plain text passwords to the server, so it should only be used with an encrypted connection" or "If you really need to use unencrypted connection, make sure to only use SASL BIND with challenge protocols etc. to prevent password sniffing."

The Safe Java Escaping Example should be updated to reflect either one of the above methods and no longer use the password in a search filter.

Are you specifically talking about this area? I agree after reading this cheatsheet I think there is a lot of misinformation of proper ldap injection prevention. I am slightly concerned though on the search filter which is using an ldap query to identify users based on the password, I feel like a bind request should be the method used here to identify the user password match and then a search filter on the user identity to identify status of that account.

I wouldn't mind taking a stab at updating this, but I will admit I don't do much with ldap queries anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. HELP_WANTED Issue for which help is wanted to do the job. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet.
Projects
None yet
Development

No branches or pull requests

4 participants