LDAP Server: Drupal to LDAP - Password sanitized [#2463755]

When you synchronise user from Drupal to LDAP with password, the password gets sanitized by check_plain() so hello&world becomes hello&world.

Patch follows.

Comment	File	Size	Author
#2	password_sanitized-2463755-2.patch	792 bytes	haggins
#2	7.x-2.x: PHP 5.3 & MySQL 5.5, D7 19 pass, 13 fail PHP 5.4 & MySQL 5.5, D7 21 pass PHP 5.5 & MySQL 5.5, D7 21 pass PHP 5.6 & MySQL 5.5, D7 21 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

haggins CreditAttribution: haggins commented 1 April 2015 at 13:26

Issue summary:

View changes

Comment #2

haggins CreditAttribution: haggins commented 1 April 2015 at 13:32

Status:

Active

» Needs review

File	Size
password_sanitized-2463755-2.patch	792 bytes
7.x-2.x: PHP 5.3 & MySQL 5.5, D7 19 pass, 13 fail PHP 5.4 & MySQL 5.5, D7 21 pass PHP 5.5 & MySQL 5.5, D7 21 pass PHP 5.6 & MySQL 5.5, D7 21 pass

Comment #3

haggins CreditAttribution: haggins commented 21 September 2016 at 08:56

Priority:

Normal

» Major

This issue still exists in Beta 11, 15 months after patch contribution. The patch still applies.

Could you please review and commit it?

Comment #4

nullkernel CreditAttribution: nullkernel commented 5 October 2016 at 22:15

Status:

Needs review

» Reviewed & tested by the community

The patch works for me. It took me a little bit of time to reproduce the issue, since Drupal's user table has the password changed too.

When using Drupal's password change form, a password like hello&world will be stored (correctly) in the users table. So you are able to log in to Drupal with that password since the account is not actually authenticated against LDAP. However the LDAP server has the password incorrectly set to hello&world.

I was able to reproduce the problem by:
1. Start with a user account has a password with no special characters (e.g. password)
2. Query the DB for the user's current password hash. (e.g. SELECT name, pass FROM users WHERE name = 'testaccount';).
3. Use Drupal to change the user's password to hello&world.
4. Overwrite pass in the users table with the hash returned by the earlier query (e.g. UPDATE users SET pass = '$S$DpNJh0AGbCGVOr.k/hklTshd6FRjMJg5FTJsd3kjbBHJM' WHERE name = 'testaccount';)
5. Attempt to log in to the user account by entering the password as hello&world. It will fail (because this patch is not applied).
6. Attempt to log in to the user account by entering the password as hello&world. You will be successfully authenticated by the LDAP server and logged in.

Applying the patch and going through these steps results in hello&world working. I also used my debugger to inspect variables and could see the hello&world value before this patch (and hello&world after this patch).

I think that the change made by the patch should be fine. The check_plain function is generally useful for preventing XSS vulnerabilities. In this case however, I can think of no reason to process a password attribute with check_plain. Once the password is saved by the LDAP server, it would never be printed within HTML (or even directly retrievable).

In my opinion, this patch looks good. Changing status to RTBC.

Edit: Used code tags for formatting so that html entities I wrote are displayed as intended.

Comment #5

haggins CreditAttribution: haggins commented 11 October 2016 at 09:34

Thank you @nullkernel for your testing efforts!

Another method to reproduce it, is to set the password in Drupal to "hello&world" and then trying to login into another (non drupal) application that uses the same LDAP. It won't work until one applies this patch.

Comment #6

15 February 2017 at 17:37

grahl committed c696441 on 7.x-2.x authored by haggins

Issue #2463755 by haggins, nullkernel: LDAP Server: Drupal to LDAP -...

Comment #7

grahl

he/him

Zürich

CreditAttribution: grahl at Namics commented 15 February 2017 at 17:37

Status:

Reviewed & tested by the community

» Fixed

Thanks for testing & reviewing, patch looks fine. I would have liked to sanitize this more but pushing it as-is for a password should be safe since we never see the original again.