Make WordPress Core

Opened 18 years ago

Closed 16 years ago

Last modified 16 years ago

#2394 closed defect (bug) (fixed)

Passwords are stored in an insecure un-salted form

Reported by: sjmurdoch's profile sjmurdoch Owned by: pishmishy's profile pishmishy
Milestone: 2.5 Priority: normal
Severity: normal Version: 2.0
Component: Security Keywords: has-patch salt password md5 phpass needs-testing
Focuses: Cc:

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 (4)

2394-salt.patch (5.3 KB) - added by pishmishy 17 years ago.
Draft of salted passwords
2394-phpass.patch (4.9 KB) - added by pishmishy 16 years ago.
Implentation of salted passwords through the use of phpass
class-phpass.php (6.4 KB) - added by pishmishy 16 years ago.
phpass file for wp-includes
pass_hash.diff (5.2 KB) - added by ryan 16 years ago.

Download all attachments as: .zip

Change History (31)

#1 @sjmurdoch
18 years ago

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

#2 @markjaquith
18 years ago

  • 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.

#3 @sjmurdoch
18 years ago

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.

#4 @abelcheung
17 years ago

Will this report be completely ignored in the future?

#5 @foolswisdom
17 years ago

  • Milestone set to 2.4

#6 @pishmishy
17 years ago

  • Owner changed from markjaquith to pishmishy
  • Status changed from assigned to new

#7 @pishmishy
17 years ago

  • Status changed from new to assigned

@pishmishy
17 years ago

Draft of salted passwords

#8 @pishmishy
17 years ago

  • Keywords has-patch added

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 :-)

#9 follow-up: @Otto42
17 years ago

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));

#10 in reply to: ↑ 9 ; follow-up: @pishmishy
17 years ago

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.

#11 in reply to: ↑ 10 ; follow-up: @Otto42
17 years ago

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?

#12 in reply to: ↑ 11 ; follow-up: @pishmishy
17 years ago

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.

#13 in reply to: ↑ 12 ; follow-up: @Otto42
17 years ago

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.

#14 in reply to: ↑ 13 ; follow-up: @pishmishy
17 years ago

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?

#15 in reply to: ↑ 14 @Otto42
17 years ago

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.

#16 @pishmishy
16 years ago

  • Keywords salt password md5 added

#17 follow-up: @ryan
16 years ago

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.

#18 in reply to: ↑ 17 @westi
16 years ago

  • Keywords needs-patch added; has-patch removed

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

@pishmishy
16 years ago

Implentation of salted passwords through the use of phpass

@pishmishy
16 years ago

phpass file for wp-includes

#19 @pishmishy
16 years ago

  • Keywords has-patch phpass needs-testing added; needs-patch removed

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.

#20 @ryan
16 years ago

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

@ryan
16 years ago

#21 follow-up: @ryan
16 years ago

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.

#22 in reply to: ↑ 21 @DD32
16 years ago

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.

#23 @pishmishy
16 years ago

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

#24 @ryan
16 years ago

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

#25 @ryan
16 years ago

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

#26 @pishmishy
16 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#27 @ryan
16 years ago

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

Note: See TracTickets for help on using tickets.