Code

Opened 2 years ago

Closed 14 months ago

Last modified 14 months ago

#18144 closed Bug (fixed)

MD5PasswordHasher: broken backwards compatibility with empty salt

Reported by: apreobrazhensky@… Owned by: nobody
Component: contrib.auth Version: 1.4
Severity: Release blocker Keywords: MD5PasswordHasher check_password salt
Cc: slav0nic0@…, carsten.fuchs@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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)

Attachments (1)

18144-1.diff (1.4 KB) - added by claudep 2 years ago.
Handle md5$$... syntax

Download all attachments as: .zip

Change History (23)

comment:1 Changed 2 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

See #17777.

comment:2 Changed 2 years ago by apreobrazhensky@…

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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.

comment:3 Changed 2 years ago by aaugustin

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

Changed 2 years ago by claudep

Handle md5$$... syntax

comment:4 Changed 2 years ago by claudep

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 2 years ago by anonymous

This is also true for SHA1PasswordHasher

comment:6 Changed 18 months ago by slav0nic

  • Cc slav0nic0@… added

comment:7 Changed 17 months ago by CarstenF

  • Cc carsten.fuchs@… added

comment:8 Changed 15 months ago by claudep

#19687 is a duplicate.

comment:9 Changed 15 months ago by aaugustin

  • Patch needs improvement set

The patch looks good, but it no longer applies.

comment:10 Changed 15 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

In 63d6a50dd8ab4f47c9ae11c1d2a4c4c7eed06be6:

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

Thanks apreobrazhensky at gmail.com for the report.

comment:11 Changed 15 months ago by 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.

comment:12 Changed 15 months ago by 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.

comment:13 Changed 15 months ago by 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.

comment:14 Changed 14 months ago by dahool

  • Resolution fixed deleted
  • Status changed from closed to new

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

I created three pull requests on github

Regards.

comment:15 Changed 14 months ago by apollo13

  • Severity changed from Normal to Release blocker

comment:16 Changed 14 months ago by aaugustin

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

comment:17 Changed 14 months ago by apollo13

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

comment:18 Changed 14 months ago by aaugustin

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

VersionReadWrittenCode
0.90unsalted MD5unsalted MD5https://github.com/django/django/blob/stable/0.90.x/django/models/auth.py#L80-L87
0.91unsalted MD5, unsalted SHA1, salted SHA1salted SHA1, with automatic upgrade of unsalted MD5https://github.com/django/django/blob/stable/0.91.x/django/models/auth.py#L80-L109
0.95unsalted MD5, unsalted SHA1, salted SHA1salted SHA1, with automatic upgrade of unsalted MD5https://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.96unsalted MD5, unsalted SHA1, salted SHA1salted SHA1, with automatic upgrade of unsalted MD5https://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.0unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)salted SHA1, with automatic upgrade of unsalted MD5https://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.1unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)salted SHA1, with automatic upgrade of unsalted MD5https://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.2unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)salted SHA1, with automatic upgrade of unsalted MD5https://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.3unsalted MD5, unsalted SHA1, salted SHA1, salted crypt (if supported by Python)salted SHA1, with automatic upgrade of unsalted MD5https://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.

comment:19 Changed 14 months ago by aaugustin

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.

comment:20 Changed 14 months ago by Aymeric Augustin <aymeric.augustin@…>

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

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.

comment:21 Changed 14 months ago by 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.

comment:22 Changed 14 months ago by 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.

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.