Code

Opened 2 years ago

Closed 20 months ago

#18182 closed Bug (fixed)

Raw password echoed on authentication if no hashing used

Reported by: danielr 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 moritzs 2 years ago.
18182-2.diff (1.2 KB) - added by claudep 23 months ago.
Updated and using identify_hasher
18182-3.diff (4.6 KB) - added by claudep 23 months ago.
Proper display in ReadOnlyPasswordHashWidget

Download all attachments as: .zip

Change History (17)

comment:1 Changed 2 years ago by Li Meng <liokmkoil@…>

  • Cc liokmkoil@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 2 years ago by moritzs

  • Owner changed from nobody to moritzs
  • Status changed from new to assigned

comment:4 Changed 2 years ago by moritzs

  • Cc moritzs 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 2 years ago by moritzs (previous) (diff)

Changed 2 years ago by moritzs

comment:5 Changed 2 years ago by moritzs

  • Cc moritzs removed

comment:6 Changed 2 years ago by moritzs

  • Cc moritz.sichert@… added

Changed 23 months ago by claudep

Updated and using identify_hasher

comment:7 Changed 23 months ago by claudep

  • Owner moritzs deleted
  • Status changed from assigned to 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 Changed 23 months ago by claudep

  • 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.

Changed 23 months ago by claudep

Proper display in ReadOnlyPasswordHashWidget

comment:9 Changed 23 months ago by claudep

  • Patch needs improvement unset

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

comment:10 Changed 23 months ago by claudep

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

comment:11 follow-up: Changed 20 months ago by 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... :-)

comment:12 in reply to: ↑ 11 Changed 20 months ago by kmtracey

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 Changed 20 months ago by robertomaurizzi

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 Changed 20 months ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.