LDAP User Provider wrong handling of passwords #18401

MortI2C · 2016-04-01T07:28:17Z

Hi, trying to use the new LDAP component I think I have found a bug on the Component\Security\Core\User\LDAPUserProvider.

Concretely, LDAP on successful authentication retrieves an array with each user parameter, and each of this have at the same time another array. Due to this on this new version, among other improvements, the Entry class was introduced to handle this situation, but on the method loadUser, when it creates a new user it gives as a password the Entry for the password field, without taking into consideration that at the same time is an array of passwords. It is needed to get the first one - which should be the only one as we only authenticate one user at a time -.
This problem makes LDAP fail to completely authenticate the user. Also it doesn't allow symfony to store the remember me cookies correctly. This is because the token generator requires requires a single element not an array on the password parameter.

And there is a second bug on the same class, on the method refreshUser, when it creates the User instance giving a null as a password, instead of calling $user->getPassword() as it looks like it should do.

I have done a quick and not necessarily good fix on my own fork to check this issue, and it worked fine just with this small changes.

To reproduce, install a LDAP server, configure it using this doc as a reference: http://symfony.com/blog/new-in-symfony-2-8-ldap-component
Except the part of the service definition, that as mentioned on symfony/symfony-docs#5756 (comment) on the new version has to be defined like this:

app.ldap:
        class: Symfony\Component\Ldap\Ldap
        factory: ['Symfony\Component\Ldap\Ldap', 'create']
        arguments:
            - 'ext_ldap'
            - host: 'host.mydomain.com'

And after an initial successful authentication you should be getting a redirection loop to the authentication layer, as LDAP constantly fails to authenticate the user given that it tries to authenticate with a null password.

If someone else has the same problem and this bug can be confirmed, I can make a pull request with a fix.
Also, on this side, I'm not sure what the rules on fixes are on symfony, but it would be nice to add a fix for the 2.8 branch as well, which had the same issue and the bug is solved pretty much in the same way.

csarrazi · 2016-04-13T19:40:26Z

Regardless of the version you are using, you can still use the LdapClient class, whose initialization is the same as described in the blog post.

Regarding the password field, this still needs some tweaking:

The userPassword field needs to be made dynamic, by adding a new option in the user provider's configuration
The password attribute needs to be converted from an array to a string. Ldap attributes are multi-valued, which explains why you have an array as the password. This can be fixed easily in the user provider, by fetching the only element from the array ($password[0], for example).

ofbeaton · 2016-05-02T20:12:14Z

Is the LdapClient/Ldap in a usable state? I am also having a ldap_bind(): Unable to bind to server: Invalid credentials error in my dev.log on 3.1.*-dev ever since trying to convert to the LDAP component from the old 2.7 only IMAG/ldap-bundle.

I'm a bit of a noob so I'm not sure if this bug means the component will currently never work, or if I made a mistake when converting my configuration over from IMAG.

MortI2C · 2016-05-02T20:40:32Z

Since in your case it's failing at binding with LDAP, it's not related to this. I haven't used the new versions of symfony as per our project we only use LTS versions, but looks like you might be missconfiguring something.

Besides, the component itself works fine. All the problems are in wrong handling of data on the default user provider, which is easily solved by just writing your own.

csarrazi · 2016-05-03T08:03:08Z

@ofbeaton Yes, the client is indeed in a usable state. If you want help with configuring the client / security providers, you can raise an issue. You can also take a look at the documentation ticket, which is not yet merged, but is ready (at least for 2.8/3.0). A new doc ticket will be created for the new 3.1 features.

bodabodah · 2016-05-22T21:23:37Z

I'm facing the same issue, the password field from ldap is an array in my case.

hash_equals(): Expected known_string to be a string, array given

csarrazi · 2016-05-25T11:41:09Z

@MortI2C I'll try to do a PR on 3.1 to fix this issue during the week.

MortI2C · 2016-05-25T12:16:26Z

Ok, but no hurry for me, on my project we need to use the 2.8 and anyway I have my own custom user provider - not only for this reason, in the end we needed to add lots of customization as well -.

Thanks

csarrazi · 2016-05-26T02:27:19Z

By the way, @MortI2C, regarding the second "bug". This is actually not a bug. During refreshUser, the password should actually not be fetched, as we don't re-authenticate users against the Ldap server, but instead rely on the initial authentication. The refreshUser method actually only refreshes the user from the session, without checking whether he has changed or not. This could change at a later time, but know that when re-authenticating (during the refresh user phase), the password can not be compared, as it is not available anymore.

As a reminder, storing the user's password inside the user's session is a security flaw. So this will most likely not change. 😃

…ining an array of values. (csarrazi) This PR was merged into the 3.1 branch. Discussion ---------- [Security][Ldap] Fixed issue with password attribute containing an array of values. | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18401 | License | MIT | Doc PR | This PR fixes #18401, as well as other possible issues: * First, the user provider no longer requires a password attribute by default. While this is not mandatory, it is more explicit to not set a password when using the `form_login_ldap` or `http_basic_ldap`, as these two providers don't use a password comparison mechanism, but `ldap_bind()` instead. * Second, the attribute is now configurable. Some implementations actually use different properties to store the user's password attribute. This will enable some users to correctly work with specific configurations. * Third, the user provider normalises the attribute array into a single string. Also, if the attribute has more than one value (which should not be possible), or if is not set, an exception will be thrown, with a clear error message. Commits ------- dbf45e4 [Ldap] Fixed issue with Entry password attribute containing array of values and made password attribute configurable

MortI2C changed the title ~~LDAP component bug~~ LDAP User Provider wrong handling of passwords Apr 1, 2016

javiereguiluz added Bug Unconfirmed Ldap labels Apr 1, 2016

carsonbot added the Status: Needs Review label Apr 1, 2016

csarrazi mentioned this issue May 26, 2016

[Security][Ldap] Fixed issue with password attribute containing an array of values. #18881

Merged

fabpot closed this as completed May 26, 2016

chalasr mentioned this issue Aug 20, 2019

[Ldap] Make LdapUserProvider::refreshUser() actually reload the user entry #33246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LDAP User Provider wrong handling of passwords #18401

LDAP User Provider wrong handling of passwords #18401

MortI2C commented Apr 1, 2016

csarrazi commented Apr 13, 2016

ofbeaton commented May 2, 2016

MortI2C commented May 2, 2016

csarrazi commented May 3, 2016 •

edited

Loading

bodabodah commented May 22, 2016

csarrazi commented May 25, 2016

MortI2C commented May 25, 2016

csarrazi commented May 26, 2016

LDAP User Provider wrong handling of passwords #18401

LDAP User Provider wrong handling of passwords #18401

Comments

MortI2C commented Apr 1, 2016

csarrazi commented Apr 13, 2016

ofbeaton commented May 2, 2016

MortI2C commented May 2, 2016

csarrazi commented May 3, 2016 • edited Loading

bodabodah commented May 22, 2016

csarrazi commented May 25, 2016

MortI2C commented May 25, 2016

csarrazi commented May 26, 2016

csarrazi commented May 3, 2016 •

edited

Loading