Cannot login using LDAP after upgrading to 2.39.0

[dhedberg]

Preflight Checklist

• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for an issue that matches the one I want to file, without success.
• I am not looking for support or already pursued the available support channels without success.

Version

2.39.0

Storage Type

Kubernetes

Installation Type

Official Helm chart

Expected Behavior

It should accept my preexisting password and allow me to login as with 2.38.0

Actual Behavior

Failed login attempt for user: "<my-username>". Invalid credentials.

Steps To Reproduce

No response

Additional Information

My existing, randomly generated password happened to contain (at least) one of the characters now disallowed (or escaped) since the changes in #3372.

The first issue is that upgrading to 2.39.0 would force us to update our password policy on the LDAP server side to keep it in sync with these new requirements, and/or handle extra support from users when they for fairly non-transparent reasons fail to login using dex with passwords that work just fine elsewhere.

The second issue is that the change itself seems somewhat strange. Is it really, actually, necessary? Are the username and the password sent to conn.Bind() really used by the LDAP library in a way that can possibly be exploited through injection? Does the pull request solve an actual issue that someone saw somewhere?

In short: if it's not necessary, this change both reduces security by limiting the characters available for use in a password while it at the same time increases configuration and maintenance burden by introducing additional password requirements beyond those already configured on the LDAP server side.

Using randomly generated passwords with various symbols seems like a very common use-case to me, and I cannot imagine that I'll be the only one who will stumble upon this change :)

Configuration

No response

Logs

No response

[nabokihms]

@dhedberg, could you please share an example of the password with the special characters that doesn't work and are fine with your password policy? (this is not required to be an actual password)

[nabokihms]

It seems like escaping the username is not necessary, because we already doing it.

dex/connector/ldap/ldap.go

Line 411 in f611470

filter := fmt.Sprintf("(%s=%s)", c.UserSearch.Username, ldap.EscapeFilter(username))

A password cannot be used for injections because it is not part of the query. I guess we can safely revert the change, but let's ask the original author of PR.

@hsinhoyeh am I getting it right that we can safely revert it?

[hsinhoyeh]

@dhedberg Thank you for reaching out, and my apologies for adding to your workload with this security issue. Our system's code vulnerability scanning with ZAP flagged an LDAP injection issue, which we identified through [1]. Based on the documentation, it's crucial to ensure that data is stored in a sanitized form in the database and that user input is normalized before validation or comparison. I believe this advice applies to your situation as well.

@nabokihms You are right, username already did the escaping, but the password (along with escaped username) are directly sent to the LDAP server for querying https://github.com/go-ldap/ldap/blob/master/v3/bind.go#L98, which is why this issue was flagged.

But I am not an expert from security fields and would like to hear from your input.

[1] https://cheatsheetseries.owasp.org/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.html

[dhedberg]

Thanks @hsinhoyeh.

Security is obviously important, but your response suggests to me that you have not actually identified an issue. Using automated tooling as an aid in finding vulnerabilities and having some general understanding of various classes of potential security issues is great.

Applying fixes suggested by automated tools without understanding the context and why the fix may (or may not as I suspect) be needed, however, runs the risk of being useless or counterproductive.

My suspicion that this fix is wrong and counterproductive remains.

[dhedberg]

As you noted @nabokihms, we already do escape the username in the userEntry:

func (c *ldapConnector) userEntry(conn *ldap.Conn, username string) (user ldap.Entry, found bool, err error) {
	filter := fmt.Sprintf("(%s=%s)", c.UserSearch.Username, ldap.EscapeFilter(username))

This seems required as we're building a filter string. But with the recent changes we're now escaping the username twice which cannot lead to anything other than double-escaping and breaking usernames containing characters that require escaping. That cannot be right.

As for the password, we're (again, as I think you've noted) not using it to build any free-form query strings. I'm not very familiar with lower-level LDAP, but as far as I can tell the go-ldap Bind-function is building up an ASN.1/TLV-encoded query with the password. Surely this is analogous to a parameterized SQL-query where no escaping is needed.

Are there any remaining question marks ? Do you want me to send a revert-PR, or anything else?

[nabokihms]

@dhedberg I started preparing 2.39.1, ETA this week. I believe that nothing is required from your side, thanks for highlighting the problem 🙏

[dhedberg]

Great, thank you!

[nabokihms]

Dex v2.39.1 is out https://github.com/dexidp/dex/releases/tag/v2.39.1
The issue should be fixed now.

[dhedberg]

Thanks! I can confirm that it's working again for us with 2.39.1, so if we consider only the password issue relevant here then this bug can be closed.

I do note however that the double escape of the username is still present. This doesn't affect us but probably means that #3436 is still an issue.

[nabokihms]

Thanks for hinting