MD5PasswordHasher: broken backwards compatibility with empty salt

[apreobrazhensky@…]

In our project we stored unsalted MD5 passwords in format: "md5$$<actual md5 hash>".
In Django 1.3 django.contrib.auth.check_password(...) coped well with such format.

In Django 1.4 trying to check such password fails with assertion in django.contrib.auth.hashers.MD5PasswordHasher.encode(...)

Traceback (most recent call last):
  File "/home/admin/sites/env/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 111, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/home/admin/sites/fxstart-production/source/apps/accounts/decorators.py", line 17, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/home/admin/sites/fxstart-production/source/apps/dashboard_personal/views.py", line 57, in view_change_password
    if form.save():
  File "/home/admin/sites/fxstart-production/source/apps/dashboard_personal/forms/change_password.py", line 129, in save
    if not self.account.check_withdraw_password(current_password):
  File "/home/admin/sites/fxstart-production/source/apps/accounts/models.py", line 184, in check_withdraw_password
    return check_password(raw_password, self.withdraw_password)
  File "/home/admin/sites/env/local/lib/python2.7/site-packages/django/contrib/auth/hashers.py", line 45, in check_password
    is_correct = hasher.verify(password, encoded)
  File "/home/admin/sites/env/local/lib/python2.7/site-packages/django/contrib/auth/hashers.py", line 319, in verify
    encoded_2 = self.encode(password, salt)
  File "/home/admin/sites/env/local/lib/python2.7/site-packages/django/contrib/auth/hashers.py", line 312, in encode
    assert salt and '$' not in salt
AssertionError

As you can see from the code, salt variable is asserted, which means AssertionError is raised every time salt is empty string.

Is this intended behavior? (which means we have to regenerate our password's database or workaround it via replacing hashes like "md5$$<actual md5 hash>" to "<actual md5 hash>" before calling check_password)

[Aymeric Augustin]

See #17777.

[apreobrazhensky@…]

Pardon me, but #17777 is not nearly the same thing I'm talking about here.
And its description and comments doesn't answer my question (and yes, I read it before posting).
I think, you were a bit hasty with bug closing.

What I'm talking about is:

from django.contrib.auth.models import check_password

check_password("test", "md5$$098f6bcd4621d373cade4e832627b4f6")

raises AssertionError in Django 1.4, when in Django 1.3 it worked perfectly.

[Aymeric Augustin]

Sorry -- #17777 looked suspiciously similar, and I didn't know you had reviewed it first.

[Claude Paroz]

Handle md5$$... syntax

[anonymous]

This is also true for SHA1PasswordHasher

[Claude Paroz]

#19687 is a duplicate.

[Aymeric Augustin]

The patch looks good, but it no longer applies.

[Claude Paroz <claude@…>]

In 63d6a50dd8ab4f47c9ae11c1d2a4c4c7eed06be6:

Fixed #18144 -- Added backwards compatibility with old unsalted MD5 passwords

Thanks apreobrazhensky at gmail.com for the report.

[Claude Paroz <claude@…>]

In c39be8b836282c1060e64dc17c6cb8184c14cf0b:

[1.5.x] Fixed #18144 -- Added backwards compatibility with old unsalted MD5 passwords

Thanks apreobrazhensky at gmail.com for the report.
Backport of 63d6a50dd from master.

[Claude Paroz <claude@…>]

In 6bd3896fcb5c626a5ef613895d52c69130156d3a:

[1.4.x] Fixed #18144 -- Added backwards compatibility with old unsalted MD5 passwords

Thanks apreobrazhensky at gmail.com for the report.
Backport of 63d6a50dd from master.

[Claude Paroz <claude@…>]

In c39be8b836282c1060e64dc17c6cb8184c14cf0b:

[1.5.x] Fixed #18144 -- Added backwards compatibility with old unsalted MD5 passwords

Thanks apreobrazhensky at gmail.com for the report.
Backport of 63d6a50dd from master.

[Gabriel]

The same happens with SHA1PasswordHasher, I didn't think necessary to open a new ticket.

I created three pull requests on github

• 1.4.x : ​https://github.com/django/django/pull/740
• 1.5.x : ​https://github.com/django/django/pull/742
• master: ​https://github.com/django/django/pull/741

Regards.

[Aymeric Augustin]

I don't think Django ever generated unsalted SHA1 passwords. dahool, can you tell us in which version it did?

[Florian Apolloner]

Django switched to salted sha1 (from unsalted md5) in a49fa746cdc056f0b660f47fbc55aa43fcd54bcc -- I can't see where it did use unsalted sha.

[Aymeric Augustin]

Here are the schemes accepted and generated by Django over time.

Version	Read	Written	Code
0.90	unsalted MD5	unsalted MD5	​https://github.com/django/django/blob/stable/0.90.x/django/models/auth.py#L80-L87
0.91	unsalted MD5, unsalted SHA1, salted SHA1	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/0.91.x/django/models/auth.py#L80-L109
0.95	unsalted MD5, unsalted SHA1, salted SHA1	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/0.95.x/django/contrib/auth/models.py#L140-L162 ​https://github.com/django/django/blob/stable/0.95.x/django/contrib/auth/models.py#L8-L20
0.96	unsalted MD5, unsalted SHA1, salted SHA1	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/0.96.x/django/contrib/auth/models.py#L140-L162 ​https://github.com/django/django/blob/stable/0.96.x/django/contrib/auth/models.py#L8-L20
1.0	unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/1.0.x/django/contrib/auth/models.py#L176-L204 ​https://github.com/django/django/blob/stable/1.0.x/django/contrib/auth/models.py#L18-L54
1.1	unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/1.1.x/django/contrib/auth/models.py#L183-L211 ​https://github.com/django/django/blob/stable/1.1.x/django/contrib/auth/models.py#L20-L45
1.2	unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/1.2.x/django/contrib/auth/models.py#L236-L271 ​https://github.com/django/django/blob/stable/1.2.x/django/contrib/auth/models.py#L16-L41
1.3	unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)	salted SHA1, with automatic upgrade of unsalted MD5	​https://github.com/django/django/blob/stable/1.3.x/django/contrib/auth/models.py#L251-L286 ​https://github.com/django/django/blob/stable/1.3.x/django/contrib/auth/models.py#L16-L43

The new password hashing was added in 1.4.

[Aymeric Augustin]

I just realized "unsalted passwords" can mean two things: just a plain SHA1 hash, or sha1$$ followed by a SHA1 hash. The request here is to handle the latter. Historically, Django hasn't checked that the salt isn't empty; now it does, and that's described as a regression here.

Django never generated such hashes and there's no reason to find them in a auth_user table. But that argument also applies to hashes starting with md5$$, and code was added to support that.

Personally I would have wontfix'd the ticket rather that add code to support dubious schemes that Django never generated. But it would be inconsistent to refuse for SHA1 what was accepted for MD5, so at this point, I'll refrain from closing the ticket or changing its "release blocker" status.

[Aymeric Augustin <aymeric.augustin@…>]

In 97a67b26f3debc40c73f835dd17cbef98fe5d8c6:

[1.4.x] Fixed #18144 -- Restored compatibility with SHA1 hashes with empty salt.

Thanks dahool for the report and initial version of the patch.

Backport of 633d8de from master.

[Aymeric Augustin <aymeric.augustin@…>]

In 33fc43895279dc3525031dba2a06ea77b28c90dc:

[1.5.x] Fixed #18144 -- Restored compatibility with SHA1 hashes with empty salt.

Thanks dahool for the report and initial version of the patch.

Backport of 633d8de from master.

[Aymeric Augustin <aymeric.augustin@…>]

In f1255a3c0904a55ef267fa5f8687a1ce78f6894a:

Fixed #18144 -- Restored compatibility with SHA1 hashes with empty salt.

Thanks dahool for the report and initial version of the patch.