Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#29952 closed Bug (fixed)

All passwords in contrib/auth/common-passwords.txt.gz should be lowercased

Reported by: Mathew Payne Owned by: Mathew Payne
Component: contrib.auth Version: 2.1
Severity: Release blocker Keywords: password, validation
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

There is a bug with the "CommonPasswordValidator" where the password validation step is done using password.lower() to make sure that case sensitivity isn't an issue in passwords.
497 of the passwords stored in the Django provided file (django/contrib/auth/common-passwords.txt.gz) are stored containing uppercase characters which means that 497 of the passwords are not checked correctly when validating.

Here are some examples of passwords in the file:

  • VQsaBLPzLa
  • FQRG7CS493
  • DIOSESFIEL
  • 3rJs1la7qE
  • ...

Here is a small test to prove that the the password validation is not working as it should:

tests/auth_tests/test_validators.py

class CommonPasswordValidatorTest(TestCase):
    def test_password_validation_issue(self):
        # using standard list
        validator = CommonPasswordValidator()
        # is the password in the list? Yes.
        self.assertIn('VQsaBLPzLa', validator.passwords)
        # check if the validation function throws an error. It doesn't
        with self.assertRaises(ValidationError) as cm:
            self.assertIsNone(validator.validate('VQsaBLPzLa'))

And I get this Unit test error:

======================================================================
FAIL: test_password_validation_issue (auth_tests.test_validators.CommonPasswordValidatorTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.6/unittest/case.py", line 605, in run
    testMethod()
  File "/user/geekmasher/django/tests/auth_tests/test_validators.py", line 193, in test_password_validation_issue
    self.assertIsNone(validator.validate('VQsaBLPzLa'))
  File "/usr/lib/python3.6/unittest/case.py", line 203, in __exit__
    self._raiseFailure("{} not raised".format(exc_name))
  File "/usr/lib/python3.6/unittest/case.py", line 135, in _raiseFailure
    raise self.test_case.failureException(msg)
AssertionError: ValidationError not raised

Recommendation:

  • All passwords in the Django supplied list should be lowered to match the validation phase.
    • Duplication's should be removed from the Django list, if any.
  • If a user supplied path is given, the user can request to auto_lower the lists items on init to match the validation.
    • If it's lowered every time a user supplied path is provided (even if they are lowered already), the performance of the function could be significant slower.

I will be submitting a Pull Request shortly.

Change History (5)

comment:1 Changed 11 months ago by Mathew Payne

Owner: changed from geekmasher@… to Mathew Payne

comment:2 Changed 11 months ago by Tim Graham

Patch needs improvement: set
Severity: NormalRelease blocker
Summary: CommonPasswordValidator with uppercase passwords in common-passwords.txt.gzAll passwords in contrib/auth/common-passwords.txt.gz should be lowercased
Triage Stage: UnreviewedAccepted
Version: master2.1

comment:3 Changed 11 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 26bb261:

Fixed #29952 -- Lowercased all passwords in contrib.auth's auth/common-passwords.txt.gz.

comment:4 Changed 11 months ago by Tim Graham <timograham@…>

In 2128c508:

[2.1.x] Fixed #29952 -- Lowercased all passwords in contrib.auth's auth/common-passwords.txt.gz.

Backport of 26bb2611a567d43bc258aa7806eef766b7adcfe5 from master.

comment:5 Changed 11 months ago by Adam (Chainz) Johnson

Great work Mathew. You also discovered there is some duplication in the file too right? That could be removed too, and a test added to check that it isn't re-added, in a new ticket now this is closed.

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