Currently, Drupal 6 does not salt passwords. This is completely unacceptable in any project that takes security seriously. I know that Drupal 7 fixes this issue, but that still leaves Drupal 6 with broken security.

This issue can be fixed rather easily, and the fix I am suggesting below is backwards compatible, and painless to both users and site admins.

Suggested fixes, in order of ease of implementation:

Add salt, only rehash on password change

Implementation:

  • Add a column 'salt' to the user table.
  • When logging in, prepend the stored salt to the password before hashing
  • When setting/changing password, generate a random salt, prepend it to the password, hash the combined value, and store salt+hash in the db

Impact and backwards compatibility:

  • As the 'salt' field is initially empty when added to the table, all existing users on all existing Drupal installations will continue to be able to log in, because we are prepending an empty string.
  • When changing existing passwords, or adding new users, those passwords will now be salted. Security is thus improved.
  • The process does not impact either users or admins in any negative way.

Additionally rehash on successful login

Implementation as above, but additionally:

  • Upon successful login, check if the salt field is empty. If so, take the password just used to successfully log in, salt and hash it, and store salt and new hash in the database.

Impact and backwards compatibility:

  • Passwords will become salted much more quickly this way, so security is improved more rapidly.
  • The update upon login is completely transparent to users and administrators, there is no serious disadvantage to doing it.
CommentFileSizeAuthor
#5 salting_for_drupal6_with_update.patch2.16 KBHerman Hiddema
#3 salting_for_drupal6.patch1.41 KBHerman Hiddema
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Active » Closed (won't fix)

Drupal 6 is feature frozen.

The added security is minimal (if some unauthorized person has access to the user database, you are screwed anyway), but you can use http://drupal.org/project/phpass for Drupal 6.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented
Status: Closed (won't fix) » Active

If some authorized person has access to the database, I'm screwed as well. For example, if the DBA for my hosting company can see the unsalted md5 hashed passwords of my users, they can recover many of those passwords, which are privacy sensitive. Same problem with anyone seeing backups.

Also, this is not a feature request, but a bug report.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented
FileSize
salting_for_drupal6.patch1.41 KB

Ok, here's a patch that implements the first part of my proposal. Add a varchar(16) 'salt' column to the 'users' table (not null, default empty) and apply this patch and you have salting for all new users, and for any user that changes their password, while remaining backwards compatible with all old users.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented
Status: Active » Needs review
Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented
FileSize
salting_for_drupal6_with_update.patch2.16 KB

And here is a patch that implements the second part of my proposal. It not only adds salt to new and updated passwords, but also rehashes after successful login if the salt is empty.

figaro’s picture

figaro CreditAttribution: figaro commented

Cant vouch for the correctness or effectiveness of your patch, but I really think it is best if you run this by the security team: http://drupal.org/node/32750

Heine’s picture

Heine CreditAttribution: Heine commented

[...] think it is best if you run this by the security team [...].

No need. There's been a long (_looong_) public discussion about password hashing/salting on http://drupal.org/node/29706.

The solution there is the only one that should be acceptable for D6. If it is included is up to the branch maintainers.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented

Re: Heine
Yes, I have read that discussion, but that one is about a whole slew of changes, such as stretching, and upgrading from MD5 to a more secure hashing algorithm. Very good, but all of that is a lot more work.

And then you have to consider backwards compatibility with PHP4. Pushing the issue back to Drupal 7 allowed developers to focus on PHP 5 only, which gives a lot more options when it comes to hashing algorithms. But that does mean that the solution developed at http://drupal.org/node/29706 is probably not backwards compatible with PHP4, and thus not backwards compatible with Drupal 6. Which means that the branch maintainers cannot include it.

These patches only address the salting issue, and are fully compatible with php4.

Heine’s picture

Heine CreditAttribution: Heine commented

Yes, I have read that discussion, but that one is about a whole slew of changes, such as stretching, and upgrading from MD5 to a more secure hashing algorithm. Very good, but all of that is a lot more work.

So what if it is a bit more work? The cryptographics have been done in that issue. Why aim lower?

And then you have to consider backwards compatibility with PHP4. Pushing the issue back to Drupal 7 allowed developers to focus on PHP 5 only, which gives a lot more options when it comes to hashing algorithms. But that does mean that the solution developed at http://drupal.org/node/29706 is probably not backwards compatible with PHP4, and thus not backwards compatible with Drupal 6. Which means that the branch maintainers cannot include it.

That is speculation. If we identify PHP 4 issues, what stops us from working around them?

If we are going to change this in a stable release, lets do it properly.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Category: bug » feature
Status: Needs review » Needs work

Let's classify this properly. This is a feature request, not a bug. Whether or not it is accepted is up to the branch maintainer.

Back to needs work anyway based on #9 and the fact that there are code style issues in the posted patches.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented

So what if it is a bit more work? The cryptographics have been done in that issue. Why aim lower?

I have no problem with the work. But I think that salting, stretching, and hashing algorithm strenght are three separate issues. And in its current state, I think that the missing salting is a serious issue, something that is much more urgent to fix than the other issues. If the work done at http://drupal.org/node/29706 is done and ready to be incorporated into drupal 6, then I'm all for it. But there doesn't seem to be any drive, at all, to get it into drupal 6. The issue is for 7.x dev and is closed.

That is speculation. If we identify PHP 4 issues, what stops us from working around them?

I don't know, what is stopping people from putting salting in drupal 6? If you feel it is better that way, then I'm fine with changing this bug to be about incorporating the work done for drupal 7 into drupal 6. But as it is, there is no open issue about that, and there seems to be no real interest for it.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented

Let's classify this properly. This is a feature request, not a bug. Whether or not it is accepted is up to the branch maintainer.

Well, I guess that is a point of view. I disagree. In my personal opinion, not salting your passwords is a serious bug. It is something that is broken and urgently needs to be fixed, not just something that would be nice to have.

Herman Hiddema’s picture

Herman Hiddema CreditAttribution: Herman Hiddema commented
Category: feature » bug

Reclassifying as a bug for the reason given above.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Category: bug » feature

I stand by the feature request classification. I personally consider that protecting passwords have a very low marginal value. The only useful thing is to reasonably protect the confidentiality of the password. Given that contrib can implement different password hashing mechanisms if it wants to.

donquixote’s picture

donquixote CreditAttribution: donquixote commented

I would like to see password salt in D6, yes.

Please also have a look at #595034: Custom password hash functions (per user), which tries to tackle the problem of importing user tables from other projects with different hash functions.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Closed (duplicate)

This is actually a duplicate of #29706: More secure password hashing.