Check for common words in password strength indicators

I marked #1764034: Improve Password Strength Checker function as duplicate, but note that that issue has a patch posted which might be a good start here also.

Rather than using a grab bag for the list here I'd like to see it based on an authoritative list from a trusted source that examines passwords for common usage.

It might be a good idea to determine up front how many common passwords we would want to include here since we can't cover all of them.

Here's a patch which works off of a dictionary (the patch in my duplicate issue only matched "password") as per @webkenny's suggestion.

The patch also checks that the user hasn't used their email address as their password, also uses JS variables and variable_get so that this list can be changed. It's basic but better than hardcoding though I do agree translation needs to be considered.

After a quick Google I used passwords from this list, It's subjective what passwords to have in core and I'm not precious about this list. http://www.zdnet.com/the-top-10-passwords-from-the-yahoo-hack-is-yours-o...

The patch is for D8, once everyone's happy I can roll for D7.

We could implement an extra weakness check which is consecutive characters entered, though I think this needs to be in addition to the common passwords check. Of the top 25 passwords in this list, only 1 has consecutive digits - http://splashdata.com/splashid/worst-passwords/index.htm

With the consecutive check, 3 characters seems sensible, we need to strike a balance between allowing users to sign up without being alarmed but also alert them to an insecure password. Occurences of tripe letter sequences are somewhat rare: http://www.fun-with-words.com/word_consecutive_letters.html The words listed are kind of a stretch whereas the 2 consecutive ones are more common "food" for example.

I'd say a blacklist is the way to go, third party is out of the box but sounds like a bad idea for a tool which is meant to simply advise, and we can do that without a third party. It does feel a bit weird hard-coding in a list of passwords, but again it's meant to be a tool to warn users that what they're doing is a bad idea and cover off against the most useless passwords. If we have this list as a variable_get then we give devs the chance to alter it if they feel particularly strongly. On i18n I'm not sure what the best approach would be.

I'll look at coding a consecutive check and fixing that coding standards issue.

OK here we go, consecutive character checker for uses of a character 3 times or more, also switched out the blacklist to use the list originally refered to (first 12 items seem the most useful). Fixed the indentation issue in the patch.

Does anyone think we should push up the minimum suggested password length? Going to 8 from 6 could be a good move.

This approach needs to be agreed on (consecutive + blacklist), but am marking as "Needs review" to at least get the patch validated against tests.

Whoops, pulled down latest changes, re-tested. Fixed a JS issue in the install process (the email address field has a different name).

Running tests locally I get a fail on unrelated things like page caching. Grabbing a brand new copy of D8 and not modifying it these tests still fail... Reading the FAQs I can see reasons why, also reading through the code changes there isn't anything that should cause a fail :(

+++ b/core/modules/user/user.module
@@ -3055,6 +3058,20 @@ function user_form_process_password_confirm($element) {
       'confirmTitle' => t('Passwords match:'),
       'username' => (isset($user->name) ? $user->name : ''),
+      'weakPasswords' => variable_get('user_weak_passwords', array(
+      	'password',
+      	'123456',
+      	'12345678',
+      	'qwerty',
+      	'abc123',
+      	'monkey',
+      	'1234567',
+      	'letmein',
+      	'trustno1',
+      	'dragon',
+      	'baseball',
+      	'iloveyou',
+      )),

I don't know if the CMI patch for user module has already landed, but if so this might need updating.

@mrf good call, patch taking that into account, no more variable_get!

#17: user-improve-password-strength-checker-1497290-17.patch queued for re-testing.

Neat patch! One thing I noticed:

+++ b/core/modules/user/user.moduleundefined
@@ -3046,7 +3046,10 @@ function user_form_process_password_confirm($element) {
+      'passwordIsKnownAsWeak' => t('Use a different password, this password is known to be weak'),

The comma here is ungrammatical. A semicolon, longdash, or parentheses would be correct in this context. Edit: or separate sentences, or maybe we can even shorten or remove the second part of the string.

@xjm thanks for the review! I've replaced the comma in favour of a hyphen as recommended.

Added issue summary, alli you might want to take a look to make sure I didn't miss anything.

Reviewed the patch and haven't found any issues.

I strongly agree with mrf in comment 6 to have an official list (if any) or match on common patterns (like 123, abc etc.)

Initial issue summary

Updated issue summary with more ui changes.

Updated issue summary.

Updated issue summary.

@michaelmol thanks for the review, @mrf looks great! Thanks for doing that, I've tweaked it a bit.

Remaining points:
Documentation - what do we want to document?
Needs security team review - will try and get in touch with a member.
Needs consensus on the list of passwords - as I've said this list is somewhat arbitrary, wondering if there's a better list out there, I'll have a look around.

#20: user-improve-password-strength-checker-1497290-20.patch queued for re-testing.

It would be good to use something like https://tech.dropbox.com/2012/04/zxcvbn-realistic-password-strength-esti... (although this library is not GNU/GPL :(

I would say "Password is a dictionary word" instead of "Known to be weak"?

Updated issue summary.

Rather than trying to compile a manual list of words as in #23, I decided to integrate a really excellent password strength library from https://github.com/lowe/zxcvbn - there is a really excellent write up on this library found at https://tech.dropbox.com/2012/04/zxcvbn-realistic-password-strength-esti...

The library is MIT licensed, and I believe that this means it is compatible with GPL (so able to be included with Drupal core)

As far as I know this library is currently in use on:

• Wordpress 3.7+
• Dropbox.com

This is a screenshot of Drupal 8 before the patch applied, with a password of "password":

and the longer passphrase "correcthorsebatterystaple":

This is a screenshot of Drupal 8 after the patch applied, with a password of "password":

and the longer passphrase "correcthorsebatterystaple":

I have kept all the password hints etc exactly as they were, and the wording on the frontend is untouched (along with the colors etc). This patch is purely about the password strength measuring algorithm.

Keen to hear feedback on this.

Test appears to be an unrelated fail. Marking back as needs review (also needs manual testing).

+1 for patch in #28, tested against HEAD and works very nicely.

Patch #28 works for me, would be nice to have hints about why a password like "S0m3tH1ng_" is considered Fair. Something like "Is a derivative of a dictionary word" would be helpful.

Excellent idea to include the third party library. No sense having Drupal try to keep up with the latest and greatest recommendations.

Relating to the library integration, would it just be better for us to use the 0 through 4 directly instead of transposing back to our 0-100 system?

If we use the library I'd prefer to see it as the sole source of strength calculation.

+++ b/core/modules/user/user.js
@@ -94,80 +94,69 @@ Drupal.evaluatePasswordStrength = function (password, translate) {
+  // Based on the strength, work out what text should be shown by the password
+  // strength meter.
+  switch (result.score) {
+    case 0 :
+      indicatorText = translate.weak;
+      indicatorColor = '#bb5555';
+      strength = 20;
+      break;
+    case 1 :
+      indicatorText = translate.fair;
+      indicatorColor = '#bbbb55';
+      strength = 40;
+      break;
+    case 2 :
+      indicatorText = translate.good;
+      indicatorColor = '#4863a0';
+      strength = 60;
+      break;
+    case 3 :
+      strength = 80;
+    case 4 :
+      indicatorText = translate.strong;
+      indicatorColor = '#47c965';
+      break;

Updated the issue summary to reflect the patch with the inclusion of a new library.

OK, have implemented the suggestions by @mrf and @acbramley.

Changes since the last patch:

• strength variable removed, result.score used instead
• adds two additional strings that inform the user to add words and not base their password on a dictionary word

Thoughts?

+ 'core/assets/vendor/zxcvbn/zxcvbn.js' => array('group' => JS_LIBRARY, 'weight' => -20),

Does the library need an explicit weight?

   var hasLowercase = /[a-z]+/.test(password);
   var hasUppercase = /[A-Z]+/.test(password);
   var hasNumbers = /[0-9]+/.test(password);
   var hasPunctuation = /[^a-zA-Z0-9]+/.test(password);

Will the new library still care about all of these things?

If making these suggested changes has no effect on the perceived strength of the password, it could be confusing for the user.

If the library doesn't care about these recommendations, but they are still considered worthwhile, do we need to tweak the "strength" output to take them into account (like the old code did) ?

(Or if the modern consensus is that those recommendations don't improve password security to any appreciable extent, then we shouldn't be suggesting that people do it.)

+  // Add some items to the blacklist of available passwords.
+  var blacklist = [username, email];

The individual components of the email address (the name portion in particular) would also make sensible blacklist entries.

+ 'basedOnADictionaryWord' => t('Do not base the password on a dictionary word'),

Should this message make it clear that the "dictionary" includes the blacklisted items as well?

Something like "Do not base the password on a dictionary word, or your own account details." ?

Thanks for the review @jweowu.

Changes in this patch:

• removal of WEIGHT attribute from the library
• only show the helpful password hints if the password is not already 'Strong'
• tokenize the email address into name and host and add this to the blacklist of words
• add specific wording to inform users their password should not be their email address

I think informing users that they can add complexity by adding upper and lower and punctuation is still a good idea, as ultimately as long as their password gets longer, that is a good thing. What do other people think here?

40: 1497290-zxcvbn-password-strength-meter-40.patch queued for re-testing.

Patch straight re-rolled as it no longer applied.

43: 1497290-zxcvbn-password-strength-meter-43.patch queued for re-testing.

The new patch at #1497290-43: Check for common words in password strength indicators works as advertised but I did spot 2 items.

The e-mail username and domain parts of the e-mail address do trigger a flag but the messages don't make sense. The e-mail address was s0b1954c4699bbe9@simplytest.me and "s0b1954c4699bbe9" triggered the "Do not base the password on a dictionary word", "Add uppercase letters", and "Add numbers".

A "Do not use parts of your e-mail address" message would be good here.

If this isn't sorted in the next 12 hours I'll roll a new patch when I get to work tomorrow.

Have added to #1497290-43: Check for common words in password strength indicators. We now have more meaningful messages for the username/domain parts of e-mail addresses being used.

Tweaked the grammar in the "User interface changes" section to better reflect what it's doing. Also added email name@domain part checks.

Hey these changes look great. Happy to RTBC this, would really like some security eyes over this issue too.

From Bruce Schneier's latest Crypto-gram:

Crackers use different dictionaries: English words, names, foreign words, phonetic patterns and so on for roots; two digits, dates, single symbols and so on for appendages. They run the dictionaries with various capitalizations and common substitutions: "$" for "s", "@" for "a," "1" for "l" and so on. This guessing strategy quickly breaks about two-thirds of all passwords.

Modern password crackers combine different words from their dictionaries. From ArsTechnica:

It is important to tighten this up in D8.

A re-roll to address the changes in the yml files.

I'm not sure that the yml files are right, so would be good to have them verified.

user_library_info() & system_library_info() no longer seem to be used any more.

OK, due to #1996238: Replace hook_library_info() by *.libraries.yml file hook_libraries_info() is no longer a thing. I have tidied up a few things since comment #52

Changes:

• Pre-process set to 0 for zxcvbn (to stop this library being bundled as it is only used on some screens)
• Added the dependency to the correct place, and now it works again

This patch is from the Drupal8 code sprint in New Zealand! #Drupal8NZ

+1 (reviewed and works fine)

Reviewed using simplytest.me
Ran through the following test cases

Add user
user name: HeikeT
email: name@domain.co.nz

HeikeT > yields error >> Make it different from your username >> pass
name@domain.co.nz > yields error >> Make it different from your email address >> pass
name > yields error >> Make it different from the username part of email your address >> pass
domain.co.nz > yields error >> Make it different from the domain of your email address >> pass

Checked also that the library zxcvbn is only included on the user form >> pass

This test was performed as part of the Drupal8 code sprint(s) in New Zealand! #Drupal8NZ

I tried all the things HeikeT tried with different username / email address + some arbitrary english words like: letter, corner, password, table. It all works very well for me. The only thing, which bothers me a bit is that the zxcvbn library only scans for english words. There are no language packs available for zxcvbn at the moment, so I'm not sure what the road map would be here.

However, I understand, that this is another question and maybe out of scope of the issue here. So I confirm, that the patch works as intended, but I have some doubts about the road map making this multilingual.

Set to RTBC.

Now the data files in zxcvbn should be swap-able I would hope.

In looking at them though, it seems like the data files haven't been updated for 2 years:
https://github.com/dropbox/zxcvbn/tree/master/data

I opened a new issue here https://github.com/dropbox/zxcvbn/issues/40

Nice that dropbox has released this code, but not sure that they are actively maintaining it. Most of the code is 2 years old now.

abcdef123 should not be considered a fair password in Drupal 8. We have to alert folks that this is a stupid password.

Very hackable password without the patch:

With the patch:

What is with the "Add words." There is both a common word & a common name (Hello#@Mike1):

All examples from SimplyTest.me.

Removing "Needs manual testing" tag. Has been set to RTBC.

What processes are needed for the "needs security review" tag to be resolved?

My main concern would be this: "Nice that dropbox has released this code, but not sure that they are actively maintaining it. Most of the code is 2 years old now."

I know there are more companies who adopted this code (like Acquia) but considering the last commit to the library was 2 years ago, I wonder how much is the library itself tested for security issues.

There must be other options to consider....

EDIT: Their initial blog was interesting https://tech.dropbox.com/2012/04/zxcvbn-realistic-password-strength-esti...

I want to make sure the issues with the library do not make us lose sight of the fact that this two year old library is still far closer to solid recommendations about password strength than what is currently in core. I think it would be better to remove the current recommendation system altogether than stick with what we have in 7.

54: 1497290-zxcvbn-password-strength-meter-54.patch queued for re-testing.

While improving password security is an important goal and also the perfect is the enemy of good enough; are we seriously considering adopting a library which has the dictionary baked right into the JS? How does that mesh with the ideas of Drupal -- being international and extensible and whatnot? Or doing something is better than nothing? After all, WP 3.7 have adopted this library so we must too? I can't find another library which is surprising. I guess 3rd party checkers are out in core which is sad cos a good and very simple password strength checker API is available at https://www.google.com/accounts/RatePassword?Passwd=CorrectHorseBatteryS...

I have looked into this issue quite a bit on my own projects, and the key, of course, is balancing convenience and security. More online services (notably good banks and payment processors) are moving to two factor authentication. How feasible would this be for Drupal?

Granted, it's not a magic bullet, but it would offer a level of enhanced security with just a bit more user input, but not a burdensome amount.

@karolus Um, that's a new discussion for maybe in contrib - but you can be pretty sure that it's WAY off topic for this minor core patch.

Even this patch has gone way off the reservation - introducing new outside libraries?? PIE is all very well, but it's making this small UI improvement into a major change, and has stalled it.
If this is going to progress, we need to rationalize back and get 'good enough' in. While *maybe* leaving behind a hook that makes whatever is happening here swappable later.

Adding 690.27 KB that gets pushed to the client!? 8-0

I wouldn't agree that this can be categorized as as just a small UI improvement.

Right now Drupal is making bad password recommendations. 11111! is considered a good password, and I really don't think we should be making that kind of recommendation.

I think this issue got sidetracked by trying to get perfect recommendations. There were earlier attempts at small improvements in #454014: Include length in password strength evaluation and even the original incarnation of this issue https://drupal.org/node/1497290#comment-6567580.

I think getting length plus repeated characters covered and leaving out dictionary words since we don't have access to a good dictionary would at least get us closer to good-enough.

Another small improvement issue that is just checking for repeated characters for reference: #340043: Warn against repeating character classes in passwords

Adding 690.27 KB that gets pushed to the client!? 8-0

True, though this is only loaded on the user edit form and then cached by the browser. https://github.com/bjeavons/zxcvbn-php would allow us to do server-side password strength recommendation.

I guess 3rd party checkers are out in core which is sad

3rd party tools are a no-go, but https://github.com/bjeavons/zxcvbn-php could let us perform server-side checking.

are we seriously considering adopting a library which has the dictionary baked right into the JS? How does that mesh with the ideas of Drupal -- being international and extensible and whatnot? Or doing something is better than nothing?

It's no worse than what we have currently in core. You have a point, but I bet the majority of the brute-force attacks are performed using English dictionaries. @mrf said it well: this two year old library is still far closer to solid recommendations about password strength than what is currently in core.

Most of the code is 2 years old now.

https://github.com/dropbox/zxcvbn had a commit 17 days ago.

#54 still applies.

Dropbox said that they were going to be looking at this. Glad to hear that they did.

So any reason not to mark this RTBC?

2 hunks failed for core/core.libraries.yml & core/modules/user/user.js

Rerolled patch. Not completely sure about core/core.libraries.yml though. There is an attribute "gpl-compatible: true" for each entry now and I'm not sure if that applies for the Dropbox license here. As I was in doubt I put a "gpl-compatible: false" there. Otherwise the patch seems to work properly again.

Would have liked to add an interdiff.txt but interdiff fails. Not completely sure why.

Hey Stefan,

The library is MIT licensed, and I believe this makes it compatible with GPL. Can someone else clarify here?

The MIT license is GPLv2 compatible. Please see GPLv2 Compatible Licenses.

Changes to core/core.libraries.yml:

• Changed "gpl-compatible" to "true".
• Changed "name" from "Dropbox" to "MIT"

This patch applies nicely to the latest Drupal.

I've gone through this list of insecure passwords and it blocks all the ones I would expect - http://pastebin.com/Fdwn6XAF

Looks ready to apply to core.

I think we need to think about frontend performance here. Adding 600kb of js to download on the user register page and account edit screen needs a bit of thought.

Also this is failing our eslint javascript rules.

eslint .

core/modules/user/user.js
  110:17  error  'zxcvbn' is not defined  no-undef

✖ 1 problem

Not sure about this one. There's the 600kb and the fact that it's english only too. I guess it does check for a lot of common pass so why not. I'd prefer that we use the async load though.

As for the error the problem is that the lib define a global variable and we need to tell eslint that it's a legit global. The .eslintrc file needs to be updated with the lib variable.

Aren't there any PHP lib that checks passwords against a dictionary? an ajax call to verify the strength coud work too.

Small point, it is only 330k gzipped (not the full 600KB). Most servers would be doing this automatically in any case.

yeah but on things like mobile it'll still take a while to unzip and process.

As far as PHP libraries go bjeavons ported the zxcvbn js library to PHP.

https://github.com/bjeavons/zxcvbn-php/

There is also a sandbox module of his incorporating this library into Drupal: https://www.drupal.org/sandbox/coltrane/2231935

With @scor's help there's also this sandbox project https://www.drupal.org/sandbox/coltrane/2292875 which is using zxcvbn-php for server-side checks similar to password policy.

I believe that if there is to be validation done on the client side as well as validation done on the server side, then they damn well better be using exactly the same rule library or madness will ensue. False positives, false negatives and frustrations are easy to trigger otherwise, when the interactive UI says "OK" but the server says "no, not really".
For this reason, a server-library with a small web-service callback would seem to be the most robust to me. If zxcvbn is the best solution, it should be available in its PHP incarnation, so this sounds useful.

If we strip back the problem, Drupal already has a client-side chunk of code which makes recommendations as to password strength. This is only a recommendation and won't prevent a password from being used if the user really wants. Not only that but it's doing a bad job as it considers "password" acceptable.

If we're talking about library size and so on, how often it's maintained and all that do we want this in core or should it be pushed to contrib where people can make their own choices.

I'm inclined to say client-side library or contrib. Server side AJAX calls make no sense from a speed perspective and would be poor experience because of that, in my opinion.

Well, the first thing I've did testing on localhost was: choosing a ridicolous password: "password" that was ranked with a >50% long orange bar. I still feel it lolful just a bit more than loading 600kb of dictionary passwords.

Maybe having a restful ajax check would be more serius but as is in this situation, I suggest to remove this feature and move it to a module because we don't forbid lame passwords also.

This should be revisited at least for the 100 or 200 worst passwords, at a minimum. abc123 comes in way too high, people are making fun of this oversight, see https://twitter.com/pentestit/status/658580015865442305 - similar really, really weak ones should at lease be thrown on that short list of hard coded ones, even if no time to write a nice ajax callback against the library above. It will save everyone trouble if this can be improved on 8.0.x before release. Also zxcvbn 4.0.1 was committed two days ago so it seems fresh / active anyhow.

While we're on the subject of Passwords, as I was reading through this thread, a thought came to me. I came up through the IT world, working with Novell Netware. Among the features that it had, was controls over passwords. For example, the Admin could set a requirement that a user had to change their password every X number of days. The Admin could set that the password had to be at least X number of characters long, and also the option to require unique passwords. (In other words, the user couldn't just "change" their password from "1234" to "1234"). It also allowed the Admin to set a number for that. In other words, the new password had to be unique to the last X number of passwords. These features were options, so on my personal network at home, I didn't have to deal with that level of security, while of course on a company network, some or all of such features could be completely necessary.
Considering that Drupal is used in everything from personal sites, to WHITEHOUSE.GOV, I think that implementing such abilities into Drupal would be merited. I know currently, Drupal (and many other CMS's) don't have such functionality, and it is common for a user to create their account, and not change passwords ever again. It is also not uncommon for users to use the same password on everything they do, thus should their credentials on one system be compromised, it puts every other site they use at risk. While we cannot control whether or not a user uses the same PW on every system, by requiring them to change their password every so often (60 days? 90 days? 180 days?) we can at least strengthen the security of our own systems.

@BEGRAFX - this is an old thread, and your thoughts would deserve a new issue ...
If you weren't just looking for : https://www.drupal.org/project/password_policy

Re #100/101:
Indeed, #100 is out of scope, plus it is very questionable (and in fact questioned in current literature) whether it is a good idea to force users to change their password from time to time. While in theory a good idea, in reality it was found to annoy users so much they're modifying their passwords just minimally and tend to use less rather than more secure passwords. The last answer on this policy might not be given, however, until a positive effect is proven, I don't see that being included in Core. However feel free to file a new issue.

Now back to the original issue:
We need to take into account the valid points made by @nod_ in #87:

There's the 600kb and the fact that it's english only too. I guess it does check for a lot of common pass so why not. I'd prefer that we use the async load though.
[...]
Aren't there any PHP lib that checks passwords against a dictionary? an ajax call to verify the strength coud work too.

Indeed, downloading 600 kb (or the half if gzipped) might be acceptable if this was our final and complete solution to the problem. However it would only contain an English dictionary, not any other language dictionaries that might be highly relevant for international sites, whether multilingual or not.

On the other hand I want to point out that password hacking activity that doesn't use brute force only, primarily targets English language passwords. Even for a website targeted at Spanish or French audiences, the English dictionary remains relevant, it is just not sufficient.

Therefore we need to make the used dictionaries pluggable. And therefore we need to go for zxcvbn-php as a server-side PHP solution checked using an ajax call.

This is what Wikimedia seems to be implementing as well, so has recently started a security review. In this context, it is interesting that they opted for the original bjeavons/zxcvbn-php rather than one of its forks, such as mkopinsky/zxcvbn-php, one commenter stating:

If we have any plans of running zxcvbn in Javascript we should go with the mkopinsky version, but we are unlikely to run it client-side because it would involve downloading large dictionaries, and a standalone node.js login service is science fiction at this point, so I don't think JS compatibility is something we need to be concerned about. Other than that, they both seem stable, not very active, both have some password quality issues and neither seems obviously superior to the other... the bjeavons version is more widely used which maybe means more eyes on security issues (which is what we are really concerned about here) so if I had to choose I'd go with that.

Note that I listed some more or less current literature on this topic on #1824800-110: Require a (configurable) minimum password length for user accounts. From that literature it becomes clear that zxcvbn is by far the easiest adoptable of all decent approaches, and has in fact been adopted by a large and growing number of relevant websites. So we're still on the right track, we just have to figure out how to implement it best.

There is some research going on whether the sheer presence of a strength meter raises awareness with users, so they pick better passwords, no matter whether the password meter is precise or not. On the other hand, it is also clear that we neither want to mis-educate our users nor show any weakness in regard to whitewashing blatantly insecure passwords such as "password1234" or "Pa$$w0rd!!!!".

So I definitely think we need to do our best shipping Core with either a state-of-the-art, at least industry-standard password strength meter, or with none at all. Therefore this is at least a task, if not major.

Exactly which version of https://github.com/dropbox/zxcvbn is used in the #83 patch?