More secure password hashing

Recategorizing, this has nothing to do with the (contributed) security.module. But I'll probably add a TODO-item to check for really simple passwords / MD5 hashes in security.module...

I would suggest using the user's nickname and id as salt. As every user has his own salt, this method should be secure enough.

The only problem here is that the user needs a new password when an administrator changes his nickname. If the user himself wants to change his name, he must provide his password in order to allow recalcing the password.

If someone submits a hash to an online collision db and get back the password, all they have is a password. If they submit it and get back the username+password, that's worse IMO, since they can take that to log in right away. I think the real solution is to change the hashing algorithm, which might be a nightmare for people upgrading.

Given the relatively conservative adoption of new programming features in Drupal, sha1 would be the only php4 backwards-compatible option, which is only a little better than md5, though I haven't seen any sha1 databases online (yet). The upgrading effort probably isn't worth it to most people for just sha1.

If you're that concerned, maybe you should look into hacking up something with the mhash extension.

We could use the $user->created timestamp + username as the hash. This would never make it into the online databases.

Is this still an issue?

Yes, but I bet since any changes from this would be painful for admins, this will be open for a while.

If we adds salt to sums, shouldn't we implement the Sha1 algoritm aswell? Or will that brake the backwards compability?

crc32, md5, sha1, and crypt are the only PHP4 built-in hashing functions. crypt has blowfish as an option, but not on win32. using sha1 would increase the collision liklihood to 2^29 from MD5's 2^40 liklihood. crypt could use a salted md5.

I don't see why we have to change the hashing algorithm at all. If two different passwords result in the same hashes, it simply doesn't matter. Additionally, you would have to use *very* long passwords to get a collision which is practically impossible.

The original issue was not changing the hashing algorithm but preventing stolen Drupal user databases from being reverse engineered. A simple salt is enought for that task.

lots of talk, no code. :p

It should be easy to generate salted passwords using the secret key each Drupal install has.

check out variable_get('drupal_private_key', '')

I would not use the private key. It can be changed too easily. Also, if you want to, say, merge two Drupal sites, you will have a problem as the private keys are different. If you use the creation date as a salt, that problem won't be there anymore and user tables are easily interchangeable.

Salt is ment to make sure that the md5-hash doesn't show up in md5-databases (1) and to make sure that two identical passwords on 1 site do not generate the same hash (2). Because of (2) salt has to be unique for every password, so the private_key can't be used as salt.

The salt does not need to be secret. In most implementations it is stored together with the actual password. The 'created'-field or an extra field in the user-database looks like a perfect solution to me.

I made a patch for the user.module to add salt (the 'created' field) to the password. I suppose the 'created' field never changes for a certain user ?
If people think this patch is a good first step, i might add some extra code to make sure that old (non-salted) passwords still work too.

Carefull: if you implement this patch, 'old' (non-salted) passwords will not work anymore. But you can still log in and change your password via 'request a new password'.

(It's obvious that this patch needs some extra work)

I definitely like this patch. Not sure sure what to do with old passwords though... you'd almost need a database-wide password reset when an update to 5.0 happened (as part of the update process), wouldn't you? Supporting both seems like a con rather than a pro; if we're going to increase Drupal's security it should be across the board, imo.

Created date is a good thing to salt against, I think. a) it doesn't change per user, b) it's not easy to derive, c) it doesn't give any additional hints as to which user the password is for, should it ever be cracked.

Given the need for backwards compatibility, it might be better to add an additional salt column which will be blank for existing accounts. Changing your password will make it salted, and any new accounts will be salted too.

the patch is ripe with code style errors.

Update is easy. Store md5($salt . md5($pass)) and then you can upgrade.

I had a brief chat with kkaefer about how strong double hash is. Warning: this is just my very basic understanding, I am not an experienced cryptographer. Each character of a password you type on a normal keyboard usually has characters from the lower half of the ASCII, and 0-31 are typically not used. So, that's about 6.6 bits, so as long as you use a password which is shorter than 20 characters, you have less than 128 bit strength. As far as I know md5 is created so that it can be thought as a fairly random 128 bit value. Given that, I would say that you are not losing encryption strength over using double hash. And with double hash, you are less vulnerable to a rainbow crack because as far as I know double hash rainbow tables are not (yet) available.

a safe way to do the update would be to create a users_new temporary table and fill it with an INSERT{users_new} SELECT uid, name, MD5(CONCAT(created, pass)), .... FROM {users}. Then check whether users_new and users table has the same amount of rows, drop users and rename users_new to users.

And while you’re at it, you can also check if SELECT COUNT(*) as num FROM {users} u, {users_new} n WHERE n.pass = MD5(CONCAT(n.created, u.pass)) returns the same number.

This patch of system.install will add the column salt to the {users} table the way chx proposed (via a temporary table {users_new}).
I did not add the check kkaefer proposed, because it gave timeouts on big installations if I did it in 1 query (I used 150000 users to test). If you think this check would be usefull (or if other checks should be added) I will make them multi-part.

The new field 'salt' is varchar(8) and is filled with md5(created). The new 'pass' is md5(salt, old_pass).

The patch does not support pgsql yet, and to be honest I do not really know how to add it and how to test it.

This patch will use the newly created field 'salt' of the {users} table. It also resolves some code style errors.
For every user we will define a new 'salt' which is 8 random characters.

Something went wrong with the patch of system.install (its 0 bytes).
This is a new try.

I think these patches contains all the necessary changes.
* Could someone test this on a PostgreSQL installation ? How do the CONCAT, MD5 and SUBSTRING functions behave on PostgreSQL? If necessary I can change the salting-process so it is executed in php and not in the db-layer.
* There is a usability problem too: As soon as user.module is replaced by the new one, It will not be possible to log in with your old password as long as {users} is not converted. If someone wants to do the db upgrade, but forgot previously to logon with uid 1, they can not log in anymore (because the algorithm to check the password has changed). The only way to log in, is requesting a new one-time password and using that to log-in. Is this acceptable ? It might confuse admins.

Pleae have a look at the patches, and give some feedback.
If you notice code style errors, don't hesitate to let me know.

And this is the patch to update the {users} table.
It adds system_update_1015() to system.install

My idea exactly was to note use a separate salt column, use created as it is for salt. No need for md5(created), even. Otherwise things look good but I hope you know that this has little chance of Drupal 5 but it'll be good in D6.

Don't you think that a solution based on a seperate 'salt' column is the cleanest and safest solution ?
For one reason or another, admins might decide to change the created date, without expecting that this would corrupt the passwords. This might lead to
disasters.

I agree that a table change makes it unlikely that this change would ship with drupal 5. If we would use a solution based on the 'created' column (so we do not need to change the table) would that increase the chance of getting into drupal 5 ?

I did the md5(created) to make it impossible to distinct converted passwords and new passwords (new passwords are salted with md5(uniqid(mt_rand(),
true)) )

(due to the implementation of the new release system, I had to chose a version for this issue (default is x.y.z). I chose 5.0-beta1 - one can always dream ;) but feel free to change it)

If someone messes around in the database directly, he should know what he does. If not, he is a dumbass.

@#27 - I don't think it is unreasonable in the least for someone to see the created on timestamp and think they can modify it without invalidating the users login credentials. IMO the only good way to go in this is creating a salt field in the table. The table fields should be indicative of their purpose.

http://groups.drupal.org/node/2564

Er, please excuse my drupal newbieness, but why don't you use the php crypt function? php crypt can use various algorithms and can deal with salts directly.

crypt() has some disadvantages:
- crypt might give different results on different systems (different php versions or different platforms). This would make it hard to transfer a database (with encrypted and salted passwords) to another system.
- under certain conditions, crypt will limit the password to its first 8 characters (while drupal allows passwords of up to 32 characters).
- crypt stores the salt as a part of the password

And as a bonus: because we use only standard stringhandling and md5() we are able to do all the encryption in the db-layer (on the condition that PostgreSQL supports concat, substr and md5 - see #30)

Follow up on Postgre issue; http://groups.drupal.org/node/2564#comment-8416. Thanks to Patrick (awele).

Hello,

All string functions in PostgreSQL are documented in http://www.postgresql.org/docs/8.2/interactive/functions-string.html

* There is no CONCAT in Postgresql. To concatenate strings you use || as in : SELECT 'start' || 'end'
It would be however simple to create a function (procedure) in postgresql called CONCAT that does the concatenation with || so that MySQL syntax works as is. However the SQL99 standard defines || as the concatenation operator, and based on my SQL book this is what Postgresql and Oracle do, where MS SQL uses + instead of || , and where MySQL uses CONCAT
* md5() seems to be there, but I'm not sure beginning with which Postgresql version (7.3 does not seem to have it, 7.4 definitely has it) ; further than that there are contributed modules that add various crypto stuff in Postgresql, such as hash functions.
* SUBSTRING is supported, in both ways: SUBSTRING('a string' FROM 1 FOR 2) and SUBSTRING('a string',1,2)

Patrick.

The system_update number needs to be moved up to the 2000s now too.

Reminder: we just decided (talking to chx, HackFest in Sunnyvale, CA) to use the 'created' field to do the salting. I will code this in a few days.

Hey, I just thought about something. What about drupal.module? This could potentially cause trouble, right? I haven't used it, so I'm not sure how the xmlrpc stuff is done.

Did this ever go anywhere and if so was the created timestamp used as mentioned? I think this sets poor precedence because "created" has little or no relation to the issue in question and unsuspecting user should not need to know that the created field is also used to salt passwords and will only find this out after they go through and manually update them for some reason or another.

This patch looks overly complicated. Why not just store the salt with the hash similar to the unix passwd file (salt,hash)? The code can easily parse this and if the salt is missing, default to the unsalted md5.

@douggreen Good idea, but there should be one addition: once a password is verified using the no-salt method, the salt and the salted hash should be generated with the verified password so the account increases in security.

the password field is a varchar(32). That leaves no extra positions to store the salt (md5 returns a char(32).
If we use some positions of the password field to store the salt, we would need to cut a part from the md5-hash of the password, making the password less secure (especially when people use md5-databases, and certainly when 2 ways of password-hashing are accepted (both salted and non-salted)).

Or do you propose another way to store salt + hash ?

@Jo Wouters Thirty years ago, we would have to seriously consider all ways to maintain the current record length. Fortunately, it's 2007, and we can just add a new column called "salt."

Supporting both is a no, see #14

@chx Supporting both is not a security risk compromise as long as you track whether a password is salted or not. The security risk comes in when you have to try both salted and unsalted versions to authenticate a user. (If you do that, the password is even less secure than a simple, unsalted hash.) I'm also against something we could just update the database with because we will have to use an algorithm like md5($salt . md5($password)) when we could be using an HMAC-based system with the raw password, which is known to be more cryptographically secure. HMAC requires a good private key for maximum security.

If we mark users with password hash versions, we can "upgrade" the password with the first sign-on because we'll have access to the original password then.

It does not require mhash to run HMAC, although it speeds it up. There are PHP implementations of HMAC SHA1 that just require PHP's built-in sha1().

Of course, if salted MD5 is "good enough," then all of this is overkill.

The technical details of implementing this are trivial:

1. Implement a more secure hashing algorithm. The current discussion is leaning towards md5 salts.
2. Implement storage for the new hash. The current discussion is to either create {user}.salt field, or add lengthen the {user}.pass field and store "salt,user"
3. When people log in update the {user}.pass with the new hash.

Looking at #1 above and the comment immediately preceding this one, I don't know that salted is "good enough". And I've seen very little discussion here from security experts. This thread started over password security. And IMHO we shouldn't settle for the easiest next level of security. We should consider the trade-offs, and implement the best security for a reasonable cost.

I'd like to consider something such as this phpass implementation -- it's a 250 line public domain php implementation of a very secure hash. The author is a security expert whom I trust and use.

When I asked him to look at this issue he replied by email. When I asked what's the advantage of phpass over salting md5, he wrote:

phpass uses a high and future-adaptable iteration count that is encoded along with the salt and hash (so compatibility is not broken when an admin increases the iteration count to use for new password hashes).

The above applies to both crypt(3) methods that phpass is willing to use (known in PHP as CRYPT_BLOWFISH and CRYPT_EXT_DES), as well as to the last resort fallback and portable hashing method implemented in phpass itself on top of md5(). The latter may be forced when portable hashes are absolutely required (but it typically has much worse cracking vs. validation performance ratio than native implementations of CRYPT_BLOWFISH and CRYPT_EXT_DES do, which translates into lower iteration counts that an admin can reasonably use and thus into hashes that may be cracked at a higher rate of candidate passwords per second).

The approach of using multiple iterations to purposefully increase the computational cost of testing a password is known as "password stretching", because it increases the effective length of a password: http://en.wikipedia.org/wiki/Key_strengthening

As far as I'm aware, the iteration count was first made variable _and_ encoded along with the salt and hash in early 1990s (1993 or earlier) in BSDI (BSD/OS) - this is what PHP calls CRYPT_EXT_DES now. The next major implementation of this approach (but with purposefully expensive Blowfish key setup instead of DES) that has seen wider use is "bcrypt" or CRYPT_BLOWFISH as PHP calls it, in OpenBSD (also used on Owl by default, and on some other systems).

As you can see, this stuff is in use for many years - yet people keep inventing salted MD5 (at best)... :-(

As it relates to salts, phpass includes code to generate them without too many collisions. It is easier to reuse usernames for salts, but when the same username is seen on multiple Drupal installs this allows for additional attacks.

The execution cost is an extra include and a little extra work on login and password changes. The benefit is that it will greatly enhance our password security.

The coding is pretty trivial. I could roll a patch by tomorrow if there was any change this would be an acceptable solution. I think that adding a period to the front of hashes create with this algorithm will be sufficient to distinguish them from md5 hashes.

That library doesn't look too great to me. It still doesnt handle the salt any special way. It also seems likely to fall back to MD5 in many cases.

Simply concatenating salt onto a password still isn't as secure as using an HMAC-based hash. HMAC is designed from the beginning to combine text (the password) and salt to create a strong, cryptographically sound hash. Almost any hash can be used with HMAC; HMAC is just a standard for combining a hash with salt. One of the most popular choices is HMAC SHA1, which is implementable in PHP without mhash. Even better HMAC SHA-256 hashes are available with mhash. mhash automatically runs in HMAC mode if you specify $key (the salt) in the function call.

Doug - thank you for posting this.

As it relates to storage, I think that the field should be made wider (this would be best), a new field should be introduced (and the old one made obsolete and unused for new hash types), or only hashes with encodings that are short enough (including hash type identifier, iteration count, and salt) should be used. With phpass, complete hash encodings (including all the info I've mentioned) can be of the following lengths:

CRYPT_BLOWFISH - 60 characters - example: $2a$08$Fp.LN8OhodJfL2Jk4oxS/O0SXku6erZSbW6PqWc6YomzzKKlPbJoG

CRYPT_EXT_DES - 20 characters - example: _zzD.ZScOC5D3G6YuGcs

portable - 34 characters - example: $P$BET3nQtN6qTZGGkU7ARbD61gg7C09g/

The last one can be made to fit into 32 characters by replacing the "$P$", which is the hash type identifier, with a single character (e.g., a tilde). This would be a Drupal-specific convention.

If you do use phpass, then my suggestion is that you keep any possible modifications to it to a minimum. It is too easy to introduce security vulnerabilities into this type of code, and the more changes you make, the longer it will take me (or someone else) to do a security audit of the resulting code. Also, it is helpful that the portable hashes be kept compatible with those in the original phpass, maybe with the trivial exception for the hash type identifier ("$P$" vs. "~" or whatever). That way, correctness of the implementation can be validated and the actual hashes can be audited with common tools (e.g., if John the Ripper is ever enhanced with support for portable phpass hashes).

Finally, since this is my first comment on this node, I'm afraid I have to say that I disagree with many (too many) things that have been said in previous comments; to me, even the node title and description look unreasonable (but I'm not sure if it makes sense to go into detail on that). It must be my 10+ years into password security. ;-)

Attached is a proposed patch using phpass. I wasn't sure how big to make {user}.pass column so I made it 255 characters.

My intent here was to create a work-able solution that minimized the risk of messing up the hashing algorithm. I realize that the phpass code is not Drupal coding standards compliant. I'm no sure what our policy is on 3rd party solutions such as this... can we ignore the Drupal coding standards and just use it as-is, or does phpass.inc need to be modified to meet standards?

Having just re-read solar's comments, I have two minor changes: limit the hash size to 60 characters and look for a leading _ as well as a leading $.

David -

I assume that you're referring to phpass by "that library". If so, you've made two comments on it:

1. That it simply concatenates the salt and the password, and that HMAC would be better. To me, HMAC would be misused. We're not verifying the integrity of a message here; we're verifying a password against a hash. While HMAC could be (mis)used to provide a locally-parameterized hash, where the local parameter (HMAC's secret key) would be stored separately from the hashes (in a configuration file rather than in the database?), noone has requested this kind of functionality for Drupal, many people would be confused by it, the added security would be questionable (often the configuration file may be stolen along with the database), it would make it hard to move individual user records between Drupal instances (or merge user databases), and it may be implemented other than with HMAC (I am referring specifically to RFC 2104 here) just as well.

Can you name and briefly describe any specific attack on the "simple concatenation" approach that phpass uses for its portable MD5-based hashes? Please note that the salt length is fixed.

2. That it "seems likely to fall back to MD5 in many cases". I'm afraid that the two of us find this fallback unfortunate for entirely different reasons. ;-) For example, a lot of people are confused by "MD5 collisions" (perhaps by those that have been demonstrated in practice recently?) Those collisions have nothing to do with uses of MD5 for password hashing.

The reason why I find the fallback unfortunate is mentioned in Doug's comment - it's cracking vs. validation performance ratio, which limits the maximum iteration counts that we can reasonably use. Other than that, MD5 is fine and SHA-1 (or whatever) would be no better for this application.

(To be fair, if we compare PHP's sha1() and md5() rather than SHA-1 and MD5, then sha1() would be a little bit better because it's "heavier" and thus we benefit from C vs. PHP code performance more. This difference could be on the order of 20%. The reason why I've picked md5() for phpass is that it's more portable - available even in PHP3. I suggest that we keep things at md5() because the ~20% difference is insignificant compared to the 10x or worse difference between this loop in PHP and native CRYPT_BLOWFISH or CRYPT_EXT_DES code. The advantage is hashes portable to/from more than just Drupal.)

Doug - I've skimmed through 29706_0.patch (out of context) and it looks fine to me. I've noticed that you forgot to replace the 255 to 60 in system_update_6019(), though. Also, I don't know the purpose of user_pass_rehash(), but it looks weird to me - this would need to be reviewed in context.

Of course, some documentation would need to be written - such as on user_hash_strength and it being the base-2 logarithm of the iteration count of a cryptographic primitive.

If this patch gets in, the next steps might be enforcing strong passwords/passphrases and dealing with timing leaks ("is this username valid?")

My second patch fixes the field length... Thanks!

user_pass_rehash() is nothing new in this code. It is used to generate a single-use time-expiring link for users who request a new password. I only updated this to use the phpass hash rather than the md5 hash.

Solar, if an admin changes the hash strength, I hope that all the already encoded hashes still work. Can you confirm this for us?

Doug - I think that you misunderstood - I was referring to the second revision of the patch. In it, you have 60 in one place and 255 in the other.

Thank you for explaining user_pass_rehash(). I don't think phpass is appropriate for this purpose - it might produce characters that require URL-encoding and strings that are unnecessarily too long, and it is unnecessarily slow. Instead, plain md5() may continue to be used, but we need to ensure that the temporary(?) password is extra-strong (system-generated using a cryptographic source of randomness and contains, let's say, 60+ bits).

Doug - of course you're correct that, with phpass, if an admin changes the hash strength, all the already generated hashes still work. That's the purpose of encoding the strength parameter along with the hashes.

"New Proofs for NMAC and HMAC: Security without Collision-Resistance" published in "Advances in Cryptology – CRYPTO ’06" proves that HMAC, despite the addition of salt, retains the psudorandom function (PRF) characteristics of the underlying hash. If a keyed hash is a PRF, it is cryptographically ideal for use in message authentication, including for passwords.

I suggest generating and putting a private key in settings.php during the setup process. This private key can be concatenated with something user-specific (preferably also random) to generate the key for the hash. If the private key is in the database, it is more likely to be compromised at the same time as the passwords hashes. By placing the private key in settings.php, a person must crack the database and settings.php before they begin using dictionary attacks on the passwords. Even then, they wouldn't be able to compare passwords between users.

If we wanted to be ultra-fancy, the system could even send a public key along with the signon form. When the user clicks to sign on, Javascript can perform public-key encryption on the password before sending it. You could even salt the password here to prevent logging of users with identical passwords.

Here's a link to the paper.

David - thank you for the paper reference.

HMAC is great for its intended purpose, but my point is that what we have here is not "message authentication". I see no advantages of possible misuse of HMAC over the trivial algorithm implemented in phpass, and you have not demonstrated otherwise. One of the goals behind phpass was its simplicity and small code size.

As it relates to local parameterization of the hash, I agree with your suggested use of settings.php, but other than that I've already shared my concerns on making this change.

Cryptographically, password authentication is message authentication. When the users sets a password, you get an original message and create a hash to verify the message's authenticity in the future. When the user tries to sign on, you get a message that the user claims is their original message (the password). The way to verify that the message hasn't been modified (read: they have the right password) is to recompute the hash and compare it against the known one.

We all understand the problems with unsalted hashes, so we add salt. We can do this two ways. We can simply integrate salt somewhere into the password string and hope for the best, or we can use HMAC, an algorithm proven to maintain its cryptographic strength. If you agree with the paper I posted, HMAC can even compensate for weaknesses in the underlying hashes. For this reason, HMAC SHA1 is known to be very secure. Because HMAC SHA1 is implementable with a handful of lines a code in PHP 4 and 5 without mhash, it's an ideal choice for Drupal.

phpass requires CRYPT_BLOWFISH or CRYPT_EXT_DES to use anything better than MD5. PHP typically only supports those methods through mcrypt. Most shared hosting providers don't compile PHP with mcrypt. When phpass doesn't have those requirements met, it falls back to basic MD5.

David - I understand how we can use (misuse? reuse?) HMAC here, and I understand your point behind its proven security. My point is that neither the weaknesses in the underlying hashes that HMAC is shown to compensate for nor attacks HMAC is meant to deal with (someone tampering with a known or even chosen message to produce another one that would "pass") are relevant to password hashing.

I agree with you that simply "integrating salt somewhere into the password string and hoping for the best" is risky. In fact, I've given some thought to this before I've opted for the trivial solution in phpass. It is important that the salt length is fixed and that it is processed first (before the variable-length password), although the loop that follows would help even if I did things differently. I am aware that there are attacks on uses of MD5-like iterated hashes resulting from their processing of input data in blocks; I've documented one such attack in my analysis of Cisco's TACACS+ protocol (scroll down to "6. MD5 context leak").

I also agree with you that HMAC-SHA-1 would be easy to implement in PHP 4.3.0+ code, but IMHO the only reason to do so for Drupal's password hashing would be "political" rather than "technical". I mean that it would make people who are concerned that MD5 has been broken happy. Unfortunately, it would not make people who are unfamiliar with HMAC and have heard that SHA-1 has also been broken happy. (In reality, neither break is relevant.)

I think that you're wrong about CRYPT_BLOWFISH or CRYPT_EXT_DES "typically" requiring a PHP build with mcrypt - at least in my experience with multiple versions of PHP this is not true. I've been testing phpass on very basic builds of PHP (even plain "./configure"). These CRYPT_* methods are available when PHP is compiled on a system that supports these hashing methods natively (in libc or libcrypt). For this reason, it is possible that one of these two and not the other is available on a given system (e.g., older *BSD should only have CRYPT_EXT_DES, but not CRYPT_BLOWFISH, whereas SUSE Linux should only have the latter).

Yes, the chance of falling back to the MD5-based hashing method is pretty high (although I expect that it will be decreasing with newer operating systems, with more PHP installs making use of Suhosin, and maybe with parts of Suhosin getting into the official PHP). However, this is not "basic MD5". It is a higher-level hashing method (that happens to be implemented on top of MD5) that has properties similar to those of CRYPT_BLOWFISH and CRYPT_EXT_DES, with the only major difference being its worse cracking vs. validation performance ratio. (I am assuming that a cracking application would use a more optimal implementation.) Those properties are very important in practice (it's password stretching by 11-13+ bits now and more later), unlike whether a fixed-length salt is prepended or processed with HMAC.

My point is that neither the weaknesses in the underlying hashes that HMAC is shown to compensate for nor attacks HMAC is meant to deal with (someone tampering with a known or even chosen message to produce another one that would "pass") are relevant to password hashing.

I think you're giving too much weight to the qualitative "intent" of HMAC and too little to its quantitative properties. But, even in the "intent" case, HMAC is designed to prevent someone from working from a known-valid hash to create a new message that authenticates. HMAC-SHA1 is a pseudo-random one-way hash function, which should -- regardless of the design intent -- provide good password security.

Another thing I'm concerned about with Blowfish and Triple DES is that they're reversible. If someone gets the key to a site, they don't even have to run a cracking utility; they can just decrypt all the passwords. With HMAC-SHA1, if someone gets the secret keys, then the passwords become exactly as cryptographically secure as with a plain hash like SHA1. In practice, they're still much more secure because, while databases exist for plain SHA1 and MD5, a database cannot be pre-built for a given site's HMAC-SHA1 because the key changes all the hashes unpredictably.

HMAC is designed to prevent someone from working from a known-valid hash to create a new message that authenticates.

Yes, but plain MD5 is also designed to achieve that, and it has not been broken in that respect. Also, phpass uses multiple iterations of MD5, with inputs guaranteed to be different at least between the first and second calls into md5().

Sure, HMAC-SHA-1 would provide good password security if augmented with multiple iterations of SHA-1 or whatever for password stretching.

For practical purposes, this difference is unimportant. It is far more important (and unfortunate) that we can't use a system's processing power optimally from PHP code with only this very limited selection of cryptographic primitives. This reduces the amount of password stretching that we can do when neither of the two reasonable CRYPT_* methods is available.

As it relates to your concern regarding block ciphers being reversible, this does not apply to hashes based on block ciphers, and in particular to hashing methods known in PHP as CRYPT_BLOWFISH and CRYPT_EXT_DES. There's no "key to a site"; each user's password is the key. Also, these are not Blowfish and DES (nor Triple DES, for that matter), but rather higher-level algorithms built on top of eksblowfish and modified DES, respectively. You can check out the original paper on bcrypt (CRYPT_BLOWFISH), as well as the web page for my public domain implementation of bcrypt.

The "databases" (rainbow tables) for plain MD5 and SHA-1 are defeated by the use of large enough salts (since a separate set of rainbow tables would need to be built for every salt, making old-fashioned searches for weak passwords more practical); HMAC is not required and provides no further advantage.

The more I read this thread, the more I think there should just be an option during install to use MD5 (for backwards compatibility on upgrades) or SHA-1 (as default on a new install) since they're the only two language functions that can be universally depended on. PHP4 is going to EOL in December, so I think it's reasonable to at least expect people to have PHP 4.3 to use Drupal 6.

Salting in settings.php is silly since if the DB server is compromised, you should probably assume your settings.php file has been, too.

I think it would be nice to be able to crypt() hash my passwords with the CRYPT_BLOWFISH option as opposed to MD5, but I have also gone through unplanned and forced Redhat Apache to Win2k3 IIS conversions and would hate to think of the headache of what would happen to all the user accounts then (even if it were different OSs). The crypt() options I think should be relegated to a contrib module. Then it could also use mhash for even fancier options.

More thoughts... I didn't address the salting against created date. The double hashing seems like it could be a lot of additional CPU overhead. Again, I'm thinking along the lines of legacy, so MD5 would remain the same for upgraded sites, but SHA-1 on a new install could have just a single hash execution with the creation date hash. Then there could be a contrib way for people to hack in the SHA-1 salt upgrade if they really wanted.

Option to continue to use plain MD5 for backwards compatibility (ability to revert to old version?) - makes sense.

"Use SHA-1" - doesn't make sense to me. There's very little difference between MD5 and SHA-1 for this application. It's the way either MD5 or SHA-1 is used that really matters. OK, we can re-work phpass to use SHA-1 instead of MD5 if that makes religious people happy (and if we have to make them happy). The disadvantage is hashes incompatible with some potential non-Drupal applications that may make use of phpass as well.

I don't think the crypt() options should be in their own module. I'd rather see an option to turn them on (best security) or off (portability of hashes). The code to implement their support is small.

"Fancier options", using mhash or otherwise - NO! This is about security and compatibility, not about breaking compatibility for fun. If someone wants to deliberately break compatibility for security, then local parameterization (already discussed) would be the way to go.

On "double hashing" and "CPU overhead" - unfortunately, password stretching is all about CPU overhead. We can't have strong hashes (against traditional off-line password cracking) that are fast to compute. Either our hashes are strong or they're fast, not both. So the strength (processing cost) should be configurable, and it is with phpass. (Of course, it is possible to have strong hashes that are fast if the passwords are extra-strong, but at this point we're not even enforcing strong passwords/passphrases, and when we do, we wouldn't want to enforce 6+ word passphrases when we could do 4+ word ones with proper stretching.)

Given that what we have isn't very good, can we agree on something, so that we'll have a chance of getting this into 6.x?

I like the proposed salting (or whatever) in settings.php because one key problem we're trying to solve here is what happens when the database is downloaded to another server (either for backup or testing). If a critical component is stored in the settings.php, then the passwords in the offsite database are more secure.

Doug - I suggest that you try to get your existing patch in first (with minimal changes).

Local parameterization in settings.php may be added later - it may be an entire separate topic to discuss, with its own issues, so I'd rather not get into it now. Yes, it will break compatibility (when used), but that's its purpose, so it's fine.

subscribe

I'm still not convinced phpass offers anything over HMAC-SHA1 in security or compatiblity. I'd like the commenters on this issue to stop assuming that phpass is some sort of null thesis that I have to defeat. Including phpass will add a bunch more code to core versus HMAC-SHA1. If anything, I think the burden is on phpass supporters to show that all the additional code is justified. Where else in Drupal core do we include an external library, even a public-domain one?

David - until now it was not clear to me that you're proposing to use raw HMAC-SHA-1, without any password stretching. The need for password stretching is understood since 1978 or earlier (scroll down to "Slower Encryption"). All password hashing methods currently in use on Unix-like systems use multiple iterations of a cryptographic primitive - starting with 25 for the traditional crypt(3) (which I do not recommend, and which phpass doesn't use), to 1000 for the FreeBSD-style MD5-based hashes, and to variable iteration counts with the two methods that phpass is willing to use (when available). The portable MD5-based hashes implemented in phpass also use multiple iterations (2049 or 8193 by default on PHP 3/4 or PHP 5+, respectively, but an admin is free to change that without breaking compatibility).

@solardiz I haven't said anything about password stretching, for or against.

David - oh, I made the (incorrect) conclusion that you're against password stretching from your comment on code size. If we implement HMAC-SHA-1, add password stretching to it (with a check for PHP 5's support for second parameter to sha1() for optimal performance), add generation of random salts, add decoding/encoding of {iteration count, salt[, hash]} strings, the code size will be comparable to that of phpass. The only savings would be from not supporting CRYPT_*. Further source code size reduction is possible with more compact coding style, simpler encoding (not a base-64 one), no comments, no error checking, not-so-random salts (bad).

@solardiz Don't we have to generate and track the random salts even with phpass?

David - we do, and this functionality is a part of the existing source code for phpass (maybe you did not realize that?) I was trying to say that most things responsible for code size are the same with both approaches. In fact, we can integrate HMAC-SHA-1 into phpass (if that would be required for inclusion, although I see no technical reason for the change), which won't affect its code size much.

How is phpass handling salt right now? The member function for encoding a password seems to only accept a password.

phpass hides the "complexity" of salts from the rest of the application. HashPassword() generates a random salt and encodes it along with the returned hash. CheckPassword() checks a password against the hash encoding (that includes the salt).

It's good to hear that phpass manages the salt for you. At this point, I'm convinced that phpass represents an acceptably secure password backend. Of course, phpass still has the external code issues. We're not currently letting the TinyMCE module include the main TinyMCE code.

Attached is a new patch that:

• is rewritten to current state of Drupal HEAD with schema API
• uses md5 for the temporary password resets (because of Solar's comments above)

I have the remaining questions:

1. Will Drupal core accept phpass.inc (a 3rd party 250 line library) as-is, the precedence being jquery?
2. Should we make the user_hash_strength settings variable more user friendly, for example, strong, medium, and weak instead of 8-31?
3. Should we limit the hash strength to something we've performance tested to work on most systems or simply provide a good error message? 31 bit hash strength takes too long on my system and results in a php timeout.

An error can occur during the upgrade process if admin is not logged in when they update the code. We update the {user} pass hash when a user logs in, and we need admin o login to run update.php to increase the hash size. This catch-22 does really cause problems. But if the user tries to login before update.php is run, they will get the error:

user warning: Data too long for column 'pass' at row 1 query: UPDATE users SET pass = '$P$BxPqxm3n1899yhHzjsEcUInPj/U7xS/', data = 'a:0:{}' WHERE uid = 1 in C:\wamp\data\apache\htdocs\doug6\includes\database.mysql.inc on line 163.

By the way, I don't want anyone to get the mistaken impression that I think the origin of phpass as an external library is, alone, a reason to block this patch. While I like Drupal-style code, I also like marking issues "fixed." :-)

This looks very good to me from a coding/security point of view. Right now, anyone who comes across a mysql dump (for example

I don't see and problems with Drupal taking the code from a third party library here (if it gets Drupal-ified - I think accepting 'as is', without this is highly unlikely), since this is really only a small piece of code, and pretty unlikely to change. Any minor upstream changes would be super easy to merge.

I do see a few things that should IMO be resolved before this is committed:
- Code style should be fixed (new line '{'s, '#' comments, CamelCaps etc)
- Function names should be properly namespaced, rather than using a class. We may want to consider another base name, since phpass is not very self-explanatory - perhaps passhash?
- I am pretty sure that we would need this relicensed as GPL (with optional public domain, of course). Also, we do not give credit in file headers, only in CVS logs and MAINTAINERS.txt (is someone wants to be the maintainer of this component).

This looks very good to me from a coding/security point of view.

I don't see and problems with Drupal taking the code from a third party library here (if it gets Drupal-ified - I think accepting 'as is', without this is highly unlikely), since this is really only a small piece of code, and pretty unlikely to change. Any minor upstream changes would be super easy to merge.

I do see a few things that should IMO be resolved before this is committed:
- Code style should be fixed (new line '{'s, '#' comments, CamelCaps etc)
- Function names should be properly namespaced, rather than using a class. We may want to consider another base name, since phpass is not very self-explanatory - perhaps passhash?
- I am pretty sure that we would need this relicensed as GPL (with optional public domain, of course). Also, we do not give credit in file headers, only in CVS logs and MAINTAINERS.txt (is someone wants to be the maintainer of this component).

The temporary password resets will likely need more work (later) to make them secure. This should not prevent this patch from being committed first.

Non-numeric values for user_hash_strength (such as "medium") either would become outdated after some years or the numeric values associated with them would need to change. I think that neither is good. So I'd leave user_hash_strength numeric, but document it in more detail. Currently, the reasonable range is roughly 4 to 12, and the default is 8.

Yes, it makes sense to introduce an upper boundary on user_hash_strength - for example, at 14 (and increase it after some years).
For values below 4 or above 14, print an error message and not attempt to do anything. Maybe allow the special value of 0 to mean the default.

Right now, the default of 8 is hard-coded into several places of the patch. I suggest that it be replaced with 0 in most places, and 8 only left in the PasswordHash() constructor function (it is already in there for values that are out of range, like 0 is) or in its replacement (if the code needs to be re-worked).

The $portable_hashes parameter is now also hard-coded as TRUE. I suggest that it be made configurable (and maybe the default should be changed to FALSE such that we can have stronger hashes on some systems, but I'm not the one to make this determination).

The updates from plain MD5 to new hashes upon login should probably be made optional or at least skipped until the field is made sufficiently long. I'm not familiar with this code to suggest a specific way to accomplish the latter.

I see no problem with coding style changes and renames, if those are needed. I think that having the code GPL'ed and public domain at the same time is best achieved by having Drupal as a whole GPL'ed, but this source file carrying a public domain notice. Trying to describe both things for one source file specifically is trickier because code should be licensed by its copyright holders, whereas public domain code is not copyrighted.

The best solution to hard-coding is to use variable_get() with the default value being the formerly hard-coded value. Implementing variable_set() for the variable is not always necessary.

Doug's patch does use variable_get() - but it does so in multiple places, which results in the default value being hard-coded in all such calls. This is why I suggested to let lower-level code pick the default value instead.

@solar, The variable_get with hard coded defaults is pretty much the Drupal way. However, patch #145164 will cleanup the variable defaults and put them in one place. Not that you need to read more about it, but I think this thread from the devel list was the genesis of the idea.

To Everyone following this thread, I hope to post a new patch within the next 24 - 48 hours that refactors phpass, after which time the code will be ready for review again. Grugnog2 and I worked on it last night and I'm awaiting review and approval from Solar before posting.

You could always just create a constant for default password strength - no need for a separate function. But it's not a big deal either way imo.

Here's an updated patch that hopefully makes the inclusion of the phpass library more Drupalish. With Solar's permission, the library has been renamed (passhash) and refactored with Drupal coding standards.

• This version has three hash settings that may be confusing to the user, and that we might want to drop
• Fixes it so that if the admin switches from passhash back to md5, that the hashes get recreated on user login, identically to how this worked on the switch from md5 to passhash
• Changes the default hash to non-portable, which Solar says is more secure. We should discuss this.

@douggreen I'd rename the .inc file to make it consistent with the passhash name.

@David, Good catch! Rerolled with the correct include file name...

+    '#options' => array('passhash' => t('secure'), 'md5' => t('original')),
+    '#description' => t('Select the method to hash the user passwords.  Use "secure" unless you really have a good reason.'),

I really don't like this 'dumbed-down' wording... Let's add some more detail here, either in the description or in the radio options themselves (Secure - Blowfish [name dynamically generated from the hashing algorithm that will be used]), to explain the difference between "Secure" and "Original" (those options should be capitalized, as well, btw).

Also, this is going to sound very weird coming from the Coding Standards Queen, but if we're going to implement a third-party library, I would actually prefer us *NOT* to fork it in any way, even for 'minor' changes such as coding standards conformance. This is especially critical for security-related add-ons, where any vulnerabilities that might be found in the upstream code will need to be addressed immediately. It's much easier to drop in the newest foo.inc file than it is to drop in the latest foo.inc file and go through the process of manually re-formatting it, risking introducing bugs.

Obviously, Dries has the definitive word here, but there's a precedent in the old xmlrps.inc PEAR library in Drupal 4.4 and earlier: http://cvs.drupal.org/viewcvs/drupal/drupal/includes/xmlrpcs.inc?rev=1.9...

@webchick

As Doug mentioned, the settings are really just there we assist reviewers, so that they can test out the different possible behaviors, and make sure things stay working and sane, and that we have the best default settings. I think we should drop all (or maybe all but one) from the final patch. We should leave the variable_get()s in, however so that a contrib module could expose these settings if it wanted.

In terms of adding 3rd party libraries, I don't think XML-RPC is the best example, since that let to the most extensive security breaks in Drupals history :)
If Dries prefers then it is no problem to use the 3rd party library directly - however, in terms of code reuse and integration I think it is well using the Drupalized code. Apart from being much easier to read for Drupal coders, it allows us to insert variable_gets, and other tweaks to make this more configurable.

I don't think the security issue you mention is a blocker, both because the original author is available here, and would likely be available to help patch in the event of a security vulnerability. Also, the volume of the code here is really quite small, and I suspect that if there were any security fixes needed they would likely normally only amount to a line or two.

Another precedent for Drupalizing a 3rd party library for core is the Packer code - which recently got committed as part of the JS aggregation patch.

I've hacked the test.php script from phpass to work with passhash.inc from 29706_3.patch and ran it with two different builds of PHP (one is PHP3, the other is PHP5), testing all three hash types (since PHP3 was not aware of CRYPT_BLOWFISH). All tests have passed. :-) Sorry, I did not update test.php for Drupal coding style - someone else may do it if desired.

What are the next steps? Can we state that the patch is ready to be committed or are there specific issues to be fixed pre-commit?

Since noone has pointed out a specific issue to be fixed pre-commit, let's state that the patch is ready to be committed.

The patch no longer applies correctly to Drupal head. I'll re-roll, test, then restore to RTBC.

I re-rolled to the current Drupal head which was necessary because of some re-organization that I think occurred when open-id was committed. I tested again, to make sure setting that changing the settings between secure and original worked properly. I also added some jquery to show/hide the settings form values that aren't available in the "original" setting.

Can someone review this again, so it can be put back to RTBC?

Doug, you seem to have lost the definition of _passhash_gensalt_blowfish(). Other than that, the patch looks OK to me - but I am not familiar with Drupal specifics.

Sorry for joining this issue that late. I'm (co-)maintaining the gallery.module to integrate Gallery2 into Drupal and a user just pointed me to this issue ...

I havent read the whole issue yet (didnt have time for it), but there are some points I cant read from the code and the inline docs:

1. There seem to be no way to recover the md5 hash from the database once the more secure method was used. And (more important) it also seems impossible to generate the secure hash from an existing md5 hash. Thats pretty bad for all modules integrating external php applications into Drupal as most of these are still using md5-based hashes or at least (as G2) a combined hash 'md5.secure'. But without that I cant see how a proper user synchronization will be possible.

2. What is a 'Portable Hash'? If it binds the hash to the server (how?) will it be impossible to migrate to a different server later? How about backups? Or working with a copy on a different machine for testing/development/improvement purposes?

3. All in all there are almost no inline docs on the inner workings or the hash format in the patch. Better docs are heavily needed in this context.

It would be great if someone could improve #3 and write a summary on what the patch changes to be added to the handbook ones this goes in. I'm also not sure this is a 'bug'. As it introduces heavy changes to the way passwords are handled it is more a 'feature' or should at least be handled as such.

Didnt meant to change the status ;)

@profix898 - if you do choose to read past comments, I suggest that you start reading with comment #44 and on. Earlier comments didn't have much effect on the current patch and even the original problem description was not entirely correct.

To address your points:

1a. Yes, no way to recover the MD5 hash, by design. There must not be such a way, or it would defeat the purpose of using the more secure hashes.

1b. Yes, also no way "to generate the secure hash from an existing MD5 hash". Frankly, this hasn't been considered. Why would you need it? Even with this patch, Drupal can still authenticate users for whom plain MD5 hashes are stored (if "secure" hashes are enabled, those MD5 hashes will be replaced with "secure" ones upon login). What's "md5.secure" and how is it any more compatible with Drupal's old way of doing things?

2. "Portable Hashes" are guaranteed to be portable across systems. Other "secure" hashes do not offer such a guarantee, but are nevertheless likely to work on other systems anyway (depending on their support in a given OS and/or PHP build).
This is not a "binding" feature ("local parameterization" as discussed in earlier comments); we've decided against implementing one at this point (it can be discussed and implemented separately, after this patch gets committed).

3. I'm not sure if more comments on the inner workings are really desired. There are some comments already and the rest might be best expressed with the code itself. However, I agree with you that documentation is needed on practical aspects of the changes. For example, my answer on "Portable Hashes" above can be a part of such documentation.

I am also not sure if Drupal's use of plain MD5 for password hashing was strictly a "bug", but for a mature application it is very close to being a bug.

@solardiz: Thanks for your quick response.

"Even with this patch, Drupal can still authenticate users for whom plain MD5 hashes are stored"
I noticed several 'magic' strings (e.g. $, _, $P$, H*, *0, *1, ...). What are they for? If I got that right the 'secure' hashes are stored in a special format and in case a simple md5 hash is stored in the db the passwords (on login) will be authenticated correctly even if 'secure' method is selected. Right? In that case it shouldnt be a problem that you cant calculate the 'secure' hash from an existing md5 hash. Otherwise it would definitely be a required feature for importing users from external apps.
As it is impossible by design to recover the md5 hash from db we will loose the ability to batch export users to external/integrated apps. Thats something I dont like much (and it is often requested by users), but I guess we have to pay that price for better security ;)

For 3) Documentation is mainly required on practical aspects of the changes, yes. I think that 2-3 sentences should be added to the Drupal help module for the password settings page. But for us integration developers (and other curious developers) there should also be a handbook page explaining the 'inner workings' (incl. the 'magic' strings). Its not required as inline docs, but it should be available somewhere. (You cant even get that information from the issue, right!?)

That's odd - re-rolled again, this time with the missing function _passhash_gensalt_blowfish from passhash.inc.

Sorry, I forgot to change the status.

Er. Fixed? I think you meant this one. :)

@profix898 - you've got it right.

Some of the "magic" strings you mention are illustrated on comment #46 - those identify hash types. The rest are internal to the code: "H*" tells PHP's pack() function what to do (so it should be in PHP documentation), "*0" and "*1" are just strings returned by _passhash_crypt_private() on error - there are two of them in order to guarantee that the returned string is different from the stored hash string (so authentication will fail) no matter how and why things went wrong.

@douggreen - the patch looks good to me. Its passhash.inc is exactly the same as I had tested with test.php.

I'm going to postpone this to Drupal 7.

I'm not sure this helps anybody, or if anybody would consider putting this into Drupal, but I have recently been migrating from a site which uses "htpasswd" style SHA hashs,

My current solution in user.module is to put the following code into the user_load function:

      $query[] = "( pass = '%s' OR pass = '%s' )";
      $params[] = md5($value);
      $params[] = '{SHA}'.base64_encode(sha1($value,true));

The obvious plan being to maintain "normal" drupal behavior, but to also allow SHA encoded passwords.

Reading through this thread, I would guess that there would be 100 reasons why this idea would be rejected for the core of drupal, but I figure I wont be the only person with 100's of SHA encoded user passwords...

(It would actually make my user interface slightly simpler if I make SHA passwords a default, because I can then use those to set up SVN access files in my SVN integration.... but this would be a vote for SHA passwords, probably postponed till Drupal 7 if I understand right?)

Given the recent wordpress scandal, let's try again. Simplicity is our keyword. No new function, no new database columns.

Scandal-shmandal. We're in freeze and this is not a bug, and is not critical if it was. I really think this issue needs more discussion rather than throw this into the last beta. I don't think MD5 hashing as it is now is a *bad* implementation, it just needs some enhancement.

Since Dries had bumped this to a Drupal version where PHP5 can be used, I thought perhaps the PHP hash extension http://www.php.net/manual/en/function.hash-algos.php could be a legitimate possibility for implementing a more modern algorithm.

Some other CMS such as joomla already have salted hashes password:

Joomla! 1.0.13 now features salted hashes which will automatically pad a password string with 16 randomly generated characters to make the hash exponentially more difficult to reverse-engineer or guess. As users login to your Joomla! powered website, their passwords will be automatically converted from the old password storage system, to the new system.

http://www.joomla.org/content/view/3677/1/

Let Dries decide again, okay? I am acting after a discussion on the security list. Yes we are in freeze and there is no DB schema change and there is no API change. If you poke the DB directly, face the consequences -- which are rather minor, just how many modules deal with login directly instead of calling an API?

@deekayen: MD5-hashing without salt is absolutely not safe. I did a quick check with the users-table of a Drupal-site (32 users, all Drupal-developers!!) and found 7 passwords via this tool http://passcracking.com/index.php These passwords could just as good have been plain text in the database.

@chx:
I just had a quick look at your patch, and I like this solution (keeping "Simplicity is our keyword" in mind).
2 remarks:
- the update might give a timeout on big installations; which would make the table corrupt (there is no way to know which hashes are salted or not). We might need to do the update in chunks.
- shouldn't we add {} around UPDATE users in user_update_1() ?

(Oops, looks like my post crossed yours, so I updated some fields by accident)

I agree with having the option of choosing the hash algorithm.
or even allow users to write their own code.. like user can write
sha1(md5($pass[0]).md5($pass));

braces yes, timeout no, query time does not count into php timeout. If this works, RTBC , so Dries can review. Also, MD5 hashin' without a salt is utterly unsafe -- I just read today that one funny bloke googled (!) a hash and got the solution.

@mgccl: now, that's Drupal 7.

+function user_update_1() {
+  $ret = array();
+  // pgsql only has a two op concat.
+  $ret[] = update_sql('UPDATE users SET pass = MD5(CONCAT(CONCAT(pass, init), created))');
+  return $ret;
+}

Either I did not have enough tea, or the above is wrong.

The pass is already md5'd in the database once.

In your code, we are doing md5 after the md5(concat(concat(...)) thing. Would the results be the same?

if your password is insecure your account was created at 111 seconds and your initial email is initial@example.com then we calculate md5(concat(md5(insecure), inital@example.com 111)) Why that? because we can't up otherwise.

$pass = 'insecure'; $created = time(); $init = 'init@example.com';
echo md5($pass); echo "\n";
echo md5($pass).$init.$created; echo "\n";
echo md5(md5($pass).$init.$created); echo "\n";

c7a6002549b0ff54324ecce62cd9ab6d
c7a6002549b0ff54324ecce62cd9ab6dinit@example.com1195760359
3acce27384c4d9f7b91092376cb0141c

We have c7a6002549b0ff54324ecce62cd9ab6d and init@example.com and 1195760359 in the database at update time so we can calculate 3acce27384c4d9f7b91092376cb0141c. We do not know the original password and we do not need it.

chx, have you tried your patch on a fresh install?
It doesn't work for me.
1. if I apply the patch and install drupal, I can't login with my password after the installation. The 'pass' in the db after the install is the same with or without the patch. I have to cvs up -C in order to being able to login with the password I entered during the install.
2. if I apply the patch after installing drupal, and I change my password to insecure, pass or is always c7a6002549b0ff54324ecce62cd9ab6d with or without the patch.

subscribe

I thought Dries decided in #107, and I know I'm protesting this without a patch or a better idea and I do agree the current proposed solution is harder to bypass with rainbow tables.

Hashing a hash seems computationally redundant in the frame of optimization (yes, I do realize it is a clear upgrade path). That's a one time event when compared to the inevitable thousands of repeat logins and account creations Drupal sites will have for hash calculations.

A 6.x patch for this seems forced and out of place to me. I think by taking some time on this [important] issue, we could come up with a way to extend password hashing so it could be hooked or display options for hashing to user 1 or during install. Going down the path of hashing a hash with two concats seems un-extendable.

we could come up with a way to extend password hashing so it could be hooked or display options for hashing to user 1 or during install. Going down the path of hashing a hash with two concats seems un-extendable.

While it is not my intend to hurt those who worked hard on this, the reason there's no password salting in D6 is this freaking huge "featuritis" issue.

Some limitations of hash salting:

1 - It does NOT protect your site when an attacker has realtime read access to the database (via SQL injection for example), as the session ID is stored in the database.
The time window for an attack is no longer unlimited though.
2 - It does NOT protect the password of active users (still logging in). This derives from 1).

No, the reason there is no solution for this issue in D6 is the sheer number of reasonable solutions. We haven't even in all this thread mentioned http://drupal.org/project/aes (unless I forgot about it) which to me says there is demand for other options for password storage. Of course, I don't have access to the stats on how popular that module actually is, but I think it serves as evidence that a changeable/extendable password system would be nice.

#110 seems like it would be even harder to reverse than the current plain MD5. Rainbow tables are only going to get bigger anyway, so it's a matter of time before password DBs can be extended and another change is needed - which circles back to password extension.

Wouldn't it be better to just increase the column size of the password column and then use rfc2307 style passwords (ie, preceding the hash with {MD5} or {SMD5} for salted md5) in the db? That way, the upgrade could just prepend {MD5} to all current user passwords, and then when the user logs in, user.module could create a salt and regenerate the password for the user. This will also be more compatible in the future for better/stronger hashing algorithms.

Granted, if you have a bunch of old user accounts lying around, their passwords will never get salted...

@chx - thank you for revitalizing this discussion. I had a look at your patch from comment #117. While I understand the advantage of having a minimal number of changes to the existing code, unfortunately, your proposed patch fails my security review. A sufficient reason for that conclusion is that the patch does not implement password stretching. These days (in fact, starting with 1970s) a lack of password stretching is just as inappropriate as a lack of salts. You might want to re-read comments #44, #70, maybe others.

Thus, my proposal remains to use the patch from comment #103.

If you'd like to have the ability to "upgrade" existing plain MD5 hashes to the new salted and iterated ones (rather than merely have both old and new hashes supported simultaneously, as it is done in the existing patch), this ability can be trivially introduced.

Are Windows users crippled by #103 (I don't have a Windows machine around to try it out)? I saw a reference to /dev/urandom in the patch and I don't remember having CRYPT_BLOWFISH available when I ran WinXP. What are the repercussions to #103 when a user switches from a CRYPT_BLOWFISH supported server to one where only the degraded MD5 option is supported?

I am still +1 for the patch in #103.

#103 indeed has a schema change to increase the length of the password field, but I think this change is much less significant than updating the value of every password field, which is done by #103 and #117 (although #103 currently does it on login, rather than en-mass).

Password stretching is clearly as important as salting (and this is not a new thing, as Solar notes), and I think that we would be best off waiting and fixing this for good, with a thoroughly thought out and reviewed patch. Password backend code is definitely not the kind of thing we want be be changing again and again.

If there is enough strength of feeling that we REALLY want to fix this for Drupal 6, then I think we should have enough energy to fully review/tweak and get a variant of #103 in, and test it thoroughly. If there is not this strength of feeling, then I think our best bet is to add a note to the docs reminding people to keep their database dumps secure (and to check contrib db_query calls of course) until we fix this in Drupal 7. I would prefer the latter to us committing something that does not include password stretching to Drupal 6 (and then likely having to fix in in Drupal 7).

@deekayen - no, Windows users are not crippled by #103. Windows is actually not any different (in this respect) from some Unix-like systems that lack /dev/urandom and lack bcrypt support in their libc/libcrypt.

The patch uses /dev/urandom when available, but it does not depend on /dev/urandom - there's a transparent fallback.

As to CRYPT_BLOWFISH, on Windows it may currently only be present if PHP is built with the Suhosin patch. But things may change to the better, e.g. if bcrypt is introduced into Cygwin or into official PHP. Similarly to /dev/urandom, the patch uses CRYPT_BLOWFISH when available, but it does not depend on CRYPT_BLOWFISH - there are fallbacks.

When a user switches from a server that has CRYPT_BLOWFISH to one that does not, either everything continues to work as usual (if the "Portable Hashes" setting was enabled) or old passwords stop to work (otherwise). In the latter case, the solution or workaround (depending on whether you like doing it or not) may be to build PHP with the Suhosin patch, which guarantees that CRYPT_BLOWFISH is available regardless of its support by the underlying OS. In general, it is preferable to anticipate the need for such migrations in advance and enable "Portable Hashes" if so.

I think Dries has already spoken (asking that we wait until 7.x). But if there is movement to get this in 6.x I'd be happy to rewrite the patch from #103 to the current Drupal head, but I'd like a nod from Heine and/or Dries that this is a direction they'd like to go. I applaud chx's effort, however I think that his patch won't solve anything. And IMO if that's all that we're going to do, I'd prefer to do nothing.

Quite a bit of time and energy went into a pretty good solution above. I know that there's a database change. I know that it doesn't rely on a php library to do the security, and it's kinda non-standard. But I think it's a pretty good solution. David and Solar (and to some extend Grugnog and I) had a pretty long discussion about this. Solar, while not well known in the Drupal community, is a well known and respected security expert. See The 59 Top Influencers in IT Security.

Then 7 it is. Store your backups in a secure place.

So there are two possible solutions:

• crypt('password', '$1$abcdefghi'); // 12 char salt starting with $1$ forces use of CRYPT_MD5, which should be available on all systems (?)
• or use phpass.

Wordpress went with phpass, but it's not clear why that choice was made.

I see a single solution with security improvements (i.e. password expiration).

Add a password column to store a password type and/or plugin name. Expire all the md5 passwords. Depending on which password plugin the admin has activated determines what hash is stored. Existing users can login and have their passwords translated to the new hash version as each user logs in transparently or force a password change. Optionally, old users have their passwords deleted during plugin installation and users with blank passwords have to re-authenticate by email.

I'd much rather have a password translation option when the next password hashing option is found to have weaknesses so I could switch from SHA256 to RIPEMD160 to TIGER160 to crypt() or whatever. Then these arguments about which hash is best with which salting method can be resolved by making a different password plugin.

@hickory - no, CRYPT_MD5 is not available on all systems (just on most), and its security is slightly worse than that of the portable hashes implemented in phpass (no variable iteration counts and the fixed number of iterations is a bit low for current CPUs). Besides, with CRYPT_MD5 you need code to generate uniformly distributed random salts anyway - similar to the code found in phpass.

I think that this comment by ryan summarizes why WordPress have chosen phpass pretty well (others in that thread seemed to agree):

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

@deekayen - With the proposed patch, we already have the hash type prefix - it's "$P$", etc. Unix systems are successfully using this approach for system login passwords, and they don't need a new per-user field for storage of "a password type and/or plugin name". As to conversion of existing password hashes to the new type upon login, the proposed patch already does that.

When you say "when the next password hashing option is found to have weaknesses", you seem to imply that some weakness was "found" in MD5, which prompted this discussion. While this may be partially true, there's no newly discovered weakness of MD5 that resulted in the need for replacing raw MD5 with something better in Drupal. Raw MD5 was never suitable for password hashing, and it was understood 10 years ago just like it is now (just not by typical PHP programmers). None of the cryptographic primitives you list (except for crypt(), which is ambiguous and not a primitive) are suitable for password hashing in their raw form, and all are when properly "cooked" (although there are subtle differences between them).

I don't think there's a need to make introduction of new password hashing methods very easy - to be done by end-users. Most end-users do not possess the knowledge and skills to implement password hashing in a better way, so they will end up doing it worse. As to having an internal API for us to make use of in future versions of Drupal, we kind of already have it with the proposed patch. I don't see a need for anything else.

I'm not arguing phpass is a bad or flawed solution, or that crypt sucks, or that #110 is bad. My point is that because they are all decent solutions, and at a few people like the AES module or rcmail module, and a thread or two or three (all different) discuss alternate password storage. Steven commented about how the password storage changed in the past from mysql crypt to md5.

I just happened to have a visionary kind of moment with this bug to have it plugable since I don't believe any of the proposed solutions is *the* solution. Again, I'll don't know when I'd ever get around to making a patch of my own, so I understand I might as well be ignored.

Since this feature has been pushed to 7.x anyway, it probably makes sense to think of the best long-term solution, which would indeed be a default core implementation of a pluggable authentication mechanism system. Then it starts to matter a lot less which one we actually end up including in core (it could even be md5... kidding ;)).

I just became aware that phpBB3 went with phpass ([1] [2]). They are using a cut-down version of phpass supporting the portable hashes only. Although they have changed the hash type identifier string from "$P$" to "$H$", the hashes are otherwise compatible with those of genuine phpass (so it is trivial to import/export or share them with other applications that use phpass).

It is curious that they've made this change in one of the last Release Candidates leading to the 3.0.0 release.

I created a 5.x contrib module, that implements phpass for storing password hashes. Consider this a proof-of-concept module.

While I was able to do this in a contrib module, I was only able to do this by using form_alter. It would be nicer to add a hook where user_authenticate is called, and another hook where the password is saved; so that this type of override is cleaner.

See http://drupal.org/project/phpass.

subscribe

It is extremely convenient for import/export that we can insert/edit passwords with a pure SQL statement like pass=MD5('foo'). it is also convenient that php has the same md5() function. it would be great to increase security and keep this behavior.

The phpass implementation will still reads these md5 passwords. It simply converts them to more secure ones when the user logs in. I'm now running the phpass module on several live sites. I suggest that you try it out on a test site to see how it works.

The salt module takes a simpler approach to implementing this, not that simpler is better in the case of password hashing. And it does not at the moment preserve the existing passwords.
http://drupal.org/project/salt

I've made some suggestions there about better nonces and backwards compatibility:
http://drupal.org/node/199358

But I like the idea of using phpass. This article doesn't seem to have been linked here yet, but it's a pretty definitive analysis:
http://www.matasano.com/log/958/enough-with-the-rainbow-tables-what-you-...

Nice article :)

+1 on everyone reading the above linked article before posting further comments on this issue!

After reading through this issue again and thinking about it, I think that password stretching should NOT be enabled in the default install of Drupal.

I want to make it clear that I'm NOT saying password stretching doesn't offer any additional security. Obviously it is a good precaution for a security-conscious application. However, I worry about the performance impact that would happen for large Drupal sites. If password stretching was implemented so that it took 1 second to calculate a hash, I could see the performance impact on sites like drupal.org being pretty bad. Has anyone tried using the phpass module with a high-traffic site to see the performance impact? Maybe I'm overestimating it.

I like the idea of just using salts for now (especially since phpass is now a drupal module, so anyone who wants that extra security can easily get it). chx's patch in comment #117 looks great to me, only the salt wasn't random, and could be guessable at some sites. I took his patch and modified it to work with the latest HEAD, and added a salt column (where the salt is made up of created, init, and a random number from mt_rand()). The salt itself is a md5hash that is appended to the currently hashed password, which makes the size of the rainbow table required to lookup values incredibly huge. It uses the same method chx's excellent patch does, which means that existing passwords will be updated to use the salt off-the-bat.

Also, there was talk about implementing some sort of API to update the hashing algorithm from md5 to something newer. Should we go that route instead? If so, I would suggest that we implement passwords using rfc2307 style (ie, "{MD5}" for storing a md5hash, "{SSHA1}" for a salted sha1). That way its immediately obvious which hashing algorithm needs to be used, and older hashes can be backwards compatible.

These are all merely suggestions; I haven't contributed to the drupal project itself yet and I don't want to step on anyone's toes :)

This is critical, btw. D7 will probably be supported until 2010. By then, I'm sure we'll have rainbow tables up to some stupid amount.

So, if we are salting with the MD5 of anything that's equally strong to adding a bit more than three of [a-z0-9]. Ie. if someone has a rainbow table now for six of [a-z0-9] which I believe exists then in the same time they generate [a-z0-9]{9} they can generate [a-z0-9]{6}[a-f0-9]{16} too. That's not a future proof method. Instead of MD5, make a nice LONG salt of [a-z0-9] characters, let's say 32 of them. That will give us some time...

Ok, John the Ripper (http://www.openwall.com/john/) reports that my box can check 8,794 MD5 checksums per second. My machine is a P4 3.2GHZ.

Assuming someone picks a reasonably secure password (say, 8 characters long, alphanumeric with caps, and random (which i believe the Drupal password checker will claim is secure)), that means there are 62^8 different combinations of passwords.

My computer would take a maximum of

62^8 / (8794*60*60*24) = 287,364 days

to crack a pass. This seems pretty huge, but imagine if someone had access to 1,000 machines equivalent to mine (or maybe just 500 dual core, or 250 quad core -- hell, if we use Moore's law, you'd only need 125 quad core machines 2 years from now). This changes the value to 287 days AT A MAXIMUM. It would likely be cracked much earlier.

I was not aware that md5 was this quick to generate. I'm starting to think the best option here is to have Drupal use a well known interface like PHP's hash() function to list the available algorithms and pick a reasonable default, falling back onto the crappier ones if needed. Password stretching would be a nice feature to add, but probably shouldn't be enabled by default. Perhaps it could have a 'benchmark' feature that auto-picks a good number to use for password stretching...

Thoughts anyone? I know some people don't really care about the security of their passwords on Drupal, because they use OpenID or don't have SSL enabled and their password is sent in cleartext anyway, but surely this would be a nice feature for Enterprise level web applications...

@chx That makes sense. What would be the best way to generate a random string that long? /dev/urandom if available?

MD5 is designed to be fast and NOT designed to be a secure password hashing mechanism. I think using the phpass method is the best choice of validated hard-to-crack methods. Users log in rarely, so performance should be alomst an non-issue.

If we want to take chx's suggestion, for a sipler per-user hash here's a idea: store a sha1 or some such of the initial e-mail as the salt, and/or generate a random string as core does in drupal_get_private_key(). Or store several such salts and do something like
hash('sha512', hash('sha512', $pass . $salt1) . $salt2 . $pass);

http://www.php.net/manual/en/function.hash.php
http://www.php.net/manual/en/function.hash-algos.php

(Remember - if they have your DB, they only need to crack the user #1 password).

Totally disregard what I said. I misunderstood how these tables work.

I think stretching is still really important - but it is important to note that the amount of stretching can be adjusted. It could even made configurable or, better yet, 'smart' - by doing a timed hashing test on install. I am sure that (after some benchmarking with typical setups and user login rates) we can arrive at a value that is reasonable for all sites. Even on really busy sites, people only log in rarely (in computer time), and these sites will already need to be on more powerful servers (since they have lots of users!) so the proportional impact should be comparable to a smaller site. As a comparison, the system phpass uses is very similar to the one implemented on all *nix systems, which can scale to even 100s of thousands of users on terminal based applications on regular hardware.

Also, phpass already supports intelligent selection of the best hash available.

While I agree that having it as a contrib is useful, I don't think that fully secure passwords should be an optional extra for Drupal, long term. In my view it is only a matter of time before someone lays their hands on a database dump for a high visibility Drupal site and cracks an admin password. Also, there are often many admins on a site, and so the chance that they all have long/complex enough passwords is much smaller.

@selmanj - there are a lot of common misconceptions in your comments:

With password stretching, it doesn't have to take 1 second to calculate a hash (although it is up to a site's admin to configure things that way). It could also be 10 ms (with stretching) vs. 1 microsecond or less (without stretching) - or a 10,000+ difference in computational complexity for an attacker. Is 10 ms to check a password at login time unacceptable? I don't think so, but if you do you can make it 1 ms and still benefit from password stretching.

What you have benchmarked with John the Ripper is not raw MD5, but rather FreeBSD-style MD5-based password hashing, which already includes quite some stretching. It corresponds to 1000 iterations of the MD5 compression function. Thus, the speed at which raw MD5 hashes may be cracked is roughly 1000 times higher, that is, it is in the millions or tens of millions of candidate passwords per second on a modern general-purpose CPU, or over a hundred million per second on a PlayStation 3.

When you calculate the number of different character combinations for a given character set and password length, you obviously only get the worst case number of candidate passwords that a password cracker program will need to try before it finds the password. If the distribution were uniform (but it is not!), the average time would be half that. With user-chosen passwords, it can as well be 1% or even 0.000001% of the worst case (based on my years of experience, this is actually quite realistic!) This is difficult to tell - it depends on a lot of things. So it is obviously wrong to rely on the worst case number alone. (Yet it is useful to consider this number as well.)

None of the algorithms available via PHP's hash() are suitable for password hashing without a high-level algorithm on top (that would add salting and stretching). Most of them are suitable with a proper algorithm on top. This means that there's no need to support more than one underlying crypto hash, and MD5 is about as good a choice as most others.

Security of stored passwords (or password hashes) is about as important even when passwords are being sent to the server for authentication in plaintext (no SSL). These two things ("secure" communication and "secure" password "storage") are not as closely related to each other as you seem to imply. Any one of them makes perfect sense without the other (and helps improve security), although obviously the combination of both is best.

...Meanwhile, I've contributed my faster-yet-portable MD5 implementation to PHP - it got committed to PHP_5_3 (as well as php6 HEAD), and thus it should be in 5.3.0+ - which means that the phpass "portable" hashes will be a little bit more efficient when running PHP 5.3.0+. A possible next step is to get an equivalent of crypt_private() from phpass into PHP itself, perhaps into PHP's crypt(), for much better efficiency, which translates into better security (by letting a site admin configure higher iteration counts).

How about this for a solution that's easy to upgrade to: for each user, store MD5(CONCAT(salt, MD5(password)))

To upgrade, take the existing password hash, generate a unique salt, store the salt in a new column and replace the existing hashed password field with an MD5 of the two combined.

The final hash doesn't have to be MD5, it could be anything that's supported everywhere.

Why stop there? Let's triple the security and do MD5(CONCAT(salt2, MD5(CONCAT(salt, MD5(password))))).

Why stop there? Because it's simple enough to work and strong enough to be secure, I guess.

again - let's use phpass - if nothing else, it will make it easier to do WP to Drupal migrations :)

@hickory - That's essentially what the patch chx submitted in #117 does, as well as the one I submitted.

@solardiz - Thanks for the clarifications. I agree now that password-stretching as well as salting is required for proper backend security. You mentioned that none of the algorithms in the hash() function are suitable for password hashing. Could we implement salting+stretching in core, and use that with algorithms in hash()? It seems like the more portable method for the future, and would allow easy upgrades for hashing functions if some new security weakness is found in MD5/SHA256/whatever, as no new code would need to be added to Drupal.

@boydjd - per-user hash is a must. speed doesn't matter. php.net says that mhash() is obselete and hash() is preferred.

http://us.php.net/manual/en/ref.mhash.php

@hickory - re: being able to upgrade existing hashes, without the users having to login for that to happen, it is trivial to achieve with any new hashing method we might choose - just pass md5(password) instead of the password itself into it. This will work with phpass, too. Unfortunately, phpass itself does not use MD5 like that, so your new hashes will be incompatible with those of genuine phpass (and thus with other apps that use phpass) if you do that. Maybe I should have addressed this earlier (before phpass got adopted by a number of apps), but my reason for not doing it in all cases was that the CRYPT_* hashes supported by phpass would become incompatible with those supported by other apps (native password hashes on Unix-like systems, Apache .htpasswd, existing password auditing tools, etc.) As to doing it within the "portable" hashes algorithm alone, maybe this was in fact a good idea... but it's too late now, and it would only be of help for apps that upgrade from raw MD5 and not from something else.

Given the above, my recommendation is to use phpass as-is, passing the actual password to it. This means that upgrades of existing hashes will only occur as users login. Maybe this is the desirable behavior anyway.

And obviously your specific proposal is unacceptable because it lacks password stretching.

@selmanj - technically yes, we could support multiple underlying hash types, but it's a bad idea. It's not any "more portable" to have different hash types supported under our high-level salting+stretching algorithm. On the contrary, this will only result in portability problems for those weird hashes. As to new security weaknesses potentially being discovered in the underlying hashes, I find it extremely unlikely that a weakness in MD5 will be found that will affect its use in phpass. There has not been a single cryptographic weakness in any of the hashes we're considering (MD5, SHA*) discovered so far that would have a real-world impact on their use for password hashing, even without salting and stretching. They are about as unsuitable for password hashing when used raw as they were 10 years ago, and they are about as suitable when properly "cooked".

Here's another reason to not use other underlying hashes:

@boydjd, @pwolanin - speed does matter - or more correctly, it's implementation efficiency that does. The more efficient PHP's implementation of a hash is, the smaller the performance gap between password validation (at login) and password cracking. This is why phpass prefers the CRYPT_* hashes, which implement the stretching loop in C (or even assembly). With the "portable" hashes, phpass has to do that in PHP, which is several times slower, effectively reducing the stretching potential by a few bits. This is why in my previous comment I mentioned that it makes sense to get phpass "portable" hashes supported by PHP natively - that way, they would remain "portable" (because they are trivially implemented in PHP code - which is done in phpass now), yet about as good as CRYPT_* in terms of stretching potential when running new (future) versions of PHP. If that is done, it will be done for one underlying hash type only - and MD5 is it, because it's the most portable one (we want those hashes to be implementable when running on old versions of PHP, as well as in other scripting languages).

Then, even if we don't get this native support into PHP, it is easier to ensure that PHP offers a near-optimal implementation of just one hash - and MD5 is it right now (for 5.3.0+).

I must admit that slower hashes - if their slowness is genuine and not caused by poor implementation - would be a bit better, because the relative overhead of PHP code in the stretching loop would become smaller - but those hashes are not sufficiently portable (across versions/builds of PHP and to programs in other languages).

ok, hopefully something worth review.

provides for a pluggable .inc file (like cach.inc). Rehashes all passwords on update using users.init as salt. Allows a default fallback to this salted password, and automatic updating to a stronger hash. Admins like Moshe can thus reset any password (or set during mass import) from SQL as:

UPDATE users SET pass = MD5(CONCAT(init, MD5('newpassword'))) WHERE name = 'moshe';

Patch provide a default .inc file which is phpass, with a couple wrapper functions thrown in to make it generic and allow us to alter or re-write it later to cut the non-portable stuff, re-write for Drupal code style, use a different method, etc.

Note, some ideas borrowed form this WP patch: http://trac.wordpress.org/changeset/6350
especially if ( strlen($hash) <= 32 )

#162 looks good (on reading - I haven't applied). I like the idea of doing a basic salting globally - at least this somewhat improves security (nominally) for the many users to rarely/never login to the site and so whose passwords would never get protected by phpass.

In terms of phpass itself, we have 4 options really:
1) Include/paste it in verbatim, even though the code style is quite non-Drupally (as with #162)
2) Update the code style, but leave it as a class (which is of course more acceptable in D7, what with PHP5!) and leave method/variable names as is
3) Update the code style and method/variable names, but leave it as a class (which is of course more acceptable in D7, what with PHP5!)
4) Fully Drupalize the code, making it not a class with normal Drupal naming, comments and code style (NOTE: this is already done in #103 - no need to do this again)

The further down the list we go, the more work we would have to do if a vulnerability was discovered and we needed to port the changes. However, I think it is worth considering that (a) a vulnerability in this part of the code is pretty unlikely (it uses well established techniques, and has been around for quite a while), (b) we have the author of the code (solar) on hand to help if required and (c) the actual method/function contents are basically unchanged, so it would basically just be a short exercise in mapping variable names.

Bearing all this in mind, and the general niceness of not making random exceptions to our code style I think (3) or (4) is my favored option, however it would be good to hear what committers feel about this, before we spend a long time finessing a patch and then decide to throw 90% of it out ;)

I lean to #4 as well.

Overnight I had a couple more thoughts:

we might want to either put some text in where init is blank (is that possible?) and/or concat something like ':'.MD5(name) to init to simply insure that none are blank or very short.

we should load * from {users} in the authenticate function. Then we can more efficiently use uid for the user laod.

we should have an API function that checks whether a non-MD5 password needs to be re-hashed also - this would be another potential use for other infor from {users}

@pwolanin - thank you for the patch. It looks mostly OK at first glance, but I like the patch from #103 better (I am referring to the actual code, not coding style here).

Having stuff pluggable is not necessarily an advantage. IMHO, for Drupal's password hashing, it will create more problems than it possibly solves. People will be inventing weak and non-portable hashing methods - we've seen plenty in past comments on this thread.

As to trivial re-hashing with a salt (but no stretching), my suggestion is that we either don't re-hash all old hashes at once (that is, drop this feature) or implement those "re-hashes" on top of phpass - I'll explain:

While making Drupal's "brand new" hashes incompatible with those phpass uses with other apps is no good, it is not nearly that bad to make the "re-hashed" ones slightly incompatible (your proposed patch produces incompatible "re-hashes" anyway). We can pass the old md5(password) (for already existing MD5 hashes) into phpass - but to distinguish the resulting hash encodings from those of phpass alone, prefix them with one more character - say, "U" (for "upgraded") or "M" (for "MD5"). That way, the full security of phpass will be available even for the "re-hashes" - and support for such MD5+phpass hashes (for all of the hashing methods that phpass supports internally) will be easy to implement in other apps (just check the first character and do an md5() prior to phpass if necessary).

Perhaps the "re-hashing" is to be invoked by an admin explicitly - that way, we won't have to use a lower phpass iteration count for those hashes, and Drupal downgrades will be possible until the "re-hashing" is done. Perhaps the admin should be reminded if "re-hashing" is yet to be done - upon each login.

The "strlen($hash) <= 32" check is wrong for CRYPT_EXT_DES hashes that phpass supports when portable hashes are not forced. CRYPT_EXT_DES hash encodings are 20 characters long. I suggest that you use "== 32" to detect raw MD5 hashes.

@Grugnog2 - as to coding style, I like (3) - this is something I might be willing to merge upstream, since the current phpass coding style is not mine anyway (it matches another application's coding style).

While vulnerabilities in phpass are unlikely (at least for critical ones), it is quite likely that I will release new and improved versions of phpass - e.g., to make use of new features of future versions of PHP to allow for better password stretching potential.

I hate to disagree again, but I really don't think init is enough for a salt. Salts should be fully random, and init is set to the email address the user is created with. If you look at the patch I submitted, it uses the output of md5(init . created . mt_rand()) for a salt, which includes both unique and random components.

Also, the salt should change when the user changes his password. This will not happen with salt set just to init.

@solardiz I agree that it could easily cause portability problems if someone selects a strange algorithm, since hash_algos() returns some pretty strange algorithms. It just seems easier down the road if we ever need to add a new hashing algorithm to just use hash() rather than coding the algorithm in the php file, as the blowfish algorithm is done in phpass (assuming you haven't applied the hardening patch).

I also liked the idea of using RFC2307 style passwords (ie: "{SHA1}") for password storage as it would be immediately obvious what hashing method is used when porting the passwords from one application to another. Phpass doesn't do this, but that is just a minor preference for me.

HOWEVER, ultimately I'd just like to see stretching+salting in Drupal. I like the idea of #4 in Grugnog2's list.

I'm sorry if these questions have already been answered, but...

Don't you already have major problems if someone has your DB? Is the utility of this patch just to make passwords harder to get after your site has already been owned? If that is so I don't really see the usefulness of the patch.

Simple salting with created time should be sufficient to prevent md5 lookup tables from being effective. I like the earlier solutions that used created.

I also do not approve of any solution that cannot be implemented in SQL.

@selmanj - thank you for your comments.

Unfortunately, even MD5 vs. SHA-1 for the underlying hash can cause portability problems - for no gain at all. Why would we "ever need to add a new hashing algorithm" - speaking of the underlying crypto hash only - given that the higher-level stuff is our own anyway, so our phpass-like hashes with another crypto hash wouldn't be compatible with anything else? OK, I can think of one possible reason - this is in case other similarly misguided people replace the underlying hash in phpass with something else and use that for their app, and we want compatibility with that on some specific Drupal install. But do we really need to cripple our code in advance just because there are possibly other misguided people out there, trying to guess just how they might cripple phpass (replace the underlying hash but not do anything else)? In my opinion - no.

I also respectfully disagree with your suggestion to use the RFC 2307 syntax for identifying hashing methods. We'd only be able to use {MD5} and {CRYPT}, with the latter being quite non-informative because the actual "crypt" hash type would continue to be represented by the second prefix, where the de-facto standard is to use the "$id$" notation, with "id" identifying the hash type. (This started with phk's implementation of MD5-based hashes for FreeBSD 10+ years ago. CRYPT_EXT_DES hashes pre-date that, which is why they use "_" for the hash type identifier prefix instead.) Then, if we have to invent something custom - such as raw MD5 post-processed by phpass (for "re-hashing") - there's not an established RFC/LDAP-like prefix string for that anyway.

@dopry - yes, people already have major problems if the DB gets into the wrong hands. Yet such things do happen, and it makes a lot of sense to mitigate the impact of such occurrences. Even if a site has already been "owned", that does not mean the attacker(s) have no interest in breaking the users' hashes - quite often they do. This is a way to maintain unauthorized access to many user accounts for a long time after the compromise is detected and the few truly critical accounts' passwords changed. The cost and time to forcibly change all users' passwords is often substantial or even prohibitive. By using stronger (salted and stretched) password hashes and by enforcing strong passwords or passphrases, the percentage of user accounts whose passwords will absolutely have to be forcibly changed (if the DB gets compromised) can be reduced a lot - like from 90-100% (for raw MD5 hashes and no password policy enforcement) to 0.1% (for phpass with reasonable settings and good password policy enforcement). Those few weak passwords can be identified by a site admin with password auditing tools such as John the Ripper.

Additionally, there may exist vulnerabilities - not necessarily in Drupal itself (or in its modules) - that may "leak" some of the password hashes without a full DB compromise. It may be surprising to some that such vulnerabilities exist even in OS kernels (e.g., they are being discovered and fixed in the Linux kernel quite often - and other operating systems with monolithic kernels are not very different). Most of these are "local" - may be exploited from another Unix account, or from another VPS/container on the same machine (e.g., FreeBSD-SA-05:02.sendfile has been successfully used to obtain password hashes during penetration tests), but a few are even "remote" (e.g., the Linux 2.0 leak of "uninitialized kernel memory" via ICMP).

"MD5 lookup tables" are by far not the only threat here. I'd say they are not even the primary threat as "traditional" password crackers such as John the Ripper are more effective than the tables when attacking large numbers of hashes at once or when the hashes are properly salted (which defeats the tables).

No sensible solution can be implemented in somewhat-portable SQL alone. As an exception, PostgreSQL with contrib/pgcrypto supports the bcrypt hashes (same as PHP's CRYPT_BLOWFISH ones), but my understanding is that Drupal is meant to become more portable (including support for different SQL backends), not tied to PostgreSQL with contrib/pgcrypto.

I really think there is a cool, Drupal way of doing this. If I may be so bold as to represent the Drupal-way, I don't think plugging in phpass is it (remember xmlrpc). I do however think phpass ought to be an option, as well as traditional MD5, PHP hash, AES, and separate binaries installed on the filesystem.

Installer or install profile:

Hi, I see you have password extensions available. Select the hash algorithm below you would like to use:

+ md5 (default)

- + PHP Hash module -------------------------------------
- md4
- sha1
- sha256
- sha384
- sha512
- ripemd128
- ripemd160
- whirlpool
- tiger128,3
- tiger160,3
- tiger192,3
- tiger128,4
- tiger160,4
- tiger192,4
- snefru
- gost
- adler32
- crc32
- crc32b
- haval128,3
- haval160,3
- haval192,3
- haval224,3
- haval256,3
- haval128,4
- haval160,4
- haval192,4
- haval224,4
- haval256,4
- haval128,5
- haval160,5
- haval192,5
- haval224,5
- haval256,5

- + phpass module -----------------------------------------------

- Use phpass's strongest option

- + AES encryption ----------------------------------------------
[description to link to http://drupal.org/project/aes
This module can provide you with readable passwords.

- Use me!

Site admin:

Wow! There are so many options, I'm glad they linked to more information on each algorithm for me to learn about.

Installer (pg2) / upgrade.php?

Mark existing passwords for replacement on next login:

+ Yes
- No

My point is all the methods discussed so far have a noteworthy flaw. I think salting MD5 is a waste of time in this discussion because you still end up with a MD5 hash. That was my point with the three layers of MD5ing. Nobody could take that seriously because it's not any more secure than it was with one salt. Re: 114 - you could salt with all the data in the database, but if the password in the account is still less than 8 characters in the dictionary, it's probably been rainbow recovered for years now.

I think the idea behind this task needs to be to look for ways to extend beyond using MD5, not find a specific replacement. Talking about a cool way to make the password hashing extendable is where I think this discussion would be more productive. Yes, people will make stupid hashing modules; no doubt there will be at least one joker make a rot13/xor/base64_encode addon. People will laugh, but I very much doubt someone who goes to the extra effort to change their hashing from MD5 will do so completely uneducated.

I have PHP hash installed on my server and can't wait till I re-create my 1 account on my blog with a whirlpool hash (sodium chloride, optional).

@solardiz - thanks for you feedback. Let me work on the code more. The problem with updating directly to a stronger hash is the potentially long time it may take during the update (or an import) for large numbers of users.

Also, perhaps this is a point at which we should require in core a minimum password length? Perhaps 6 chars?

I respectfully disagree about the pluggable framework. I think it's rare that people will go to the trouble of implementing a different hashing scheme, but imagine that a specific site requires it for some other reason (for example, it's for a government agency that specifies the hashing scheme in the contract requirements). Making this easy to do is good for everyone. Another example - you want to use the strongest phpass hashes for your site, but the Drupal core code only supports the portable hashes.

If someone put a module on d.o implementing an alternative but weak hash, I'd hope the security team would ask the contributor to remove it (chx?).

If the purpose of this patch is to mitigate password hash leaks, I don't think changing the hashing method is an effective solution. The standard practice of most systems adminstrators for exploited systems is to change all passwords.

It seems like adding a UI to reset all user passwords with a custom email option would be more effective at mitigating the risks of compromised databases, and far simpler than adding a complicated password hashing mechanism. user_password already allows us to reset users passwords.

I strongly oppose making it easy for end-users (site admins) to enable alternate or custom hashing methods. There should be easy choice between raw MD5 (for backwards compatibility), phpass (plus its configurable settings), and maybe the installed modules - but those modules should be separate from Drupal core and require separate download before they're even mentioned. The ability to add custom hashing methods (or reversible encryption or simple obfuscation), if present, must be under the hood, not immediately visible to a site admin.

Specifically, the long list of hashes on #169, most of which are inappropriate for password hashing, must not be presented to a site admin, who will be unable to make an educated choice - not even after reading some info on the hashes - it takes some people years of experience to get this right. I'd even say that a module that makes all of PHP's hash() functions available for such misuse must be banned from d.o. CRC-32? Raw Whirlpool instead of raw MD5? That's nonsense.

@pwolanin - re: the mass-update potentially taking a long time - you're right, and I've tried to address this in my recent suggestions. In fact, it is one of the reasons to not implement those mass-updates, at least not right away. The patch in #103 (or equivalent) may get in first.

As to requiring a minimum password length in core, this is fine, but there's more to password strength, and 6 characters is not going to be enough except when some really uncommon character combinations are used (and password stretching is in place). I think we should get the basic password hashing improvement patch in first (like #103), then start separate threads to discuss password policy enforcement and maybe local parametrization. (FYI, I implemented a widely adopted password strength checking / policy enforcement module for PAM - it is integrated into FreeBSD 5+ and several Linux distributions. Most of the code was written in 2000, and no major change was needed during those 7+ years. Maybe we just need to convert some of that code from C to PHP.)

You have some good points in favor of a pluggable framework this time. :-) What I criticized in #168 was quite different - namely, plugging different underlying hashes into phpass-like salting and stretching. I don't strictly oppose a higher-level pluggable framework, but I think that, if present, it must be under the hood and not too easy to use - not by typical end-users.

@solardiz - I agree with you in pretty much all respects. I would never present such a list of hash options on install- this would be terrible. The pluggable framework would be like that for caching - have you installed an alternate caching framework for Drupal? For 99% of admins, the answer is no. It's not available via the core UI, but might be implemented by specific modules.

@dopry - the point is that you may not know for days, weeks, or months that your DB was compromised. And the potential vulnerability for DB exposure will increase in 7.x if SQLite is supported in core.

@pwolanin: and you really think a complex hashing algorithm is going to deter a determined hacker who has dictionary based attacks at his disposal and can easily replicate whatever password hashing algorithm you're using? Brute force tends to be successful even if it takes longer, changing passwords is still the only acceptable solution to mitigate risk in this scenario. Once the user data is compromised it is only a matter of time no matter what hashing algorithm you use.

@dopry - I might point out that we have the author of one of the most successful password testing/cracking tools in existence on this thread. If anyone would know exactly how to best thwart someone trying to attack a password list it would be solar - it is his job ;)

Please note that making the hashing algorithm more 'complex' is not the point, but instead to make it as hard as possible (without harming server resources) for someone to crack the passwords, if they should be uncovered. If you are arguing that the passwords hashing should be made less complex then you will need to back it up with strong arguments as to why your proposal is better. Passwords are valuable things, and despite every flashing warning people still use the same password for multiple accounts (banks included, sadly) - hence, I think we have a strong obligation to protect those passwords.

Obviously if your database is downloaded you have already lost half the battle. Obviously changing passwords in the event of this is a good idea. Obviously encrypting the credentials transfer is a good idea. Obviously at some point the NSA or a quantum computer will defeat whatever we come up with. However, none of these are good arguments against securing passwords with the best means possible.

ok, here's a new patch.

I have cut the phpass part down to use only the portable hashes, only PHP 5 (since 7.x requires PHP 5.2), and made it more consistent with Drupal code style. The initial update in system.install now uses CONCAT(init, created) as the salt.

I also added a new public method to the PasswordHash object and tweaked the object code a little so that we can easily check if the current hash has less than the standard number of iterations.

The .inc file has a simple, but flexible API of 3 functions. By passing in $account when checking the password, it makes it easy to use this API in lots of ways by replacing the .inc file and following the prototypes of these functions.

I've done basic checks of the update functionality, as well as that this code works with a portable hash generated with the unmodified phpass code. Of course, detailed review appreciated.

In no way am I suggesting password hashing be made less secure. I'm simple pointing out that there isn't a whole lot of return to be gained by adding more complex password hashing. A simple salting should be sufficient to complicate the password cracking process. The only thing more complex hashing algorithms really buys you is time.

Suggesting that a particular hashing algorithm makes a hash safer post exploit creates a false sense of security. Solardiz is correct in stating that the security of hashes is important, because it buys you time post exploit. However, it is very unlikely you will be aware of an exploit until after an attacker has already compromised several privileged user accounts.

The only solution once your system has been compromised, no matter how tedious, is to update all user authentication information. Aside from just protecting your site, it is good manners to inform you users that your site has been compromised and as a result their password should also be considered compromised. It's at this point you should also tell them if they use the same password for other accounts, those accounts should also be considered compromised.

Improving password hashing is simply not an effective solution to mitigate the risk of compromised user data.

@pwolanin: sorry didn't mean to change status... races are fun.

no, I disagree. If the proposed stretching (2^14 hashing operations) turns the time to exploit from a day to 1000 days, I think that's meaningful.

Also, I assume you did not mean to change state.

I don't think that there is a real need to worry about people having the option to change their hashing method. If somebody really wants to change their hashing method, in the process of learning how to change it, they will in turn learn something about security enough to see if they really should change it. The only people that should be able to mess with their own hashing methods are people who know what they are doing, so in short, I think there should not be a choice of their hashing method, exleast in the core, let someone get a module for it.

@dopry: At the end of the day, any password hashing scheme is 'just about time'. If you continue your line of thought ad absurdum there isn't any point in having any password hashing at all, since even straight md5 hashing 'only buys you time'.

The aim of a good password hashing scheme should be to increase the time it would take to crack as much as possible - at the very least to a level where with current technology the probability is high that it takes a 'very long time' to crack a good password. Password stretching has been well recognized for decades to provide this, by forcing the repetition of a well optimized operation, and is critical to increase the average 'crack time' to an acceptable value.

Changing passwords when the system is compromised is also obviously a good policy, but is IMHO a completely different issue to the one we are discussing here. It is concerned primarily with the 'cure', whereas password hashing is concerned primarily with prevention. A good hashing scheme can prevent you getting to the point where you need to force all passwords to be changed, because it can make it unfeasible to crack the passwords, even if you found a database dump.

I reviewed the patch in #176 and tested that it works on a fresh install.

I believe that phpass only requires 60 characters. Do we need to set the password length to 128?

I like the check for the current minimum hash length. But why 14 as the minimum password hash length? I'm using 8 in the 5.x/6.x phpass module (and I'm not sure why, other than I think that was the minimum).

You've done a nice job in removing un-needed stuff in phpass (non-portable hash, pre php 5). But I'm now second guessing why are we changing phpass at all? to match Drupal coding standards? because it has too many lines of code? has Dries already weighed in that this should be done? I know that my initial patch did this too, but for a security tool, I think we'd want to be as safe as possible, and I think the safest thing is to include phpass unaltered. Solar will now need to re-review this to make sure we didn't lose anything. And G-d forbid, if there's ever an update to phpass, we'll need to re-write it again, rather than just release the fix immediately.

I like the way you added the salting for people who haven't logged in.

I'm a little concerned that the 5.x and 6.x phpass module that I wrote changes the password field to be 'phpass'. I did this so that I didn't have to change the length of the user.password field (which I think is generally a bad practice). If we go with your code as-is, we'll need to write an upgrade for the contrib phpass module.

Can the modified version of phpass authenticate users whose phpass was generated by un-altered versions of phpass? Or is the contrib phpass module going to need to provide an upgrade here too?

@douggreen - I've spent several hours with the code, and the 2^8 you think you are using is really 2^13 on PHP5. I changed the code so that the input # and actual # match in order to enable the checking and processing of the iteration count using a common function. Also, Moore's law say that basically we should increment this number by 1 for every Drupal version, right? So if 13 is ok for D6, D7 should have 14.

The code is small enough that I don't think i will be a burden to maintain or fix such a customized version. In fact, I think that having the code match Drupal code style makes it more likely to be maintained, since it makes it clear that it's "ours" vs. some external entity.

I did test that a hash generated using the portable method and the unmodified phpass code works if I just paste it into the DB. And note that if I generate one using a lower # of iterations, it will be automatically re-hashed to a stronger one the first time the user logs in.

Also, since this is a varchar field, it should not require significantly more storage than what's used, right? Since the patch allows for alternate hashing schemes, it seemed also prudent to allow for longer hashes, rather than restrict the length to what core is using.

If anyone still has doubts about the importance of having secure hashes:
http://www.devicepedia.com/security/harvard-site-hacked-and-then-leaked-...

Apparently the password hashes are also included in the torrent. I'm sure people are trying to crack the passwords now, and if it they're stored as a plain md5() hash it won't take them long. If the hashes are stored using phpass or some other password stretching/salting method, I don't think any of them would get cracked before everyone can change their passwords.

If I were the admin there, I would feel a lot better about losing the hashes if phpass were used. He/she is probably scrambling right now to get people to change their passwords everywhere they use them.

Okay, I just applied this patch. Since I don't have the expertise the code, I logged out and logged back in. I am not sure if this is simply how the patch works, or if it is broken, but after applying this patch my username and password no longer worked. It worked again after unapplying the patch.

I don't know if that's my fault or not.

Setting to CNW after retesting on a clean install of head.

Ugh, my bad. Had to run update.php. This works fine.

slightly revised based on chx's comments- I need to retest.

re-tested, and seems to works as expected. Hashes can be use with or from the unmodified original phpass code.

note, new code for function get_random_bytes($count) is borrowed from: http://api.drupal.org/api/function/drupal_get_private_key/6

though, I guess if we believe Moore's law, we only need to increment the log2 number of hashes every 2 Drupal versions :)

Further work on the code to make this Drupal version as lean and clear as possible. Re-checked functionality and re-checked compatibility again with portable hashes from the unmodified framework code.

note: http://drupal.org/node/216301#comment-715861 hence assigning to me.

When someone re-rolls this for more substantive changes, note that

+ * The standard log2 number of iterations for password streching. This should

has a typo.

Regarding

+ * This modified version and other file contents licensed under the GPL.
+ * See COPYRIGHT.txt and LICENSE.txt.

If this section of code is going to be included under its own copyright header, which the included patch implies but does not state, then IMHO we need to add a line for this at the end of COPYRIGHT.txt, similar to the way the jQuery entry is formatted.

fixed typo and cut back the top docblock based on feedback from Crell, webernet, and keith.smith in #drupal

after discussion with grugnog on IRC, refactored the .inc file similar to #103 to use totally non-OO code.

retested and all seems fine.

ok, compiled and used the test suite here: http://www.fourmilab.ch/random/

The random bytes in #195 are actually quite bad. This patch is much better in terms of that evaluation - I've switch it back to /dev/urandom as the default and at least on the Mac the backup is about the same in terms of quality.

(Suggests that drupal_private_key() maybe should be altered.)

here's the ocde I tested, and sample results attached as well as the patch.

$fp = fopen('./test-data.bin', 'ab');
if (($fh = @fopen('/dev/urandom', 'rb'))) {           
  for ($i=0; $i < 200000; $i++) {
    fwrite  ($fp , fread($fh,6), 6);
  }
}
fclose($fh);fclose($fp);

$fp = fopen('./test-data2.bin', 'ab');    
for ($i=0; $i < 200000; $i++) {
  $output = substr(hash('sha256', mt_rand() . uniqid() . mt_rand(), TRUE), 0, 6);
  fwrite  ($fp , $output, 6);
}
fclose($fp);

$fp = fopen('./test-data3.bin', 'ab');       
for ($i=0; $i < 200000; $i++) {
  $output = substr(hash('sha256', uniqid(mt_rand(), TRUE)) . md5(uniqid(mt_rand(), TRUE), TRUE), 0, 6);
  fwrite  ($fp , $output, 6);
}
fclose($fp);

note also, why mt_rand() is the fallback, not default:

"PHP: mt_rand() uses a Mersenne Twister, but is nowhere near as good as CryptoAPI’s secure random number generation options, OpenSSL, or /dev/urandom which is available on many Unix variants. mt_rand() has been noted to produce the same number on some platforms – test prior to deployment."
http://www.owasp.org/index.php/Cryptography

Reviewed this code, and it looks clean to me. I think non-OO (as with Doug's patch in #103) is the way to go here, since using a class adds little in this case, and is not really the kind of data/functionality that we have talked about possibly OO-ifying fully in D7, such as files nodes, users etc.

I don't know if it has been noted, but this version only supports the md5() primitive, because otherwise it could cause significant trouble for people (especially on shared hosts) when moving ISPs (i.e. their passwords no longer work because a hash primitive becomes unavailable). Also, the password hashing is now pluggable (in the style of caching etc), which allows a module to add back in the other phpass hashing primitives if their host supports them, and also provide other options for non-portable hashes and iteration counts etc. Sticking with md5 only also makes the code a lot simpler, which should make it easier to understand, maintain and port any phpass improvements to if necessary (even though the functional naming is now slightly different).

This 10kb of code is a huge security leap for Drupal password security in my opinion :)

chx wisely pointed out to me that hash() is not assured of being available (PHP may be compiled with the "--disable-hash" switch).
So minor re-roll substituting sha1() which is always available for PHP 4 >= 4.3.0, PHP 5.

note that using either md5() or sha1() in place of hash() gives me "ent" results that are pretty much indistinguishable from those for hash() shown in http://drupal.org/files/issues/ent-eval.txt

If anyone cares, I can post them.

I applied the patch in #198, and installed Drupal using a new database, and tested logging in and out, creating users, etc., all of which worked fine. As an added bonus, I could no longer copy out the pass value in the database, paste it into Google and determine my password. :)

Several points:

- _passwordhash_random_bytes() uses /dev/urandom, which does not exist on Windows. So on Windows, $output is empty. How does that affect users on Windows? Anyone tested it there?

- Right now, you can go into the database and update the pass field to md5('yourpassword'). This is very useful for support and development. How would that work under the new scheme? Doesn't seem like MD5(CONCAT(CONCAT(init, created), MD5('yourpassword'))) is enough, like in the update function.

aclight asks a good question about edge cases for init as salt, and it seems that since it must have passed though
http://api.drupal.org/api/function/valid_email_address

this field cannot contain any special characters though might be escaped or cause other problems.

@kbahey - there is a fallback to the PHP code if /dev/urandom does not return any bytes. This should work fine on Windows.

@kbahey - I've tried MD5(CONCAT(CONCAT(init, created), MD5('yourpassword'))) to set the password and it worked for me - did you try?

In system_update_7001(), instead of

+  // Rehash with a salt all current hashed passwords.

I would have said

+  // Rehash all current hashed passwords with a salt.

I tested this out and it works as advertised.

However, there is one issue with the update that I feel is a problem.

If the code is changed (by patch or presumably by downloading the eventual tarball) but the user is not logged in as uid = 1, they will have to log in as uid=1 to run update.php. However, this will be impossible because this patch will expect that the value of password stored {users} will have been rehashed already. To get around this the user will either have to change the password hash in the database OR will have to modify the settings.php file and change

$update_free_access = FALSE;

to

$update_free_access = TRUE;

The recommendation to first login as UID = 1 is made in UPGRADE.txt, but I suspect that not all users follow that every time. In addition, as mentioned in #225880: non-writablilty of settings.php when created by webserver, it's possible that users on shared hosts will not be able to modify settings.php in the first place, further complicating the matter.

An alternative would be to set a variable when system_update_7001 completes, and then in the new user_check_password() function see if this variable is set, and if not then compute the password hash as md5($password) as we currently do.

UPGRADE.txt gives you advice to log in and that's about it. Yes we can do more, but this issue is nice and good to go.

I tested this on Windows Vista as well and it works.

checked update code and basic functioning on PostgreSQL 8.2/PHP 5.2.3, and all works.

@pwolanin

Here is the pass field when I use "yourpassword" as the password.

mysql> select pass from users where uid = 1;
+------------------------------------+
| pass                               |
+------------------------------------+
| $P$CeGv8znvpkRPOIaIIYrNO8SxhhwtYP. |
+------------------------------------+

And here is the password generated by the MD5/Concats

mysql> select MD5(CONCAT(CONCAT(init, created), MD5('yourpassword'))) from users where uid = 1;
+---------------------------------------------------------+
| MD5(CONCAT(CONCAT(init, created), MD5('yourpassword'))) |
+---------------------------------------------------------+
| 408a53c95e938da455a24aef9c92ba8c                        |
+---------------------------------------------------------+

As you can see, they are not the same.

@kbahey - true they are not the same - the former is the stretched and salted phpass password, and the latter is a non-stretched but salted md5 password (easy and fast to generate from our current md5s), which is better, but not really as strong as we would like. This patch updates all passwords to the salted md5 variant in update.php, but then transparently updates each password *on login* to the phpass password.

Hence, if on a large bulk import you need to automatically insert users with the salted md5 passwords this is not a problem, since Drupal will check the password using the appropriate function and (if correct) log the user in and transparently rehash each one with phpass. I hope this answers your question.

@pwolanin - First of all, thank you for working on this!

I still like the patch from #103 better, but it appears that you win, so be it. Here's what I dislike about your patch from #198, in arbitrary order:

mt_rand() must be explicitly seeded in older versions of PHP (apparently, below 5.2.1). I don't know if other Drupal code takes care of that; if not, then either you should or, better yet, you can use code similar to what I had in phpass instead - or just leave that code unmodified. There's no magic about PRNGs - no matter how good a PRNG is, it can't do better than it's seeded. Thus, I see no advantage from the use of mt_rand(). On the contrary, if you don't seed it and you run on an older version of PHP, you'll get the same "random" numbers each time. Joomla suffers from this as it relates to their initial passwords and salts. They do seed mt_rand() explicitly, but in a way that only 1M different seeds are possible - and indeed their many calls to mt_rand() can't and don't introduce any additional randomness. (Perhaps Drupal's initial passwords are similarly problematic - I just have not looked into that yet - to be taken care of once we're done with password hashing.)

Similarly, there's no need to use sha1() in _passwordhash_random_bytes() - md5() will work just as well. Not using sha1() could save a few PHP .text pages from being loaded into RAM.

I had standardized on the bcrypt-equivalent log2 numbers for the phpass API; you "broke" that. I understand and agree that you've made the code a bit more straightforward by using the same log2 numbers everywhere in this version of the code, but this might make things more difficult in the long run. What if we get a CRYPT_PHPASS into PHP itself, allowing for higher iterations counts when running on those newer versions of PHP? I'd approach it similarly to the way I did for PHP 3/4 vs. 5 - same bcrypt-equivalent log2 numbers on the API, but different numbers of iterations for new phpass hashes produced within phpass code.

Then, I think the iteration count for new hashes should be made configurable by a site admin. And it is better to make it a standardized number that translates to roughly the same number of milliseconds when running on the same hardware, regardless of version of PHP (and hashing method choice, in case CRYPT_* are re-introduced). #103 had this desirable property - your patch lacks it.

As to the CRYPT_* hashes not being portable enough, I agree. If you'd use phpass with portable hashes forced by default, I would understand and agree. What I regret is that you've completely dropped the CRYPT_* support, not letting a site admin consciously enable the use of those not-guaranteed-portable but stronger hashes. Yes, I understand that such support can be re-introduced by replacing the .inc file - thank you for leaving at least that option. :-)

I really don't like the re-hashing to temporary hashes that are neither raw MD5 nor phpass. If you want to do any mass re-hashing at all, let me repeat my suggestion: use phpass-of-MD5, but prefix those hashes with an "M". If you want this mass re-hashing to be fast, then use the lowest allowed phpass iteration count for that. With present code, that would be 129 MD5s, which you will likely find fast enough on PHP 5 - but if not, I am willing to relax the minimum iteration count check in the upstream phpass code as well.

One of my concerns is the ability to use password auditing tools on Drupal databases. While those tools are indeed "dual use", let's not rely on the obscurity aspect. It is beneficial for site admins to be able to audit their hashes. With your salted but not stretched MD5s, the hashes will be so fast that efficiency (or lack thereof) of the audit tools will depend on the "overhead" of many of their code layers. If a malicious cracker has a lower-overhead tool, or is willing to use a special-purpose tool that is less convenient to use, they will gain an advantage. With phpass and at least some stretching enabled (e.g., the 129 iterations), it's the actual low-level crypto code that will affect the audit tool's efficiency most - and this is desirable.

You're replacing raw MD5 hashes with your re-hashed ones, yet you seem to store the new hashes in the same way - same 32-character strings, with no hash type identifier. This may be confusing and it may cause real problems as well. (Once again, I do not think it is wise to rely on this obscurity aspect for security.)

You use MD5() in an SQL query. You also use md5() in PHP anyway, so better stick to it in all cases (or better yet, do "M"+phpass).

I am sorry that this comment is full of criticism; my hope is that you will use it in a constructive way.

There are also many things about your patch that I actually like; it's just that those did not need to be brought to your attention. ;-)

Marking code needs work based on solardiz's comments. Can a couple people update/ test #103 instead?

@webchick - I'll look at #103 and see about rolling in some of the differences. I think that sticking with the fully portable hashes as the sole default implementation is the better choice, for the reasons mentioned by Grugnog in #197.

@solardiz - the mt_rand() docs say: "As of PHP 4.2.0, there is no need to seed the random number generator with srand() or mt_srand() as this is now done automatically."

However, I also see this user-supplied note:
"The algorithm used by mt_rand() changed in PHP 5.2.1. If you are relying on getting the same sequence from mt_rand() after calling mt_srand() with a known seed, upgrading to PHP 5.2.1 will break your code. See http://bugs.php.net/bug.php?id=40724 for something of an explanation; there is no workaround."

Reading the bug entry, it only sounds as though the seeding algorithm changed, not that there was a fundamental flaw with seeding in the prior version. I did put uniqid() there, which is supposed to be based on microtime() but seems to give me a little better results in terms of "ent". So even if mt_rand() is totally broken, this is about the same as the original code.

@solardiz - I see that #103 allowed the user to configure the number of hashes, but I do not see anyplace the timing was tested. Or do you just mean the normalization to the bcrypt iteration count?

After the various debates in this thread, I think that there is no good good reason for the default (core) version of this to have any user-configurable options, and that minimizing this code for core will help it be maintained effectively in the long run.

@solardiz - I was not intending to rely on obscurity for the re-hashed passwords, so certainly adding an identifier is possible. The debate (in this thread and in IRC) is that some admins rely upon the ability to set/reset passwords via simple SQL. I really don't see that as that important, but it's a debate within the community.

After contemplating the length of time it would take to update ~200,000 users in batch mode, I think sticking to a single SQL query is the only feasible choice. This patch adds a simple 1-letter identifier 'U' to indicate that it's an updated hash.

Otherwise, during the update we have to do one update query per user - if someone wants to run the timing on this and thinks that even for the query alone we can do several thousand per page load on modest hardware, I'll write the batched update.

Also, as requested I added back the offset of 5 for the log2 count for purposes of comparability to the bcrypt number, and I further added a variable 'passwordhash_count_log2' so that the # of iterations can be configured easily via an add-on module or in settings.php.

I altered function _passwordhash_random_bytes($count) to be closer to the original and only use md5().

I also changed function user_needs_rehash() to user_needs_new_hash() to avoid confusion with the use of the existing user_pass_rehash() function.

@Grugnog2

Thanks for the clarifications.

@pwolanin

The 214 patch works fine on a fresh install. I think the U identifier is a good thing.

Here is why:

For development, testing and support, we now use MD5('realpassword'). With the new salt and such, I think the workaround would be to use the U trick, like this:

UPDATE users SET pass = CONCAT('U', MD5(CONCAT(CONCAT(init, created), MD5('realpassword')))) WHERE uid = xxx;

Then, on login, the salt would kick in, and the password would have the $P$ and the new hash.

@pwolanin - you're correct re: mt_rand(), and I was confused. Yes, according to the docs, the auto-seeding was already included in PHP 4.2.0. I've just tested 5.0.3 vs. 5.2.5 with and without manual seeding. Without manual seeding, both produce different sequences of mt_rand() numbers on each invocation. With manual seeding, both produce fixed sequences of mt_rand() numbers - but the sequences differ between these two versions of PHP. This means that Joomla is just as vulnerable on 5.2.5 - only the specific set of 1M initial passwords is different - and someone with a list of 2M possible initial passwords (that's just 18 MB with Unix linefeeds) can crack Joomla password hashes in seconds (for those users who haven't changed their initial passwords). ;-)

As to the seeds to use, the original phpass, when /dev/urandom is unavailable, would use "microtime() . getmypid()" for $random_state at initialization, and then mix more microtime()s into $random_state using MD5 as random numbers are requested. This is far from perfect, but I think it's very slightly better than what you have in the 214 patch, where you rely on microtime() and PHP's internal seeding of mt_rand() - which, in turn, combines time- and PID-derived values into a 32-bit seed with multiplication (I've just checked PHP 5.2.5 sources), which may make some PIDs "indistinguishable", thus making use of less "PID entropy". For better effect, you may keep what you have now but initialize $random_state to getmypid() rather than to an empty string.

#103 did not do any timing, and, yes, I was referring to the iteration count normalization for different hash types supported by phpass.

You have a typo in a comment in _passwordhash_get_count_log2() - "bycrypt" instead of "bcrypt".

Maybe user_needs_new_hash() should return TRUE if the iteration count is different from the currently configured one, not only if it's "less". That way, a site admin will be able to convert the hashes to faster ones after migration to a slower machine and/or to an older version of PHP - or after having re-considered the initial decision on the iteration count to use. Or maybe this change could be introduced later, along with CRYPT_PHPASS (because that would mean a 10x performance difference between different Drupal-supported versions of PHP).

Thank you for considering my suggestions, and for applying those changes that you did choose to make.

after discussion with chx, here's a version that does re-hash all passwords using stretching as suggested by solardiz. This is a bit time-consuming and CPU-intensive, so those updating large sites, will need to walk away and have their lunch while it runs...

Those doing large user imports can use the batch API and essentially the same code as this update, but can set the iterations lower if needed for speed.

ah - apparently cross-posted while I was testing the new patch. This version differs following suggestions above in spelling correction, initialization of $random_state as suggested, and checking inequality (rather than <) as compared to the current iteration setting.

@pwolanin - the #218 patch looks good, thank you!

Here's one concern related to the new way of re-hashing: the way you temporarily alter the passwordhash_count_log2 variable, its reduced value might affect more than just the old passwords being updated. Also, there might be scenarios where the variable_del() is never reached. You could want to see if there's a clean enough way to avoid these issues - maybe by using two variables?

Also, rather than hard-code "8" and have a comment that it's "1 less than DRUPAL_HASH_ITERATION", you could use two defines - say, DRUPAL_NEW_HASH_ITERATIONS and DRUPAL_UP_HASH_ITERATIONS.

I suspect that the default for DRUPAL_UP_HASH_ITERATIONS will have to be reduced from 8 to maybe 4 or 2. Or you may have it depend on the number of users in the database.

@solardiz - I hard code it at 8, since the admin may be doing an update with an alternate hashing scheme (i.e. an alternate include file) and hence DRUPAL_HASH_ITERATION may not be defined.

Another way for this would be to have an optional second param for user_hash_password. I don't like it in some ways, but I would mean not relying up the variable being cleared.

Also, since we are doing this batched, the time constraint is mostly on the execution time per page load, rather than the total (assuming that people can be patient enough to let the whole thing run). With this set at 8, it took a couple minutes to update 5000 users on my macbook. Maybe it could be set down to 7 or 6 and the number per page load increased to 1000?

ok, this implements the above-suggested changes.

Also, is 9 high enough for DRUPAL_HASH_ITERATION?

On my desktop (pretty fast and not loaded with much other stuff) I can do 140 users rehashes a second. This means that a half a million user site (i.e. very big indeed) would be done in about a hour, which seems pretty reasonable to me, considering that any site of that size will expend many human hours on upgrading other parts of their site. What do other people think?

Another approach is to do what we do with the search index, and just do this in the background, perhaps 500 or so at a time on each cron run.

Unless anyone has any serious concerns about 500K users in around an hour (and on a huge site this could always be done in advance and UPDATE()ed into the database during the upgrade), I think we are ready to go RTBC here (or should that be RTBTC?).

For reference, taking the $drupal_new_hash_iteration down to 5 (which, IIUC is 'half' as secure) increases the users/sec to 181 - or about 45 minutes for 500k users.

#221 looks good to me.

I'll let others in the community determine the default settings. I find those in #221 reasonable.

Grugnog2 - your testing suggests that there's probably no point in going below 6 for $drupal_new_hash_iteration - the relative overhead of database updates becomes too large for lower values.

Now what? Are we waiting till the patch no longer applies, so that it needs to be re-rolled, re-reviewed, and re-tested?.. ;-(

If the patch still applies, can someone go ahead and commit it, please, such that we can move on to other password security issues (to be discussed separately)? I don't even dare to approach those until we're done with this one, given how many comments this one has taken (I wish we committed #103 last year).

@solardiz - Dries was traveling for the last two weeks, and many of us were busy at Drupalcon this past week, so hopefully we'll get this soon now. The patch still applies cleanly, and I'll re-roll it if it doesn't.

Question: how could I reset password for user 1 direct through sql? Now I just do UPDATE users SET pass=md5('my_new_pass') WHERE uid=1. Is there a similar way to do that?

recidive: I haven't tried it, but see #215.

@recidive - the old way (which you have described) should still work. The password hash will be converted to phpass upon first login.

@keith.smith - no, the approach described on #215 will not work for the latest patch.

@recidive - we can roll a little CLI script in the /scripts directory to generate the hash.

ping?

at the urging of Dries, I reworked this again to try to further reduce this core implementation to a very bare minimum LOC that will need to be maintained.

The algorithm is essentially unchanged, but the hash formatting is altered, and the code uses the built-in base64_encode() rather than the custom encoding in phpass which lets us cut out one substantial function.

This solution does lose some of the advantages of using the framework more-or-less as-is (i.e. #221), but it should take fairly minimal effort to write a little code that will implement a base64 decode per the coding scheme in phpass so that we can readily interchange those hash with any such Drupal-style ones that use base64_encode().

Good work pwolanin.

I see that backward compatibility is still maintained (one can change the pass field in the users table to md5() and then the user will be updated when they login the next time).

Minor point: Rename system_update_7001 to system_update_7002. The reason is that there is another patch that uses that here http://drupal.org/node/235821, and has to go in because it is a followup for something we missed in another issue.

@kbahey - sorry but "one can change the pass field in the users table to md5()" is not correct for the more recent patches. However, it's not hard to set a single password via SQL from the hash of a known password.

I'm just fixing a couple minor indentation issues pointed out by the coder module. Other than that this looks like a solid patch.

a couple other notes - if we were to make the salt longer than 6 bytes, we could take phpass hashes and make them into hashes compatible with the code in this or the last patch, but the hashes generated with such a longer salt could not be converted back to the phpass format. Probably no good reason to do this if 6 bytes is a strong enough salt.

Also, I think we should make function _passwordhash_random_bytes($count) into a general API function like function drupal_random_bytes($count) that we could use in http://drupal.org/node/235821, in generating build IDs for forms, in generating the drupal private key, etc.

The attached patch makes such a function drupal_random_bytes($count) in common.inc, and also moves the update to user.install. Both changes on the suggestion/concurrence of kbahey and catch in IRC.

I re-tested the update path, etc, so I think this code is pretty good.

Some first comments:

* The PHPdoc at that top of the file should explain why this is better than using PHP's built-in functionality. Please include a motivation at the top of the file.

* Optionally, explain what a 'stretched hash' is.

* drupal_random_bytes() could use some more code comments, especially the part that is using the random block device and that uses getmypid(). It would be good to explain what happens when the fopen call fails on non *nix platforms. It would also mention why this function is better than the existing PHP functions/techniques.

* "This should increase by 1 at least every other Drupal version." -- please explain in the code why that is the case, even if it is slightly technical.

* "Subtract 5 to compare to bcrypt hash strength." can't make sense of that comment.

* _passwordhash_gensalt is not a very Drupal-ish name. We don't glue words together and we don't abbreviate words unnecessarily. I'd say, _password_generate_salt() or something a little bit nicer.

* For the same reason, I'd prefer to call this file password.inc or hash.inc, and not passwordhash.inc.

* The PHPdoc of _passwordhash_gensalt should explain in one or two sentences what a salt is and when/why to use it.

* The names of the functions in this file are not namespaced consistently. Some functions start with 'user_', other functions start with 'passwordhash'. It is not a big problem, and I might buy it, but it is inconsistent with the rest of Drupal.

* In the code comments of user_needs_new_hash(), I'd capture some of the history/motivation behind this function. It is fresh in our memory now but it will be not.

* In user_check_password() and user_needs_new_hash(), it is not clear why we pass in $account instead of $account->pass. Passing in $account->pass would make the function easier to reuse.

* The documentation of $iteration_count_log2 in user_hash_password() should probably what the expected values are. It defaults to NULL which is slightly confusing. It made me instantly wonder whether we need to pass in an integer or an object for $iteration_count_log2. To answer that question, you have to study the code. I'd try to answer that question in the documentation.

* I still have to think a bit more about an upgrade path and how to get rid of the legacy stuff. Unfortunately, forcing people to update their password sounds like a very intrusive thing to do. Then again, it could be a good feature to have.

@Dries - will look at the naming issues.

This upgrade path does NOT (at the moment) force people to change their password - minimum password length/strength is separate concern we may want to address (or not) in core.

The user_* vs. other function names are because I envision this as a small API. An alternate .inc file would still have to provide the user_* functions since they are called from user.module, but could have arbitrarily different function names for everything else.

ok, mucho code comments & renaming. Also retested.

Caution: pedantic review of comments follow.

+ * Returns a string of highly randomized bytes (over the full 8-bit range)

Add period.

+ * An alternative or custom version of this password hashing API my be

Replace "my" with "may".

+ * even if two users have the same password it will not be evident by examing the

"examining"

+ *   separated by ':', for exmple - $D:14:fF3nVpiud

"example"

+  // Maximum log2 iterations is DRUPAL_MAX_HASH_COUNT

Add period.

+ * By using a salt and repeated hashing the password is "stretched" - its
+ * security is increased because it becomes much more computationally costly
+ * for an attacker to try to break the hash by brute-force computation of the
+ * hashes of a large number of plain-text words or strings to find a match.

Erm. This is too much of something, here. Like maybe there are two sentences oddly joined together or something.

+ *   An existing hash or the output of _password_generate_salt()

+  // Hashes may be imported from elsewhere, so we allow != DRUPAL_HASH_COUNT

Add period.

+  // primitives always available in  PHP 5. To implement our own low-level crypto

Extra space between "in" and "PHP". Plus, "crypto"? Do you mean cryptography?

+ *   A plain-text password

Add period.

+ *   mass operations where a value less than the default is neeeded for speed.

Replace "neeeded" with "needed".

+ * password is available.  A new has is needed when the desired iteration count

Replace "has" with "hash".

+ * Alternative implementations of this function might use other critera based

Replace "critera" with "criteria".

+ * Increase the length of the password field to accomodate better hashes.

Replace "accomodate" with "accommodate".

+ * Also re-hashes all current passwords to improve security. This will take a
+ * while to run.

Replace "This will take a while to run." with "This may be a lengthy process." (or something similar).

+  // Lower than DRUPAL_HASH_COUNT to make the update run at a reasonable speed

+  // Multi-part update

+    //  Hash again all current hashed passwords

Add periods.

thanks! - I guess php -l doesn't catch those...

New patch and also a start on a hash conversion script for grugnog in case he wants to confirm that we can easily make these hashes usable with phpass's functions. Should something like this be added to /scripts?

and for moshe and kbahey - a CLI script to generate a hash - should something liek this go in /scripts?

This looks good to me - the function names and comments are definitely an improvement, and are clear to me.

There is an blank line after function _password_crypt($password, $setting) { that probably needs removing.

Also, it should be mentioned that there are 2 references in comments to user_update_7000 which should be updated if we need to increment the update number before commit.

removed stray blank line, reworked new script as a separate patch.