Ticket #2394 (closed defect: fixed)

Opened 2 years ago

Last modified 5 months ago

Passwords are stored in an insecure un-salted form

Reported by: sjmurdoch Assigned to: pishmishy
Priority: normal Milestone: 2.5
Component: Security Version: 2.0
Severity: normal Keywords: has-patch salt password md5 phpass needs-testing
Cc: sjmurdoch

Description

Passwords stored in the database are simply the MD5 of the plaintext password, as shown by the following code from wp-includes/functions.php:161

function user_pass_ok($user_login,$user_pass) {
        ...
        return (md5($user_pass) == $userdata->user_pass);
}

If an attacker can gain read-only access to the password database, such as through SQL injection, timing attacks or local compromise, this construction is insecure. The conventional defence against these attacks is salting, as used in the Unix /etc/passwd file.

Unsalted passwords are vulnerable to a number of attacks. A dictionary attack can be applied against all users simulataneously, whereas with salting, each user has to be attacked separately. Also, pre-computed tables can be used to crack unsalted passwords almost instantaneously. Time-space tradeoff attacks, such as those used by RainbowCrack are capable of breaking passwords not vulnerable to dictionary attacks.

Salting effectively defeats these attacks, at almost no cost.

The current contents of wp_users.user_pass are 32 characters in the range [0-9a-f] so a prefix character outside of this could be used to indicate that salting is used. This would allow both schemes to co-exist.

Attachments

2394-salt.patch (5.3 kB) - added by pishmishy on 06/28/07 14:00:25.
Draft of salted passwords
2394-phpass.patch (4.9 kB) - added by pishmishy on 11/23/07 13:06:34.
Implentation of salted passwords through the use of phpass
class-phpass.php (6.4 kB) - added by pishmishy on 11/23/07 13:07:08.
phpass file for wp-includes
pass_hash.diff (5.2 kB) - added by ryan on 11/29/07 07:28:46.

Change History

02/05/06 20:00:49 changed by sjmurdoch

Properly salted hashes can be generated in PHP by crypt()

02/07/06 09:39:14 changed by markjaquith

  • owner changed from anonymous to markjaquith.
  • status changed from new to assigned.

Clever solution to the "upgrade" problem. I'm in favor of this. What determines the salt though? It has to be unchanging.

02/08/06 11:33:13 changed by sjmurdoch

What determines the salt though?

The salt should be chosen randomly when the password is generated. There is no need to store it separately, since it is put in the crypted password.

crypt() will generate a random salt if it is not specified, but generally for DES passwords which is not ideal. With DES, the salt is only two characters and the password is limited to 8 characters. Using CRYPT_MD5, CRYPT_EXT_DES or CRYPT_BLOWFISH is better, but here you are on your own for generating the salt.

Another option is to use phpass from Solar Designer which is a lot easier to use. It defaults to CRYPT_BLOWFISH, then falls back to CRYPT_EXT_DES. If neither is supported it implements it's own hashing function (based on MD5). I am not sure why it doesn't use CRYPT_MD5. I am generally hesitant at using custom cryptography which has not been heavily reviewed, although Solar Designer has been working in this field for a long time and has a good reputation.

On my system (PHP 4.1.2) only CRYPT_STD_DES and CRYPT_MD5 are supported and crypt() defaults to CRYPT_STD_DES if a salt is not specified.

04/27/07 00:28:41 changed by abelcheung

Will this report be completely ignored in the future?

04/27/07 17:18:55 changed by foolswisdom

  • milestone set to 2.4.

06/28/07 10:26:32 changed by pishmishy

  • owner changed from markjaquith to pishmishy.
  • status changed from assigned to new.

06/28/07 10:26:46 changed by pishmishy

  • status changed from new to assigned.

06/28/07 14:00:25 changed by pishmishy

  • attachment 2394-salt.patch added.

Draft of salted passwords

06/28/07 14:09:20 changed by pishmishy

  • keywords set to has-patch.

Attached is a patch file that adds salted passwords to WordPress whilst retaining support for the current plain MD5 scheme. I've not used the crypt() scheme but done simple salting without any additional functions apart from one to generate salt. As suggested I've used the length of the stored password to determine if new salted passwords, or non-salted passwords are being used for this account.

I've chosen to use a short, random alphanumeric string as the salt but there's no reason why password_salt() can't be replaced with something else. A friend suggested that the salt could be overloaded to store other information such as the date the password was last changed but I'm not sure this is worth the slight drop in security it would bring.

This is my largest patch to date and the changes effect authentication, cookies, account changes and password recovery. I've done some simple testing of everything (apart from the changes to user_pass_ok()), both with old and new style password forms and everything appears to work as it should - probably worth further testing though :-)

(follow-up: ↓ 10 ) 06/28/07 15:16:45 changed by Otto42

Minor suggestions:

For PHP versions above 5.1.2, using hash('md5', 'string'); is faster than md5('string'). Might be worth detecting the PHP version and using that instead. Every little bit helps.

For PHP 4 versions, this is faster than md5('string') and returns the same result: bin2hex(md5('string', TRUE));

(in reply to: ↑ 9 ; follow-up: ↓ 11 ) 06/28/07 15:20:23 changed by pishmishy

Replying to Otto42:

Minor suggestions: For PHP versions above 5.1.2, using hash('md5', 'string'); is faster than md5('string'). Might be worth detecting the PHP version and using that instead. Every little bit helps. For PHP 4 versions, this is faster than md5('string') and returns the same result: bin2hex(md5('string', TRUE));

This issue should have it's own ticket. WordPress doesn't only uses md5() hashes in password management.

(in reply to: ↑ 10 ; follow-up: ↓ 12 ) 06/28/07 15:23:24 changed by Otto42

Replying to pishmishy:

This issue should have it's own ticket.

It was a suggestion, not an issue. It doesn't need it's own ticket. I was just commented. Chill.

WordPress doesn't only uses md5() hashes in password management.

As far as I can tell, yes, actually, it does. It uses md5 hashes in the database and double md5 hashes as cookies. Where does it use anything else?

(in reply to: ↑ 11 ; follow-up: ↓ 13 ) 06/28/07 15:35:03 changed by pishmishy

Replying to Otto42:

It was a suggestion, not an issue. It doesn't need it's own ticket. I was just commented. Chill.

Sorry, no offence meant. I was just being overly terse to stay concise.

As far as I can tell, yes, actually, it does. It uses md5 hashes in the database and double md5 hashes as cookies. Where does it use anything else?

In generating the code in the URL used to confirm password recovery, also in bookmark.php, category.php, taxonomy.php, cache.php and tinyMCE, to generate keys that are used in a cache. I'm afraid that I don't know anything about that code.

(in reply to: ↑ 12 ; follow-up: ↓ 14 ) 06/29/07 13:09:26 changed by Otto42

Replying to pishmishy:

In generating the code in the URL used to confirm password recovery

Password recovered is accomplished by generating a new random password and emailing that to the user. And yes, it uses an MD5 of the new random password in the database.

also in bookmark.php, category.php, taxonomy.php, cache.php and tinyMCE, to generate keys that are used in a cache.

I fail to understand your point. Yes, those all use md5 for key generation, but none of that has anything to do with user passwords.

(in reply to: ↑ 13 ; follow-up: ↓ 15 ) 06/29/07 13:17:16 changed by pishmishy

Replying to Otto42:

Password recovered is accomplished by generating a new random password and emailing that to the user. And yes, it uses an MD5 of the new random password in the database.

It's also used in to generate the occurrence of c6d0fbc7 in /wp-login.php?action=rp&key=c6d0fbc7 (for example).

I fail to understand your point. Yes, those all use md5 for key generation, but none of that has anything to do with user passwords.

If we decide that there are faster ways to generate an md5 hash than through md5() then would it not make sense to make the change across the code and not just where it's involved with passwords?

(in reply to: ↑ 14 ) 06/29/07 13:34:57 changed by Otto42

Replying to pishmishy:

If we decide that there are faster ways to generate an md5 hash than through md5() then would it not make sense to make the change across the code and not just where it's involved with passwords?

OOOOHHHHHHH! Okay. Sorry, you had me completely confused at first. Your original sentence structure was very weird, I thought you were saying that it used something other than MD5 for passwords somewhere.

11/21/07 00:28:11 changed by pishmishy

  • keywords changed from has-patch to has-patch salt password md5.

(follow-up: ↓ 18 ) 11/22/07 19:46:02 changed by ryan

phpass seems flexible and portable and has a compatible license. Why not use it? I'd rather not reinvent what someone more knowledgeable in the field has already done.

(in reply to: ↑ 17 ) 11/22/07 21:46:07 changed by westi

  • keywords changed from has-patch salt password md5 to needs-patch salt password md5.

Replying to ryan:

phpass seems flexible and portable and has a compatible license. Why not use it? I'd rather not reinvent what someone more knowledgeable in the field has already done.

Agreed. Marking as needs-patch

11/23/07 13:06:34 changed by pishmishy

  • attachment 2394-phpass.patch added.

Implentation of salted passwords through the use of phpass

11/23/07 13:07:08 changed by pishmishy

  • attachment class-phpass.php added.

phpass file for wp-includes

11/23/07 13:13:26 changed by pishmishy

  • keywords changed from needs-patch salt password md5 to has-patch salt password md5 phpass needs-testing.

I've attached a patch that achieves the same using phpass instead - it not that different to how I was salting passwords. I've gone for the portable, MD5 based hash it provides, leaving the option to switch to other hash functions when they are more widely available.

I've tested the patch with old style passwords, a new installation, and a new user and all appears to work as intended. Testing on Windows may be in order - phpass appears to attempt a different source of random data in that case.

11/28/07 18:28:49 changed by ryan

That patch looks pretty good. I'll give it some testing.

11/29/07 07:28:46 changed by ryan

  • attachment pass_hash.diff added.

(follow-up: ↓ 22 ) 11/29/07 07:32:49 changed by ryan

I modified 2394-phpass.patch to abstract password hashing and checking into functions -- wp_check_password() and wp_hash_password(). These functions are pluggable so if someone doesn't like phpass they can plug in their own hasher.

Also, upon successful login using a plaintext password, old hashes are replaced with phpass hashes.

(in reply to: ↑ 21 ) 11/29/07 07:46:32 changed by DD32

Replying to ryan:

Also, upon successful login using a plaintext password, old hashes are replaced with phpass hashes.

When this change goes in, Be sure to remind everyone that unless they have a backup of the database, If they wish to downgrade to a previous revision, or version (Once released) they'll need to reset all passwords. I can see that as being something small which initial testers(maybe RC's) will ignore at first.

11/29/07 15:43:45 changed by pishmishy

Pluggable functions look good. See also #5401 - vaguely related.

12/02/07 05:14:11 changed by ryan

(In [6350]) Hash passwords with phpass. Add wp_check_pasword() and wp_hash_password() functions. Props pishmishy. see #2394

12/03/07 17:39:07 changed by ryan

BTW, it looks like Drupal will be using phpass in a future release. Check out the discussion here in which the author of phpass explains the benefits of phpass.

http://drupal.org/node/29706

12/05/07 10:21:08 changed by pishmishy

  • status changed from assigned to closed.
  • resolution set to fixed.

12/17/07 06:02:46 changed by ryan

(In [6396]) wp_set_password(). see #2394