Fixed #20138 -- Added BCryptSHA256PasswordHasher #958

Merged
merged 2 commits into from Mar 26, 2013

Conversation

Projects
None yet
5 participants
Member

dstufft commented Mar 26, 2013

BCryptSHA256PasswordHasher pre-hashes the users password using SHA256 to prevent the 72 byte truncation inherient in the BCrypt algorithm.

docs/topics/auth/passwords.txt
+.. admonition:: Password truncation with BCryptPasswordHasher
+
+ The designers of bcrypt truncate all passwords at 72 characters which means
+ that ``bcrypt(password_with_100_chars ) == bcrypt(password_with_100_chars[:72])``.
@alex

alex Mar 26, 2013

Member

Stray before)

docs/topics/auth/passwords.txt
+ that ``bcrypt(password_with_100_chars ) == bcrypt(password_with_100_chars[:72])``.
+ The original BCryptPasswordHasher did not have any special handling and thus
+ is also subject to a hidden effective password length limit. To combat this
+ a new password hasher (BCryptSHA256PasswordHasher) has been created which
@alex

alex Mar 26, 2013

Member

Use class reference reST syntax here (and above)

@dstufft

dstufft Mar 26, 2013

Member

I don't think I can use the reference syntax because the hashers themselves are not documented (and thus the reference has nothing to link too).

Member

alex commented Mar 26, 2013

At least stick it in ``, so that it shows up in the code formatting.

docs/topics/auth/passwords.txt
+ that ``bcrypt(password_with_100_chars) == bcrypt(password_with_100_chars[:72])``.
+ The original BCryptPasswordHasher did not have any special handling and thus
+ is also subject to a hidden effective password length limit. To combat this
+ a new password hasher (BCryptSHA256PasswordHasher) has been created which
@mjtamlyn

mjtamlyn Mar 26, 2013

Member

"new" only makes sense for a while...

Member

dstufft commented Mar 26, 2013

Addred `` to make class named show up in code formatting, removed the "new" from the admonition, and added a section to the release notes.

Member

mjtamlyn commented Mar 26, 2013

Would the intention be to remove or deprecate the old hasher at some point? It seems like a fairly significant flaw with it. Alternatively perhaps it should have a check that passwords are <72 characters?

Member

dstufft commented Mar 26, 2013

We cannot remove the old hasher because otherwise passwords that are hashed with it cannot be automatically upgraded to a better hash. So it will stick around and just be not recommended like the other hashes. The practicality of this flaw is pretty small since even brute forcing the first 72 characters of a password hashed with bcrypt would take an astronomical amount of time.

Member

dstufft commented Mar 26, 2013

Related Pull Requests:

  • Documentation for 1.5 #961
  • Documentation for 1.4 #962
docs/topics/auth/passwords.txt
+ thus is also subject to a hidden effective password length limit. To combat
+ this a password hasher (``BCryptSHA256PasswordHasher``) has been created
+ which first hashes the password using sha256. This prevents the password
+ truncation and should be preferred over the original ``BCryptPasswordHasher``.
@jacobian

jacobian Mar 26, 2013

Member

Minor edit:

``BCryptPasswordHasher`` does not have any special handling and
thus is also subject to this hidden password length limit. To combat
``BCryptSHA256PasswordHasher`` fixes this by first first hashing
the password using sha256. This prevents the password truncation
and so should be preferred over the ``BCryptPasswordHasher``.

dstufft added some commits Mar 26, 2013

Fixed #20138 -- Added BCryptSHA256PasswordHasher
BCryptSHA256PasswordHasher pre-hashes the users password using
SHA256 to prevent the 72 byte truncation inherient in the BCrypt
algorithm.

dstufft added a commit that referenced this pull request Mar 26, 2013

Merge pull request #958 from dstufft/prehash-bcrypt
Fixed #20138 -- Added BCryptSHA256PasswordHasher

@dstufft dstufft merged commit c1d4af6 into django:master Mar 26, 2013

@dstufft dstufft deleted the dstufft:prehash-bcrypt branch Mar 26, 2013

Typo: forgotten quote

Member

dstufft replied Mar 26, 2013

Fixed here f2a0be6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment