Project

General

Profile

Actions

Bug #39356

closed

Converting MD5 Password Hashes to SaltedPasswords using Blowfish fails

Added by Steffen Ritter over 11 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Must have
Category:
Authentication
Target version:
Start date:
2012-07-29
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
4.5
PHP Version:
5.3
Tags:
Complexity:
easy
Is Regression:
No
Sprint Focus:

Description

A Salt with BlowFish uses 60 chars.
When converting from MD5, an M is added upfront.
As the Database field is only 60 chars long login is not possible anymore - the salt is truncated.


Files

Actions #1

Updated by Gerrit Code Review over 11 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/13324

Actions #2

Updated by Gerrit Code Review over 11 years ago

Patch set 2 for branch master has been pushed to the review server.
It is available at http://review.typo3.org/13324

Actions #3

Updated by Gerrit Code Review over 11 years ago

Patch set 1 for branch TYPO3_4-7 has been pushed to the review server.
It is available at http://review.typo3.org/13331

Actions #4

Updated by Steffen Ritter over 11 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #5

Updated by Gerrit Code Review over 11 years ago

  • Status changed from Resolved to Under Review

Patch set 2 for branch TYPO3_4-7 has been pushed to the review server.
It is available at http://review.typo3.org/13331

Actions #6

Updated by Steffen Ritter over 11 years ago

  • Status changed from Under Review to Resolved
Actions #7

Updated by Elliot Sawyer over 10 years ago

Hello,
This patch was not included with the latest 4.5.29 release. When the converter task converts MD5 into Blowfish, the resulting hash is 61 characters long. Because the password field is only a varchar(60), the last character is truncated and comparisons to crypt() output will fail. You can confirm this in the latest 4.5.29 release on the Git repository: https://git.typo3.org/Packages/TYPO3.CMS.git/log/ffa8250573d4d0c421e6ea7b4c019ff9f111091f

Please include this patch in the next maintenance release

Actions #8

Updated by Wouter Wolters over 10 years ago

There is a reason for this not be backported. We do not allow database changes is maintenance releases.
The message fromt the patch that was pushed into review and is back then abandoned for the reason described above.

Christian Kuhn Jan 28, 2013

Patch Set 1: Do not submit

Will not be merged for 4.6, and also not for 4.5

Actions #9

Updated by Elliot Sawyer over 10 years ago

Respectfully, this extension is broken without the database update. Some warning in the documentation to not execute the scheduler task on 4.5 or 4.6 would be helpful, or a check within the code that prevents the conversion if field is not long enough. Otherwise, any account the task modifies will be prevented from logging in, and there is no warning as to the cause of the error.

I have created a patch which prevents the task from converting passwords if the field is not long enough, and a warning in the backend if this error occurs.

Actions #10

Updated by Patrick Schriner over 9 years ago

Wouter Wolters wrote:

There is a reason for this not be backported. We do not allow database changes is maintenance releases.
The message fromt the patch that was pushed into review and is back then abandoned for the reason described above.

Christian Kuhn Jan 28, 2013

Patch Set 1: Do not submit

Will not be merged for 4.6, and also not for 4.5

As someone who just broke all backend user access due to the utterly idiotic decision that lead to this patch not being merged into 4.5 or 4.6:

WTF are you smoking.

If I ever need a good example of what is wrong in TYPO3, I'll at least have an amazing example. Thanks.

And why does the commit message imply that this was fixed for 4.5 or 4.6. That's even more misleading crazy.

Actions #11

Updated by Helmut Hummel over 9 years ago

  • Is Regression set to No

Wouter Wolters wrote:

Will not be merged for 4.6, and also not for 4.5

I disagree. We merged such no impact DB changes previously and I see no reason not to merge this in 4.5, especially regarding the impact when this is not fixed.

Can you elaborate what the reasons are not to merge it (besides the policy)? What is the impact if we would do so?

Actions #12

Updated by Wouter Wolters over 9 years ago

I backported the changes initially but they were denied by 2 core devs, where one is a release manager, see below answer from Xavier in the 4.6 backport:

Xavier Perseguers Jan 28, 2013

Patch Set 1: Do not submit

I consider this change as totally not critic and dislike having to see DB structure changes in a minor (well, for 4.6 it will even be security) update.

People running MySQL wouldn't have too much troubles but when using DBAL, it's really less enjoying and having this additional stress during a security update is inadequate if not absolutely mandatory.

Actions #13

Updated by Christian Kuhn over 9 years ago

Yeah. And there were further (2 year old!) reasons:

  • At this point in time we already had very late DB changes in 4.5 and got hit by multiple people like "hey idiots, do not change db layouts in minor releases"
  • With db changes, everyone is hit, while this specific issue probably affects only a very small faction of people
  • This specific issue, pops up only if an anyway critical task is run that is hopefully covered by a backup for safety reasons anyway. And it is hopefully tested on non-production systems before?!
  • saltedpasswords was at that time not as popular as it is now (after we finally made it mandatory in 6.2)
  • blowfish is not the default hash algorithm, and it seemed pretty unlikely someone understands the impact at this point and changes the defaults - without backup and testing this scenario in 4.5 before!

Therefore, we decided not to backport this specific issue.

Actions #14

Updated by Helmut Hummel over 9 years ago

Hi Christian!

Thanks for giving some arguments here so quickly.

First off: It may be a bit late with integrating such a change now, as the group of people now starting with a 4.5 version is most likely very small.

But still I would like to have this cleared up for future changes like that.
I maybe forgot the reasoning of a specific thing (or remember it differently)

Christian Kuhn wrote:

  • At this point in time we already had very late DB changes in 4.5 and got hit by multiple people
  • With db changes, everyone is hit

What is the impact of (not required) DB changes like this or similar ones where the DB field is extended?
People who do not change the DB after a minor update and do not use blowfish will not suffer any issues, do they?
People who do a new install with a fixed version and start with a fresh (correct) db schema will be able to use blowfish conversion

Actions #15

Updated by Helmut Hummel over 9 years ago

Hi Patrick!

Thanks for your feedback.

Patrick Schriner wrote:

As someone who just broke all backend user access

Sad to hear that.

WTF are you smoking.

Strong language will be of no help (neither for you nor for anybody) in any case and it violates our code of conduct.
Please consider your words for future contributions. Thanks.

For this specific thing I would have decided differently back then (and even remembered a different decision on that), but I respect the decision and can understand the reasoning behind it. I would also not fix this any more now as the benefits would be low.

Lets look froward and improve things for the future instead.

Actions #16

Updated by Riccardo De Contardi over 6 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF