Harden one-time login links against vulnerability from disclosure of SQL backups, or SQL 'SELECT' injection

This approach could be optional, because in standard installations, following sysadmin best practices, SQL injection and the loss of the DB dump is uncommon. My Proposal:

0. On password reset request, send a similar link to the requesting users email address as in the current version
1. Let this link include some random magic number to prevent password resets using DB dumps. These magic numbers can be stored until expiration in the database.
2. If the user follows the link, instead of allowing him to change his pass, send another email to his email address with a random generated, strong password
(3. After logging in with the new password, force the user to change it)

Don't reset the users password until he follows the first link! This prevents malicious attackers from disabling logins by requesting password resets.

I suggest we just generate a nonce and:

* send it to the client in plaintext,
* but hash it like a password when storing it in the database (in a separate table, not in the user table).

I can agree with this too.

I didn't know about this sry, anyway, Damiens proposal seems much better to me than mine.

The "out-of-DB" way seems not that attractive because on many systems SQL SELECT's allow you to read files with the privileges of the mysqld (or whatever DBMS you use). On proper configurations the DBMS would only access to its own files, but this is not an everyday situation...

Ouch, that fileinode() seems hard job (possibly impossible) from SQL, and from some leaked backup too, very nice!

However, I still won't recommend the usage of the _content_ of some file, because of the difficulities of the proper server configuration - most of the users won't think about this issue, won't care about that file, and won't revoke the DBMS's access to it (sometimes they are even restricted to do that), and we are back to where we started from.

OK, even the thousand miles journey starts with one step, I just want to reveal the possible attack surfaces.

#600718: Improve the password reset functionality marked as duplicate

My idea would be to send random "reset code" in the password reset email, and add a column to {users} table (or create new table). On the one time login page the user needs to enter the reset code to access his account.
the db column could be something like password_reset_hash = some_hash($reset_code . $something)

pros:
The password reset link could remain the same as it is now.
The reset code in the email is required to reset the password.
This is much better than a secret word in the settings.php
Can not reset the password without email. Right now you can create a one time login link without sending any email. (check the issue #600718)

cons?:
yes, it is a db write when requesting a password reset. Maybe with a new table (instead of new column) the table could be moved to a faster storage.

I am not sure what embedding the code in the link means for you, and I am quite confused with the comments.

(a) /user/reset/1/1255145008/26b5a387b0634b59cb23f3893bda6e7f &code=DE522573
(b) or just changing the hash value?

I am talking about option (a) where this reset code is not stored in the db (just the hash($reset_code) is stored) and generated with every new request, so the attacker does not have any chance to reset the password without the email (even if he has a read access to db and settings.php).

I do not know which problems are you referring to as the only one seems that under DOS attack you can not use the password reset function. Is that really an issue?

working reset code
This patch is submitted as an explanation of the idea, not a final solution..

Yes, I have read the comments, but to be honest I have no clue what are you talking about. Why would a one time login help is case of DoS, and which change in the patch break it? But I am not really eager to know the answer, I just wanted to show what could be a possible solution. If you say it does not work in some cases I believe you..

This looks really good to me as a first step to hardening the one-time login process. To describe for new readers, the patch in #25 creates a settings.php variable that's used for salting one-time login links. The hash is created during update/install. If the hash isn't set the patch defaults to a hashed serialization of the databases setting. You can also specify the contents of a file to use instead.

My only reason for not setting to RTBC is I think there is value in generalizing the variable name. A file-level hash could be useful for developers similarly needing a salt not stored in the DB. Instead of $user_pass_rehash_salt how about just $salt?

I agree with coltrane.

The patch itself looks good, but I think we should use a generic name for the salt variable, as it may be reused in other contexts.

Offers:
$system_salt
$drupal_system_salt
$drupal_system_key

Waiting on testbot but if it checks out I think this is RTBC

+++ includes/common.inc
@@ -4500,7 +4513,7 @@ function drupal_get_token($value = '') {
-  return (($skip_anonymous && $user->uid == 0) || ($token == md5(session_id() . $value . variable_get('drupal_private_key', ''))));
+  return (($skip_anonymous && $user->uid == 0) || ($token == drupal_get_token($value)));

Great catch to use drupal_get_token() here.

1 days to code freeze. Better review yourself.

Looking good and tests pass.

+++ includes/common.inc
@@ -4482,7 +4495,7 @@ function drupal_get_private_key() {
   $private_key = drupal_get_private_key();
-  return md5(session_id() . $value . $private_key);
+  return md5(drupal_get_hash_salt() . md5(session_id() . $value . $private_key));

Any reason we call md5() twice? If so, please add a code comment.

Looks great now. Committed to CVS HEAD. Thanks.

subscribing

subscribing

subscribe and bump to backport

Is this tested on PHP 4? Who tested the patch?

I've looked over the patch and don't see why it would fail on php4, but can't test it.

It uses sha1 in one place but uses md5 twice in other places. Should probably be standardized.

fixed global $db_url variable.