-
Notifications
You must be signed in to change notification settings - Fork 25
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
userPassword is sent to the browser #23
Comments
Hello @AlexandreTunstall, Thanks for the thorough analysis. I agree that the The password verification uses a server-side There might be two issues when changing passwords:
Will come back to it, but need to do some refactoring first, so this might take a moment. |
Hello @AlexandreTunstall, Updates the password logic: The backend never sends passwords to the client, but Exception: For new entries, clear text passwords are allowed, it is up to the directory to hash them. When this happens, the client gets the hash. Hope this helps, please close this issue after testing. |
Please review, and let me know whether the issue can be closed. |
Sorry, I haven't had much time to test in the past month, but I've had another try with the latest commit. The
This still doesn't seem to work on the latest version. When I try to add a I think the easiest way of reproducing this is to try setting a password for the user you're signed in as and then refresh the page. Basic authentication should fail after refreshing. |
Although the UI hides a user's existing password, it is still sent to the browser and can be seen in the network logs. Arguably, this is more an issue with the LDAP access configuration (more on this later), but fixing it is still a good idea for better defense in depth.
The only fathomable reason for this is to verify the "old password" field on the change password prompt, yet that verification process works with only
=wx
access (i.e. unreadableuserPassword
), so I think this could easily be fixed without compromising on functionality.Using
=wx
access to try and work around this problem allows the user to change their own password (theuserPassword
attribute has to be manually added since it's not visible and the "old password" field has to be manually enabled), but then ldap-ui reports an error because it tries to read the attribute, making the user think the operation failed.In summary, here's what I think should be changed for better security:
userPassword
when it's not readable (disabled yet required "old password" field and misleading error after making the change). I can't think of any way of soundly determining whether an object has a password without adding other permissions, which may be less secure [1].userPassword
attribute is readable, don't send its value to the browser (even better if it's possible to avoid reading it from LDAP in the first place).[1] With OpenLDAP 2.4,
=wsx
allows use of a(userPassword=<value>)
filter to find users with specific passwords. This might be fine with salted hashes (like the default SSHA), but certainly isn't with unsalted or unhashed passwords. If this risk is acceptable, it could be used to determine whether an object has a password with the(userPassword=*)
filter.The text was updated successfully, but these errors were encountered: