Opened 13 years ago
Closed 12 years ago
#18182 closed Bug (fixed)
Raw password echoed on authentication if no hashing used
Reported by: | Daniel Roseman | Owned by: | |
---|---|---|---|
Component: | contrib.auth | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | liokmkoil@…, moritz.sichert@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
If a password is somehow created without being hashed - say by the developer setting user.password
directly rather than via set_password
- the check_password
function wrongly assumes that the entire password is the hashing algorithm, and passes it to get_hasher
, resulting in an error message which reveals the actual password:
>>> user = User.objects.create(username='foo', password='bar') >>> authenticate(username='foo', password='bar') ... ValueError: Unknown password hashing algorithm 'bar'. Did you specify it in the PASSWORD_HASHERS setting?
The bug is in django.contrib.auth.hashers.check_password
, line 41, where it assumes that the result of encoded.split('$', 1)[0]
will always be an algorithm, when in the above case it's the password itself.
Although the password shouldn't have been created in this way in the first place, the code in check_password
should be more intelligent about whether or not it's found an algorithm name.
Attachments (3)
Change History (17)
comment:1 by , 13 years ago
Cc: | added |
---|
comment:2 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 13 years ago
Cc: | added |
---|---|
Has patch: | set |
Added patch (Github Pull Request)
contrib.auth.hashers.is_password_usable now checks if the encoded string contains a $ character. For backward compatibilty to unsalted md5 hashes all encoded 32-character-long strings without a $ character are assumed to be usable. check_password won't return True for a 32-character-long plain password, though.
by , 13 years ago
Attachment: | patch_18182.diff added |
---|
comment:5 by , 13 years ago
Cc: | removed |
---|
comment:6 by , 13 years ago
Cc: | added |
---|
comment:7 by , 13 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
hashers.py does now contain a identify_hasher function. In this patch, I suggest to use this function in is_password_usable.
comment:8 by , 13 years ago
Patch needs improvement: | set |
---|
The current problem with altering is_usable_password is that the error message "Invalid password format or unknown hashing algorithm" is not displayed in the user change form. This should be addressed.
comment:9 by , 13 years ago
Patch needs improvement: | unset |
---|
Now ReadOnlyPasswordHashWidget is still displaying an error message if encoded is not valid.
comment:10 by , 13 years ago
Note to self: "Unusable password, the user cannot login." should probably simply be "No password set."
follow-up: 12 comment:11 by , 12 years ago
What is the recommended way to fix an auth_user table full of "non passwords" as created following the provided code sample for an external authentication handler from https://docs.djangoproject.com/en/dev/topics/auth/#writing-an-authentication-backend ?
I'm planning to migrate an application to 1.4.1 and all my users have a 'nice' "password read from legacy_passwd table"...
What is the correct procedure to set all users without a valid password to an invalid password? Moreover: it'd be nice to update the sample for about external authentication... :-)
comment:12 by , 12 years ago
Replying to robertomaurizzi:
What is the recommended way to fix an auth_user table full of "non passwords" as created following the provided code sample for an external authentication handler from https://docs.djangoproject.com/en/dev/topics/auth/#writing-an-authentication-backend ?
I'm planning to migrate an application to 1.4.1 and all my users have a 'nice' "password read from legacy_passwd table"...
What is the correct procedure to set all users without a valid password to an invalid password? Moreover: it'd be nice to update the sample for about external authentication... :-)
Is this question related to this ticket? The ticket seems to be about Django possibly revealing a plain-text password if an auth backend has incorrectly stored a plain-text password in the password field. In your case it sounds like you have an innocuous string stored in your password fields. Also I'm not sure why upgrading to 1.4.1 is going to cause these to be a problem, unless you are also simultaneously getting rid of your custom authentation backend?
It's easy enough to change all the passwords to something more officially unusable:
Python 2.7.2+ (default, Oct 4 2011, 20:03:08) [GCC 4.6.1] on linux2 Type "help", "copyright", "credits" or "license" for more information. (InteractiveConsole) >>> from django.contrib.auth.models import User >>> from django.contrib.auth.hashers import UNUSABLE_PASSWORD >>> User.objects.filter(password='password read from legacy_passwd table').update(password=UNUSABLE_PASSWORD)
I suppose the referenced doc could be updated to use the "official" unsuable password, but I'm missing the reason why what's there now causes a problem. And I'm not sure adjusting that doc has anything to do with this ticket.
comment:13 by , 12 years ago
You're right, I should have described the connection :-)
My problem is indirectly related to this ticket, since #18453 'Unknown password hashing algorithm error if password is blank' actually describe my problem (error if no $ in the password field) and it's stated that 'This should be fixed if current patch of #18182 is committed, however let this one open until then.'.
This happens because 1.4 introduced support for multiple hashes and it seems that something is checking for the hash signature even if I'm using a custom authentication backend.
Setting the passwords to UNUSABLE_PASSWORD (a '!') works, so problem solved (and... documented).
I agree the docs should be updated with the correct unusable password. Is there an official way to ask for this?
comment:14 by , 12 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Since this may lead to leaking sensitive information, I suppose Django should test if the string contains at least a $ character, and complain that it doesn't look like a password hash otherwise.