Code

Opened 17 months ago

Closed 16 months ago

Last modified 16 months ago

#19799 closed New feature (wontfix)

PASSWORD_HASHERS attempt to look up empty algorithm in certain cases

Reported by: wdoekes Owned by: nobody
Component: contrib.auth Version: 1.4
Severity: Normal Keywords:
Cc: wdoekes Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

the Django 1.4+ PASSWORD_HASHERS lookup conflicts with $1$<salt>$<password> (md5crypt) style hashes.

I'm using Modoboa which uses it's own auth first and falls back to Django auth if passwords don't match. For the aforementioned hash the algorithm detected becomes '' (the empty string).

1.4:

    if len(encoded) == 32 and '$' not in encoded:
        hasher = get_hasher('unsalted_md5')
    else:
        algorithm = encoded.split('$', 1)[0]
        hasher = get_hasher(algorithm)

master:

    if ((len(encoded) == 32 and '$' not in encoded) or
            (len(encoded) == 37 and encoded.startswith('md5$$'))):
        algorithm = 'unsalted_md5'
    else:
        algorithm = encoded.split('$', 1)[0]

That yields:

ValueError: Unknown password hashing algorithm ''. Did you specify it in the PASSWORD_HASHERS setting?

I thought I could fix that by adding a custom hasher which always returns False, but that fails because I'm not allowed to use the empty name for algorithm.

ImproperlyConfigured: hasher doesn't specify an algorithm name: modoboa.auth.hashers.NoAlgorithmHasher

To fix this, I either need it to refuse the empty algorithm (fix1.patch), or allow the empty algorithm (fix2.patch, where I'll have my bogus hasher return false later on).

In both cases the fixes are trivial.


As an aside, the current if not getattr(hasher, 'algorithm') looks like a typo. I'd go for either if not hasher.algorithm or if not getattr(hasher, 'algorithm', None).

        if not getattr(hasher, 'algorithm'):
            raise ImproperlyConfigured("hasher doesn't specify an "
                                       "algorithm name: %s" % backend)


Kind regards,
Walter Doekes
OSSO B.V.

Attachments (4)

fix1.patch (421 bytes) - added by wdoekes 17 months ago.
fix2.patch (604 bytes) - added by wdoekes 17 months ago.
noalgorithmhasher.py (1.0 KB) - added by wdoekes 17 months ago.
Example to go along with fix2.
19799-identify_hasher.diff (2.0 KB) - added by claudep 17 months ago.
Proof of concept about identify_hasher asking hashers

Download all attachments as: .zip

Change History (8)

Changed 17 months ago by wdoekes

Changed 17 months ago by wdoekes

Changed 17 months ago by wdoekes

Example to go along with fix2.

comment:1 Changed 17 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type changed from Uncategorized to New feature

As explained here [1], Django currently only supports hashes in the form <algorithm>$<iterations>$<salt>$<hash>. There is also a note about bcrypt [2] that explicitely tells about incompatibility of such hashes (starting with $), and even suggests updating hashes in the database.

In 1.5, you could create a custom user model and customize check_password to pass a modified hash to the hashers.check_password function.

What Django could do in the future (need to be confirmed by a security-savvy developer) is to pass hashes to the hashers who could determine themselves if they support the hash format (in identify_hashers). Marking as DDN for this idea.

[1] https://docs.djangoproject.com/en/dev/topics/auth/passwords/
[2] https://docs.djangoproject.com/en/dev/topics/auth/passwords/#using-bcrypt-with-django

Changed 17 months ago by claudep

Proof of concept about identify_hasher asking hashers

comment:2 Changed 17 months ago by wdoekes

Thank you for the links. In my case I cannot switch passwords at will since the mail auth daemon uses them too.

I like the idea of your identify_hasher fix. That would be an even better solution indeed.

Regards,
Walter

comment:3 Changed 16 months ago by jacob

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

Now that we have custom user models I don't really see the need to support custom schemes in this way; marking wontfix.

comment:4 Changed 16 months ago by claudep

As for me, I think that custom user model is orthogonal to custom hashers functionality. The idea here is to give more flexibility to the password hashers system, and not hard code the hash starting with $algorithm$....
I even think that asking people having to cope with custom password hashes to rewrite the password checking mechanism instead of simply creating their own hasher has security implications, as they might easily introduce flaws in this sensible part of their code.

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.