Problem/Motivation

Password cracking tools contain lists of commonly used passwords, we should warn users that some passwords are too weak to be of use. Motivation is that currently the password "password" is ranked as fair by the checker tool.

Proposed resolution

This patch integrates Drupal with a third party (MIT licensed) library for password strength checking. The library is zxcvbn.

Remaining tasks

  • Needs security team review
  • Needs signoff from someone to assert the inclusion of the new JS library into core - 680k (320k gzipped)

User interface changes

Password strength meter will reflect a better approximation of how long it would take to brute force the password, e.g. the following things will be checked:

  • English words, with a frequency list skewed toward American usage and spelling
  • Names and surnames, coming from the US census
  • A few common keyboard layout based passwords (eg QWERTY)
  • If the user's email address is used
  • If the user's email address name part is used
  • If the user's email address domain part is used
  • If the user's username is used

API changes

n/a

Original report by webkenny

So while at the code sprint today I noticed when you type the word, "password", as your password it marks that as "Fair" - Luckily I happened to be sitting with Jakub and greggles was in earshot so we thought maybe based on this report to the security team (See http://drupal.org/node/454014#comment-5743806), it might be worth checking for a list of common words.

Files: 
CommentFileSizeAuthor
#83 interdiff.txt399 bytesStefan Lehmann
#83 1497290-zxcvbn-password-strength-meter-83.patch690.43 KBStefan Lehmann
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,572 pass(es). View
#78 1497290-zxcvbn-password-strength-meter-78.patch690.43 KBStefan Lehmann
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,982 pass(es). View
#61 HelloMike1-withpatch.png33.68 KBmgifford
#60 Hello#@Mike1-withpatch.png33.68 KBmgifford
#60 abcdef123-withpatch.png35.52 KBmgifford
#60 abcdef123.png32.75 KBmgifford
#54 interdiff.txt887 byteswiifm
#54 1497290-zxcvbn-password-strength-meter-54.patch690.27 KBwiifm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1497290-zxcvbn-password-strength-meter-54.patch. Unable to apply patch. See the log in the details link for more information. View
#28 before-short.png13.05 KBwiifm
#28 before-long.png12.65 KBwiifm
#28 after-short.png13.43 KBwiifm
#28 after-long.png9.2 KBwiifm

Comments

webkenny’s picture

Title: Check for common words in password strength indicator on install » Check for common words in password strength indicators

Bad title, removing the install.

webkenny’s picture

Status: Active » Needs work

Ok, this is one of those things that when I started coding it I was like: "Hm, there has to be a better way"

  // Check if password contains common words.
  common_words = new Array('password', 'qwerty', 'monkey', 'letmein', 'trustno1');
  if ($().inArray(password, common_words)) {
    // Passwords with common words are always very weak.
    strength = 5;
  }

It somehow doesn't feel right that we'd be hardcoding words in which won't sustain as times change, etc. Not only that but it makes broad assumptions about the person's base language. Password might be irrelevant in other languages.

Thoughts?

webkenny’s picture

Version: 7.x-dev » 8.x-dev

While I'm here, I know better. Marking this into 8.x --

David_Rothstein’s picture

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.

David_Rothstein’s picture

Component: user system » user.module
mrf’s picture

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.

heylookalive’s picture

FileSize
2.14 KB
PASSED: [[SimpleTest]]: [MySQL] 40,548 pass(es). View

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.

heylookalive’s picture

Status: Needs work » Needs review
webkenny’s picture

Status: Needs review » Needs work

I wonder if we can do better with regular expressions here. e.g. We can probably check numbers, consecutive sequences of letters, etc. using some regular expressions and, at the same time, improve our ability to check common words. While many passwords, even those by non-English speakers, tend to be English and that's not a concern - I wonder if we could somehow make this more dynamic.

I suppose a call to a 3rd party service is out of the question so what are some other creative ways to make this more adaptable?

FWIW, the patch itself looks good. There's only one small formatting nit with the first entry of the common words array. e.g. The space in front of the first set of numbers.

heylookalive’s picture

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.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
FAILED: [[SimpleTest]]: [MySQL] 41,237 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, user-improve-password-strength-checker-1497290-11.patch, failed testing.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
FAILED: [[SimpleTest]]: [MySQL] 41,249 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, user-improve-password-strength-checker-1497290-13.patch, failed testing.

heylookalive’s picture

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

mrf’s picture

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

heylookalive’s picture

Status: Needs work » Needs review
FileSize
3.69 KB
PASSED: [[SimpleTest]]: [MySQL] 42,087 pass(es). View

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

heylookalive’s picture

xjm’s picture

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.

heylookalive’s picture

FileSize
3.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user-improve-password-strength-checker-1497290-20.patch. Unable to apply patch. See the log in the details link for more information. View

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

mrf’s picture

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

michaelmol’s picture

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

michaelmol’s picture

Issue summary: View changes

Initial issue summary

heylookalive’s picture

Issue summary: View changes

Updated issue summary with more ui changes.

heylookalive’s picture

Issue summary: View changes

Updated issue summary.

heylookalive’s picture

Issue summary: View changes

Updated issue summary.

heylookalive’s picture

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

webkenny’s picture

Issue tags: +needs security review

Reviewed on my end as well. Looks good. Marking as needs security review.

heylookalive’s picture

Status: Needs review » Needs work
Issue tags: +needs security review

The last submitted patch, user-improve-password-strength-checker-1497290-20.patch, failed testing.

meba’s picture

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

meba’s picture

Issue summary: View changes

Updated issue summary.

wiifm’s picture

Assigned: webkenny » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.2 KB
13.43 KB
12.65 KB
13.05 KB
687.36 KB
FAILED: [[SimpleTest]]: [MySQL] 59,844 pass(es), 1 fail(s), and 0 exception(s). View

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

password of 'password'

and the longer passphrase "correcthorsebatterystaple":

password of 'correcthorsebatterystaple'

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

password of 'password'

and the longer passphrase "correcthorsebatterystaple":

password of '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.

Status: Needs review » Needs work

The last submitted patch, 28: 1497290-zxcvbn-password-strength-meter-28.patch, failed testing.

wiifm’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

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

sambonner’s picture

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

sambonner’s picture

Status: Needs review » Reviewed & tested by the community
acbramley’s picture

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.

mrf’s picture

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;
mrf’s picture

wiifm’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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

wiifm’s picture

Issue summary: View changes
wiifm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.43 KB
689.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,862 pass(es). View

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?

jweowu’s picture

+ '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." ?

wiifm’s picture

FileSize
3.71 KB
689.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1497290-zxcvbn-password-strength-meter-40.patch. Unable to apply patch. See the log in the details link for more information. View

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?

Gold’s picture

Status: Needs review » Needs work

The last submitted patch, 40: 1497290-zxcvbn-password-strength-meter-40.patch, failed testing.

wiifm’s picture

Status: Needs work » Needs review
FileSize
689.88 KB
PASSED: [[SimpleTest]]: [MySQL] 64,444 pass(es). View

Patch straight re-rolled as it no longer applied.

Status: Needs review » Needs work

The last submitted patch, 43: 1497290-zxcvbn-password-strength-meter-43.patch, failed testing.

wiifm’s picture

Status: Needs work » Needs review
Gold’s picture

Status: Needs review » Needs work

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.

Gold’s picture

Status: Needs work » Needs review
FileSize
690.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1497290-zxcvbn-password-strength-meter-47.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Gold’s picture

Issue summary: View changes

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

wiifm’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 1497290-zxcvbn-password-strength-meter-47.patch, failed testing.

mgifford’s picture

Issue tags: +security

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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
690.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,288 pass(es), 19 fail(s), and 1,328 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 52: 1497290-zxcvbn-password-strength-meter-52.patch, failed testing.

wiifm’s picture

Status: Needs work » Needs review
FileSize
690.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1497290-zxcvbn-password-strength-meter-54.patch. Unable to apply patch. See the log in the details link for more information. View
887 bytes

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
wiifm’s picture

Issue tags: +#Drupal8nz

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

HeikeT’s picture

+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

Stefan Lehmann’s picture

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.

Stefan Lehmann’s picture

Status: Needs review » Reviewed & tested by the community

Set to RTBC.

mgifford’s picture

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.

mgifford’s picture

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:

 abcdef123 no patch

With the patch:

 abcdef123 with patch

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

 Hello#Mike1 with patch

All examples from SimplyTest.me.

mgifford’s picture

FileSize
33.68 KB
Gold’s picture

Issue tags: -Needs manual testing

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

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: 1497290-zxcvbn-password-strength-meter-54.patch, failed testing.

meba’s picture

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.

mgifford’s picture

There must be other options to consider....

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

mrf’s picture

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.

dman’s picture

chx’s picture

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

karolus’s picture

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.

dman’s picture

@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

mrf’s picture

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.

mrf’s picture

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

scor’s picture

Status: Needs work » Needs review
Issue tags: +Security improvements

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.

mgifford’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 54: 1497290-zxcvbn-password-strength-meter-54.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll

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

Stefan Lehmann’s picture

FileSize
690.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,982 pass(es). View

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.

Stefan Lehmann’s picture

Status: Needs work » Needs review
Stefan Lehmann’s picture

Issue tags: -Needs reroll
wiifm’s picture

Hey Stefan,

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

geerlingguy’s picture

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

Stefan Lehmann’s picture

FileSize
690.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,572 pass(es). View
399 bytes

Changes to core/core.libraries.yml:

  • Changed "gpl-compatible" to "true".
  • Changed "name" from "Dropbox" to "MIT"
superspring’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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.

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript

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
nod_’s picture

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.

wiifm’s picture

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

nod_’s picture

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

mrf’s picture

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

coltrane’s picture

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.

dman’s picture

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.

heylookalive’s picture

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.

tassoman’s picture

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.

HongPong’s picture

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.

droplet’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

BEGRAFX’s picture

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.

dman’s picture

@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