[11.x] Add default max length for Password rule object

[angelej]

Set the default max length for the Password rule object to 72 chars, see #49739

[ziadoz]

Is the use of mb_strlen() here going to cause problems with passwords that contain multi-byte strings?

> $strmb = str_repeat('∆', 24); // 72 bytes
= "∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆∆"

> mb_strlen($strmb); // *characters* in string
= 24

> strlen($strmb); // *bytes* in string
= 72

[taylorotwell]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

[valorin]

Is the use of mb_strlen() here going to cause problems with passwords that contain multi-byte strings?

Oh, that's a good point - should fix it in 10.x since #49739 was merged.

@angelej, are you able to PR a fix for that?

[angelej]

Thanks for your comments.
Of course, I can change it if this is the expected behavior.
However, this also affects the Password::min(8) rule and workarounds like Password::min(8)->rules('max:80).
The official laravel docs refer to chars and not bytes when using Password::min(8). I fully understand, that the max rule should prevent the truncation for the bcrypt algorithm, but this would be counterintuitive using chars for the min rule and bytes for the max rule.
Do we then also have to add a new validation message?

The :attribute field must not be greater than :max characters.

Clearly refers to chars and the number of chars could differ to the number of bytes.

[valorin]

Oh, damn, good point. I always get confused between those...

I don't like the idea of using bytes in the validation message - the concept is confusing to those who don't understand the difference between bytes and chars.

I wonder if we should stick with simple and validate based on 72 characters, and just allow bcrypt to truncate it if someone uses multi-byte chars?

Another option is to hash the raw password before bcrypting, to remove the limits. But I've seen advice against this. 🤔

[ziadoz]

I think it makes sense for min() and max() to use mb_strlen(), as password validation is shown to users, who don't know or care about bytes vs characters.

Perhaps the best thing to do on the bcrypt 72 byte truncation issue is... nothing? It's a limitation of the hashing algorithm that's impossible to meaningfully explain to users.

Any developer who decides it does need explaining could do so easily enough in their app:

if (config('hashing.driver') === 'bcrypt' && strlen($password) > 72) {
    $fail('Password is too long for hashing algorithm');
}

[valorin]

I think it makes sense for min() and max() to use mb_strlen(), as password validation is shown to users, who don't know or care about bytes vs characters.

Perhaps the best thing to do on the bcrypt 72 byte truncation issue is... nothing? It's a limitation of the hashing algorithm that's impossible to meaningfully explain to users.

I agree. I think it's fine to keep mb_strlen(), limit on characters, and ignore the bytes issue.

[angelej]

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@taylorotwell , we've discussed this topic and came to the conclusion to limit the password by chars and not bytes.
Can you please reopen this pull request?

[ziadoz]

@angelej In the discussion above I think we reached the conclusion that it isn't worth trying to handle the bcrypt byte truncation behaviour, so I suspect this PR doesn't need re-opening.

[pxlrbt]

I don't understand the OWASP recommendation on this. People that use 72 or more chars for their password are very likely using a password manager. I don't care whether it uses 72 or 75 or 80. All should be more secure than any handpicked password. But I am annoyed when I need to generate passwords multiple times because the max length is locked.

[Jubeki]

@pxlrbt if you use a password which is longer than 72 characters an it is truncated (like with bcrypt), but still accept such a password knowing only the first 72 characters will be used, you will give the user a false sense of security, because they assume, that all characters will be used.

[ziadoz]

@Jubeki Bcrypt truncates the password after 72 bytes not 72 "characters". ∆ is 1 "character", but 3 bytes.

[Jubeki]

@ziadoz I mostly assume that ASCII-charactes (which are 1 byte) are what normally are used in passwords, though that is wrong to assume you are right.

[valorin]

I don't understand the OWASP recommendation on this. People that use 72 or more chars for their password are very likely using a password manager. I don't care whether it uses 72 or 75 or 80. All should be more secure than any handpicked password. But I am annoyed when I need to generate passwords multiple times because the max length is locked.

While I agree with the sentiment that enforcing a maximum length is annoying, 72 characters/bytes is significantly more than the 8, 16, or 20 characters we're normally getting frustrated at in stupid password limits. Do you actually use passwords longer than 72 characters normally that this would affect you?

In my opinion, a 72+ character password is massive overkill, I can't see any practical reason for using a password that long.

Regarding OWASP, my understanding is the same as @Jubeki - it's to prevent a false sense of security with the hidden truncation of the password.
There are ways around it - such as pre-hashing the password before bcrypting, although they have their own risks.

Also note, this wasn't merged, so all we've got so far is an optional rule in the other PR that isn't enabled by default. Making this a moot point if this isn't reopened or attempted in another PR.