Update genpass_password to include best elements of Drupal 7 user_password

The code comments on the genpass function say 'Based on the original D6 user_password function (with more characters)' so it seems the original intention was to user more characters so it would be more secure.

The current comparison to user_password shows that genpass has a shorter default length of 8 vs. 10 (less secure) uses mt_rand (less secure) but also has a longer set of allowable characters (more secure, but also can cause some support problems if people confuse lower case L, capital I and the number one - l, I and 1)

The Drupal 7 user_password function is a bit different, so I think this is an important thing to update so that genpass_password is at least as strong as Drupal 7's function. I've updated the issue to be about that.

Password generation has been updated for both supported branches by the commits for sa-contrib-2018-042 #2982180: Address sa-contrib-2018-042 in the 8.x branch.

The 7.x-1.x branch code is now effectively based off the D7 user_password function. The 7.x-2.x branch is basically the same.

The 8.x-1.x branch code is now effectively based off the D8 user_password function.

All that remains is the comment incorrectly stating that the function is based off the D6 version. It was removed from the D7 version with commit adb924c335f1c494f9c4e53ba4e24a15164c33a5.

Attached is patch to remove it from 8.x-1.x branch. This will likely fail testing for some unknown reason, but it should be ready to go.

The 7.x-2.x branch is basically the same, but a bit stronger to help satisfy password checking. For example, the 7.x-1.x branch might randomly choose all letters some of the time, which doesn't satisfy a password strength requirement that requires at least 1 number and special character. The 7.x-2.x branch will always include a number, lower case letter, upper case letter, and special character.

I think the 7.x-2.x strategy is the better one to copy for 8.x. What do you think?

Updating title to reflect what I see as the goal.

Yup, it makes sense to use an improved password generation method for the D8 release.

I have since spent far more time looking into this than I probably should have. Random numbers and cryptography are always full of gotchas, and I ran into a pile of 'em. I've ended up re-writing the summary to include a lot more information.

I've had a look at the 7.x-2.x method of generating a password and it looks pretty good in that it guarantees to use characters from all four sets.

It does have an issue in that the first 4 characters have reduced entropy since they each is guaranteed to always use a more limited set of characters in the same order for every password. It's still quite a random password, but it's not as random a password where every character could be any one character. I've addressed this by limitation by adding a random sort (using shuffle) just prior to returning the new password. It is safe to use shuffle here because the source is already random. The increase in length does vastly over-compensate for any reduction.

The length being param + 4 is also unexpected unless someone is being really careful about reading the docs or the code. The user_password function this is meant to be compatible with (as in 8.x-1.x version where it assumes a hook of hook_password) does return a password where $length = returned length. Min length ends up being 4 no matter what, which is again unexpected. Can't win them all.

While testing the randomness of the resulting passwords, I found that the simplistic password rules of always ensuring there's a character from each set resulting in a much higher probability of a digit being included in the password. It ends up not being a truly random password, but one with a bias. With a sufficiently large length of password, this can be made insignificant. As such, the minimum length of password should probably be increased to this extent. Doing so then vastly reduces the chances of a password not including a character from each set in the first place and making a forced inclusion less needed.

The coded Character Sets removes the need for the "Password Entropy" setting. It might be worth still allowing that to be configurable (via code) if an advanced user really knew what they were doing and wanted less secure passwords. It has been an option since the module's inception which is the reason I'd have thought it might be kept as an option. Who know how many people want poor passwords? :P It's certainly easier to just not given people the option to shoot themselves in the foot. They can patch the code if they're that keen.

So, not attached is a re-working of the D8 genpass_password function which keeps track of character sets usage and if the password fails to include one of the required sets, it just re-tries. It doesn't bother to check passwords that are too short (length <= number of character sets) as this would obviously fail. Alas, after testing a few tens of millions of passwords, I found this to end up spending quite a large amount of time amount of time generating passwords all the way up to around 20 char length (maxed out at 97 attempts on a 5 char password). Even with balancing the character sets to make it equally likely to come from any of them didn't improve performance enough. That idea is dead.

Patch attached is now your method from 7.x-2.x, with a random shuffle and a change to choosing index. This is the method used by KeePassX, which does a very good job. Tested on 20 million passwords which ended up being nicely random. This seems to fulfil what is needed for a truly random password.

Should probably port the shuffle back to 7.x-2.x.

Also ended up changing the characters included in the 'special' group to all of those from the original. I don't think that should cause issues. There was also a double up of the / character. Old is first line, new is 2nd line.
@#$%^&()=/|[]{};<>/
@#$%^&()=/|[]{};<>\!*+,-.:?~
I haven't tested to see if any of those will cause issues other than typing them all in as a password and confirming I can log in with it.

This patch does also diverge from what Drupal Core is doing in user_password() with the looping on $index = ord(Crypt::randomBytes(1));. With the core's source char string there this only returns a valid index around 1/5 of the time: 57÷256=0.2226. This looping is done to avoid modulo bias of cutting a random number down to fit a smaller range where [rand_max] mod [range] != 0. A better method is now available in random_int($min, $max)[1]; this is cryptographically secure and modulo bias safe, returning an int from $min to $max inclusive. It can deal with any size of source characters which is useful if we allow people to change the size of source chars arrays. This function is available from PHP7 and also has a back-ported version to PHP 5.x in paragonie/random_compat[2] which is included in my default drupal projects (is it in everyone's?). Failing that, a locally implemented version could be included.
[1] https://www.php.net/manual/en/function.random-int.php
[2] https://github.com/paragonie/random_compat
(Further review may be needed; paragonie's random_int just uses range looping too instead of bias compensation which is faster.)

In an ideal world, support for random_int would be added to Drupal\Component\Utility\Crypt. The call to randomBytes is already a wrapper around the newer PHP random functions.

Still @todo

• Remove UI option to set source characters
• Add hook for altering the source character_sets.
• Update to remove defunct setting.
• Wrap random_int to deal with older PHP versions.
• Document hook_password in api file if not present elsewhere

Wow, lots of very cool and interesting work! Thanks for the code and explanations.

It ends up not being a truly random password, but one with a bias. With a sufficiently large length of password, this can be made insignificant.

That hadn't really occurred to me and it's a great point! I do think that the extra variety of characters offsets this a bit, but I agree that shuffling the characters of the resulting string is an even better solution.

One area I'm not sure about:

This patch does also diverge from what Drupal Core is doing in user_password()

I'm hesitant to deviate from what core does. I think it would be good to get those changes into core first and then bring them over here. Core gets way more reviews (both as part of committing and from security researchers) so it would be good for this module to "do what core does" to choose a random string to benefit from that research.

Agreed about random index generation matching core. Alas I don't have the drive to go push a better solution into core. It's not called constantly so a few more loops while picking a random number inside the index isn't harming anything. I'm sure a modulo bias safe and fast random_int would be quite useful to be added to core.

Attached is the version that essentially just does the following

• shuffling password
• increase chars in the special character set
• moves $length admin setting into genpass_generate() so it is used for all modules implementing hook_password

Still @todo everything in the summary.

Added to summary:
- Need to throw an exception if altered character_sets (from hook_genpass_character_sets_alter) has a single set length or combined length which exceeds 256 characters as this is outside the bounds of a single random byte for the random index. random_int call avoided this problem as it calculated how many random bytes it needed based on the range.

Agreed on the elements
Thanks for all your work

Could we support elements from Password Policy if we want to have them both in the same site.

To ignore password policy rules when we generate, but let it take over after that
or obey its rules when we generate

The current status, we could have number of right generated passwords, but some time the form want save as it did not pass the password policy.

@RajabNatshah have you tried the logic in this patch? It should help to always pass basically any password policy since it's longer and has more variation in the kinds of characters that are included.

Thank you Greg for following up

We had a random failed cases 1 in 10 when we
Issue #3060823: Change Automated Functional Acceptance Testing to work with changes after enabling [Generate Password] module in the 8.x-7.x branch
https://git.drupalcode.org/project/varbase/commit/c1b959f

The robot test will have issues
https://travis-ci.org/Vardot/varbase/builds/549324872

Then we had to go back to
Issue #3060823: Revert [ Feature: Create default testing users ] Automated Functional Acceptance Testing to the old normal way
https://git.drupalcode.org/project/varbase/commit/518abea
Then it passed https://travis-ci.org/Vardot/varbase/builds/553735926

Sure we should test with the new patch
to full test we could run about 10 to 20 times to make sure it will not fail in random cases

I hope that we could move foreword with this issue
#10

Been a bit silent on this project sorry - I now have a little sprog who has taken over my life.

It appears that more recent discussion in here has diverged off just fixing up password generation to possible integration with password policy module. I think this should be forked off into a separate issue as that's quite a bit of unrelated code. There is actually already an issue opened on that front: #1964974: Integrate with Password Policy module so the generated password meets policy requirement

Attached is a patch to do all of the remaining tasks on this issue in addition to path #7.

- Remove genpass_entropy ui, test and config elements
- hook_update_N to remove the configuration item from existing installs
- Add genpass.api.php describing hook_password and hook_genpass_character_sets_alter
- Move settings into their own details fieldset to separate them out from "registration and cancellation"
- Implement genpass_character_sets alter to allow other modules to change the character_sets if they're really really keen
- Throw InvalidCharacterSetsException when resulting character_set doesn't quite make it

Provided this works for people, I think this fix is complete.

Neglected to also sanity check the maximum length of altered character sets. The length cannot exceed the possible range of random indexes provided by Crypt::randomBytes(1).

Thanks for your continued work on this, ELC. And congratulations on the sprog :)

Here are some minor bits of feedback and questions. I'd be happy to reroll with these changes included or have you do that. Just say the word if you'd like me to do it.

In InvalidCharacterSetsException.php:

 * @package Drupal\mlm_structure

The package seems like some copy/paste that could be removed.

In genpass.module:

        '#title' => t('Genpass Configuration'),

I think moving this to its own fieldset makes sense. I'm not sure the word Genpass is used in user-facing text, much though. On the modules page the name of the module is shown as "Generate Password" so maybe "Generate Password Configuration" though that sounds pretty awkward. What do you think about calling this "Password Generation Configuration" or something similar to that?

    $index = ord(Crypt::randomBytes(1));

It seems Crypt::randomBytes is deprecated and for a module that runs only on php7 we should use random_bytes(). What do you think?

I also noticed that the core user_password function is now a bit different:

  $pass = '';

  for ($i = 0; $i < $length; $i++) {
    $pass .= $allowed_characters[random_int(0, $max)];
  }

We could move to that style of generation to "do what core does" and also remove a deprecated function.

OK, here's an updated patch with the changes I suggested in #14 and I made 3 other adjustments:

* Using random_int means that the length of the character sets can't (reasonably) exceed the max int returned, so we can stop throwing that exception.
* I updated the test to use BrowserTestBase as we aren't testing any javascript and it's easier and faster to run BrowserTestBase
* In running the tests I noticed some deprecation issues related to the theme so I fixed those as well.

I think this could be ready to commit and then we'd be ready for the 8.x-1.0 release!

Let me know what you think about these changes.

Thanks for re-rolling it and fixing it up.

Ironically, the call to random_int was what I was talking about in #5. Core is using the pretty much exactly the code I wrote. I'd patched it in and then removed it after #6 :P It's good to see that core has updated to use this crypto secure function, and good that we're using it here now. Heck, we could even update the subject.

The length exception is now quite irrelevant so good riddance. It was an ugly hack to deal with the lack of random_int in a weird way anyway.

I did want to call the settings group something less crap. The developer in me got in the way and wanted people to know the settings were from a particular module. The front end user in me wants to know what the settings are actually for. So yes, the name of the fieldgroup is much better.

Looks good to me.

Thanks for the feedback and review.

I totally get how it probably feels a bit circular for us to have arrived back at random_int - you were right before, clearly. I'm just always hesitant for a module to make a move faster than core given how many more people review core changes than they do in a random contrib.

Committed/pushed now.

I also created a new release and will likely make an 8.x-1.0 in the next few weeks.