[10.x] Fixes Str::password() does not always generate password with numbers

[crynobone]

fixes #48677

If we rely on Str::password() to generate a secure password and use the same logic for password validation, the provided password will fail if it's expected to contain symbols but doesn't have one because Str::password() MAY contain symbol instead of MUST contain symbol.

The expected behavior should be Str::password(letters: true, numbers: true) should indicate that the password MUST contain letters and numbers.

[mpyw]

It seems to be sufficient to determine if a value is true or not:

Suggested change

	$options = (new Collection([
	'letters' => $letters === true ? [
	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
	'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
	'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
	'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
	'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
	] : null,
	'numbers' => $numbers === true ? [
	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
	] : null,
	'symbols' => $symbols === true ? [
	'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
	'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
	']', '|', ':', ';',
	] : null,
	'spaces' => $spaces === true ? [' '] : null,
	$options = (new Collection([
	'letters' => $letters ? [
	'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k',
	'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v',
	'w', 'x', 'y', 'z', 'A', 'B', 'C', 'D', 'E', 'F', 'G',
	'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R',
	'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
	] : null,
	'numbers' => $numbers ? [
	'0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
	] : null,
	'symbols' => $symbols ? [
	'~', '!', '#', '$', '%', '^', '&', '*', '(', ')', '-',
	'_', '.', ',', '<', '>', '?', '/', '\\', '{', '}', '[',
	']', '|', ':', ';',
	] : null,
	'spaces' => $spaces ? [' '] : null,

Since there is no native PHP type declaration, it is safer to treat all truthy values as true.

[crynobone]

but the parameter isn't typehinted as bool

[mpyw]

but the parameter isn't typehinted as bool

Yes, I rather think so, which is why === true should be omitted. For example, if you pass (int)1 here, I think it should be handled as true because it is a truthy value.

In a situation where either true or false is determined, it seems somewhat unnatural that all truthy values other than true would be false. It would be also consistent in the case of adopting a native PHP type constraint, where 1 would be cast to true at the start of function execution.

---

If we move to using native type constraints somewhere in the roadmap, using === true at this stage would be a breaking change.

https://3v4l.org/buI6E

[crynobone]

framework/src/Illuminate/Support/Str.php

Line 842 in 09137f5

public static function password($length = 32, $letters = true, $numbers = true, $symbols = true, $spaces = false)

What's your expectation with without === true and the following example:

Str::password(numbers: 'nah');

[mpyw]

Str::password(numbers: 'nah'); should be Str::password(numbers: true); because PHP treats "nah" as truthy. The important thing is that we should follow the interpretation of the PHP standard, not determine "what should be true" at the the framework.

You would be correct if Laravel were to adopt the rule of always treating truthy values other than true as false, but it is not actually happened so far. I don't think there is a lot of code that judges === true for something received with @param bool $arg, so I think that needs to be consistent throughout the framework.

[crynobone]

You would be correct if Laravel were to adopt the rule of always treating truthy values other than true as false, but it is not actually happened so far

I would disagree https://github.com/search?q=repo%3Alaravel%2Fframework%20%3D%3D%3D%20true&type=code

[mpyw]

@crynobone I see, that is certainly the case in some codes. But apparently it is not consistent across the board. Somewhat confused.

• https://github.com/search?q=repo%3Alaravel%2Fframework+path%3A*.php+%2F%5C%24%5Cw%2B+%5C%3F%2F+AND+%2F%40param%5Cs%2Bbool%5Cs%2B%2F&type=code
• https://github.com/search?q=repo%3Alaravel%2Fframework+path%3A*.php+%2Fif+%5C%28%5C%24%5Cw%2B%5C%29%2F+AND+%2F%40param%5Cs%2Bbool%5Cs%2B%2F&type=code

For the moment, I see that there seems to be no need to discuss this excessively here. I think this has been an off-topic discussion, but thank you for your patience.

[valorin]

I wish I'd seen this before it was merged, there are a couple of issues I can see with it:

Less Secure: The shuffle() function is not cryptographically secure and using it within a password generation algorithm means the output should not be considered cryptographically secure. Since the whole point of this function is to generate cryptographically secure passwords, this is a significant issue.

The shuffle() needs to be replaced with a secure algorithm.

On a side note, replacing those helper methods with proper cryptographically secure implementations is on my list to do before v11. You can see my last attempt that I got in just too late for v10 at: #46105

Length Bug: It cannot generate a password shorter than 3 characters:

Str::password(1) => "&1C"
Str::password(2) => ";B9"
Str::password(3) => "9l."
Str::password(4) => "Io5\"
Str::password(5) => "*al74"

Less Entropy: This is a tricky one, but you've lowered the entropy of this function.

Using the calculator at https://www.omnicalculator.com/statistics/password-combination to check possible combinations for 8 character passwords. There are a total of 88 characters, based on 52 letters, 10 numbers, and 26 symbols.

With the original algorithm, it tells us there are 3,378,005,142,470,400 possible passwords.
The new algorithm, which requires a letter, number, and symbol, there are only 1,739,426,494,740,480 possible passwords.

Side by side:

3,378,005,142,470,400 (original)
1,739,426,494,740,480 (new)

The change is incredibly significant, as you're effectively limiting the full 88 character set to 5 of the characters, and the remaining 3 have a much smaller set - especially the numbers.

So based on the raw numbers, it's far less secure. However, there is definitely the argument to be made that if the generated password is missing a number or symbol, it could be easier to crack and implies a simpler password scheme in general. Plus, there are the issues you've found with not having the required characters - which can cause users to pick rubbish passwords instead.

So I think it's ok like this, but I wanted to raise it as something to be aware of. Ultimately you're still generating a very random password, which is going to be better than anything a human would produce.

Oh and one more thing I noticed: it doesn't enforce uppercase, so although you'll hit the required number and special character, you may find passwords missing uppercase letters. Not sure if this is an issue or not though - it depends on the problem you're trying to solve. I just mention it because uppercase is usually separated out from lower as a toggle too.

[vlakoff]

The real, underlying issue is that "can contain" and "must contain" should be separately configurable.

But if you want to limit the amount of parameters to keep the helper simple, I think the configurability should be reverted to "can contain".

[daliendev]

The real, underlying issue is that "can contain" and "must contain" should be separately configurable.

But if you want to limit the amount of parameters to keep the helper simple, I think the configurability should be reverted to "can contain".

When I worked on something similar a few months ago I suggested an $enforce param to cover that and don't change the actual behavior.

#48142

[lk77]

Does providing a seed to the shuffle function makes it more secure ?

Something like :

return $password->merge($options->pipe(
            fn ($c) => Collection::times($length, fn () => $c[random_int(0, $c->count() - 1)])
        ))->shuffle(random_int(-PHP_INT_MAX, PHP_INT_MAX)))->implode('');

but i don't know if it adds anything to the security of the shuffle

[valorin]

My understanding is that shuffle() uses the same predictable randomisation that rand(), etc, use. Therefore, even if you're starting with a proper random seed, the order of the shuffle may expose the seed and allow the complete order to be predicted.

So I'd say it's still not secure.

[talkinnl]

Using the calculator at https://www.omnicalculator.com/statistics/password-combination to check possible combinations for 8 character passwords. There are a total of 88 characters, based on 52 letters, 10 numbers, and 26 symbols.

With the original algorithm, it tells us there are 3,378,005,142,470,400 possible passwords. The new algorithm, which requires a letter, number, and symbol, there are only 1,739,426,494,740,480 possible passwords.

You're right of course, but changing the password length with just a single character has waaaay more impact. Input a range of lengths in the linked calculator and that becomes very obvious.
When generating random passwords and using password managers, we shouldn't talk about length=8 anymore.

You already know this, and the default length already is 32 chars, but I think it needs repeating the length=8 isn't best practice nowadays.

[talkinnl]

My understanding is that shuffle() uses the same predictable randomisation that rand(), etc, use. Therefore, even if you're starting with a proper random seed, the order of the shuffle may expose the seed and allow the complete order to be predicted.

So I'd say it's still not secure.

The master branch requires PHP 8.2 and could switch to Random\Randomizer::shuffleArray()

The introduction of random_int() and random_bytes() was already great for removing the old-school, tainted, rand() family of functions. So maybe it's time to plan to let go of shuffle() too. Or at least in secure contexts, but I'm not sure if the micro-optimization is worth it anymore.