Opened 12 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: Claude Paroz <claude@…>
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)

patch_18182.diff (1.5 KB ) - added by Moritz Sichert 12 years ago.
18182-2.diff (1.2 KB ) - added by Claude Paroz 12 years ago.
Updated and using identify_hasher
18182-3.diff (4.6 KB ) - added by Claude Paroz 12 years ago.
Proper display in ReadOnlyPasswordHashWidget

Download all attachments as: .zip

Change History (17)

comment:1 by Li Meng <liokmkoil@…>, 12 years ago

Cc: liokmkoil@… added

comment:2 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

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.

comment:3 by Moritz Sichert, 12 years ago

Owner: changed from nobody to Moritz Sichert
Status: newassigned

comment:4 by Moritz Sichert, 12 years ago

Cc: Moritz Sichert 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.

Last edited 12 years ago by Moritz Sichert (previous) (diff)

by Moritz Sichert, 12 years ago

Attachment: patch_18182.diff added

comment:5 by Moritz Sichert, 12 years ago

Cc: Moritz Sichert removed

comment:6 by Moritz Sichert, 12 years ago

Cc: moritz.sichert@… added

by Claude Paroz, 12 years ago

Attachment: 18182-2.diff added

Updated and using identify_hasher

comment:7 by Claude Paroz, 12 years ago

Owner: Moritz Sichert removed
Status: assignednew

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 Claude Paroz, 12 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.

by Claude Paroz, 12 years ago

Attachment: 18182-3.diff added

Proper display in ReadOnlyPasswordHashWidget

comment:9 by Claude Paroz, 12 years ago

Patch needs improvement: unset

Now ReadOnlyPasswordHashWidget is still displaying an error message if encoded is not valid.

comment:10 by Claude Paroz, 12 years ago

Note to self: "Unusable password, the user cannot login." should probably simply be "No password set."

comment:11 by robertomaurizzi, 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... :-)

in reply to:  11 comment:12 by Karen Tracey, 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 robertomaurizzi, 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 Claude Paroz <claude@…>, 12 years ago

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In 703c266682be39f7153498ad0d8031231f12ee79:

Fixed #18182 -- Made is_usable_password check if hashing algorithm is correct

The display of the ReadOnlyPasswordHashWidget has also been improved to
distinguish empty/unusable password from erroneous password.
Fixed #18453 also.
Thanks danielr and Leo for the reports and Moritz Sichert for the
initial patch.

Note: See TracTickets for help on using tickets.
Back to Top