Code

Opened 7 years ago

Closed 6 years ago

#4151 closed (duplicate)

Patch to add support for more secure password hashes in Python 2.5 or newer

Reported by: Nick Efford <nick@…> Owned by: nobody
Component: Contrib apps Version: master
Severity: Keywords: authentication, password, hash
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Django currently uses the sha module from the standard library to compute the SHA-1 hash of a password. Developers with particular concerns about security may prefer to use a stronger, more secure hashing algorithm such as SHA-256. Such algorithms are available in the standard library as of Python 2.5, via the hashlib module.

This patch modifies django.contrib.auth.models in two ways. First, it adds support for hashlib and the SHA-224, SHA-256 and SHA-384 algorithms to the check_password function. (For SHA-512 to be supported, the password field of the User model would need to be lengthened.) Second, it modifies the set_password method of the User model to use SHA-256 by default for password hashing, falling back on the sha module if hashlib cannot be imported.

doctests for the check_password function are included with the patch.

Attachments (4)

password.diff (1.7 KB) - added by Nick Efford <nick@…> 7 years ago.
diff for django.contrib.auth.models adding support for stronger hashes
tests.py (1.3 KB) - added by Nick Efford <nick@…> 7 years ago.
doctests for the patched check_password function
password2.diff (2.3 KB) - added by Nick Efford <nick@…> 7 years ago.
Updated diff to add support for stronger password hashes - replaces password.diff
docs.diff (2.3 KB) - added by Nick Efford <nick@…> 7 years ago.
Diff of authentication.txt, documenting the changes made by password2.diff.

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by Nick Efford <nick@…>

diff for django.contrib.auth.models adding support for stronger hashes

Changed 7 years ago by Nick Efford <nick@…>

doctests for the patched check_password function

comment:1 Changed 7 years ago by Nick Efford <nick@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The original version of the patch can be improved by adding a new setting to django.conf.global_settings, called PASSWORD_HASH_ALGORITHM, and making the set_password method of the User model use this setting.

I'm updating the patch to include this enhancement. PASSWORD_HASH_ALGORITHM is set to a default of 'sha256' in global_settings.py. Django users can redefine it in their settings.py to 'sha224' or 'sha384',
or even to 'sha1' if they wish to preserve the pre-patch behaviour of the admin app. Note, however, that this setting
will be ignored if the hashlib module is not available (i.e. in versions of Python before 2.5).

Changed 7 years ago by Nick Efford <nick@…>

Updated diff to add support for stronger password hashes - replaces password.diff

Changed 7 years ago by Nick Efford <nick@…>

Diff of authentication.txt, documenting the changes made by password2.diff.

comment:2 Changed 7 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

Extending the password length to fit SHA-512 now (prior to 1.0) is probably a good idea. I'd also prefer to keep the default in GLOBAL_SETTINGS as sha1 (since this is the current, documented, method), and just document that better hashes are available in python 2.5

But - we'll let core decide on these for now.

comment:3 follow-up: Changed 7 years ago by SmileyChris

Simon jumped in while I was writing this:

Thanks Nick, but you won't be getting this accepted if you are trying to make a change which requires anything above Python 2.3.

This doesn't mean we couldn't support these secure hashes, they just can't be the default in global_settings.py.

The other consideration (hence my moving to design decision required) is that this puts a partial requirement on >2.3 in the case of databases which containing new hash formats. This might be acceptable, I'm not sure.

(Side note: it's usual practice to just provide one complete patch with both code, tests and documentation changes)

comment:4 Changed 7 years ago by SmileyChris

  • Patch needs improvement set

comment:5 in reply to: ↑ 3 Changed 7 years ago by Nick Efford <nick@…>

Replying to SmileyChris:

The other consideration (hence my moving to design decision required) is that this puts a partial requirement on >2.3 in the case of databases which containing new hash formats. This might be acceptable, I'm not sure.

Good point.

How about we add another setting, USE_STRONG_HASHES, set to False in global_settings.py? When this setting is False, the current hashing code is used; when it is set to True explicitly in a project's settings.py, an attempt is made to use hashlib and one of the longer hash algorithms (as specified by PASSWORD_HASH_ALGORITHM).

This approach forces the user to turn on the potentially incompatible behaviour. We could add documentation to carefully explain the potential consequences of doing this. Would that suffice?

I'm happy to rework the patch to follow this slightly different approach, if encouraged to do so :)

comment:6 Changed 7 years ago by jacob

I like the idea -- there's not reason for Django not to use the strongest hashes available (mmmm... strong hash....). However, the setting is a bit pointless -- there's no reason to use weak hashes if strong ones are available.

So while I'm -1 on this patch, I'd be +0 on a patch that used SHA-256 if hashlib is available, and fell back to SHA1 if not.

comment:7 Changed 6 years ago by SmileyChris

Firstly, I disagree with jacob. There is a reason for not blindly using the strongest hash available - if run on a Python 2.5 system, it causes the Django auth component of the database to become dependent on Python 2.5.

Replying to the original description: Nick Efford <nick@efford.org>:

(For SHA-512 to be supported, the password field of the User model would need to be lengthened.)

Not true, just use a different encoding rather than hexidecimal:

>>> import hashlib
>>> from binascii import b2a_base64
>>> hsh = hashlib.new('sha512', 'test')
>>> len(hsh.hexdigest())
128
>>> len(b2a_base64(hsh.digest()).rstrip())   # rstrip because output always ends in \n
88

comment:8 Changed 6 years ago by jacob

  • Resolution set to duplicate
  • Status changed from new to closed

Chris is completely right here, as I came to realize over in #5600. Marking this one a dup.

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.