Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#19799 closed New feature (wontfix)

PASSWORD_HASHERS attempt to look up empty algorithm in certain cases

Reported by: Walter Doekes Owned by: nobody
Component: contrib.auth Version: 1.4
Severity: Normal Keywords:
Cc: Walter Doekes 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 Walter Doekes 11 years ago.
fix2.patch (604 bytes ) - added by Walter Doekes 11 years ago.
noalgorithmhasher.py (1.0 KB ) - added by Walter Doekes 11 years ago.
Example to go along with fix2.
19799-identify_hasher.diff (2.0 KB ) - added by Claude Paroz 11 years ago.
Proof of concept about identify_hasher asking hashers

Download all attachments as: .zip

Change History (8)

by Walter Doekes, 11 years ago

Attachment: fix1.patch added

by Walter Doekes, 11 years ago

Attachment: fix2.patch added

by Walter Doekes, 11 years ago

Attachment: noalgorithmhasher.py added

Example to go along with fix2.

comment:1 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedDesign decision needed
Type: UncategorizedNew 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

by Claude Paroz, 11 years ago

Attachment: 19799-identify_hasher.diff added

Proof of concept about identify_hasher asking hashers

comment:2 by Walter Doekes, 11 years ago

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 by Jacob, 11 years ago

Resolution: wontfix
Status: newclosed

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 by Claude Paroz, 11 years ago

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.

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