[security] Password handling: Limit maximum allowed password length

[timoh6]

Reading http://www.codeigniter.com/userguide3/general/security.html#password-handling I noticed there is a recommendation:

DO NOT put artificial limits on your users’ passwords.

There’s no point in forcing a rule that a password can only be up to a number of characters, or that it can’t contain a certain set of special characters.

Not only does this reduce security instead of improving it, but there’s literally no reason to do it. No technical limitations and no (practical) storage constraints apply once you’ve hashed them, none!

This is actually not correct "as is" (from a security point of view), as overlong user inputs may cause different kinds of problems to the system.

With CI, it is actually a security problem at least if PBKDF2 is used (from the compatibility layer), as long passwords will cause the PBKDF2 implementation to choke on calculating the inner HMAC (i.e. if the password is, say, 100000 bytes long). As an example, hash_pbkdf2() with a million byte password, SHA-256 and 20 000 iterations from the compatibility layer takes ~130 seconds.

This could be fixed (with regards to PBKDF2) by pre-hashing the password when it has greater length than the underlying hash algoritm's block size, but this approach feels a bit hacky to say the least, as CI supports PBKDF2 with many different hash algorithms (you would need to determine the underlying block size for every hash algorithm).

That being said, I recommend you set a password length limit. It could be something "huge", like 1024 bytes etc. to make sure it won't affect (at least in a meaningful way) real passwords used with PBKDF2, and at the same time it mitigates the DOS possibility.

[narfbg]

Agreed ... how's that?

a8c499d0125b2e96f7f3c539f6b46cff7547aa80

[timoh6]

Documentation part looks good to me (thought "bcrypt" should be typed all lowercase).

In addition, I'd set a password length limit on the code side too, as otherwise it requires extra work from the framework user. If using "high limit", it will be unnoticeable and at the same time mitigate the DoS without requiring any extra work from the CI user. 1024 - 4096 bytes limit will be just fine.

[narfbg]

We don't have wrappers around password_hash(), hash_pbkdf2(), etc. We only have "backports" of these functions that should act exactly the same as in PHP, so putting limits inside of them isn't something we'd want to do.

A third-party library should do that instead.

[narfbg]

Oh, and thanks for noting this ... we hardly get any meaningful feedback on security features.

[timoh6]

Ahhh I see. Being "100% compatible" with PHP requires to do the "pre-hashing" when the password is longer than the underlying block size (as PHP's hash_pbkdf2 does it), but indeed it becomes hacky (as there is no way to tell the underlying block size except hard coding them, at least as far as I'm aware).

Maybe maintaining some kind of hard coded list of common hash function's block sizes would be fine (and pre-hashing when needed).

But on the other hand, even if CI limited password lengths (with 1024+ bytes), I don't think it causes any problems (as we are talking about passwords and 1024+ lengths).

[narfbg]

Hmm, I'm quite sure we're replicating PHP's behavior on 100% for hash_pbkdf2() and at this point I'm not sure what you mean by pre-hashing. Can you elaborate?

[timoh6]

Keys for HMAC must be exactly the length of the underlying hash algorithms's block length. If the key is longer, it is hashed first with the underlying hash algorithm (if it's shorter it's zero-padded).

PHP's hash_hmac() will internally take care of that (the end result will match), but problem arise when it is done in a loop thousands of times. If the password is pre-hashed (say, before the outer for loop, if it is longer than the block size), the HMAC operation will not suffer from extra long input, and thus will run much faster (mitigating the DoS).

[narfbg]

I see ... I guess I'll have to add a list of block sizes then.

[timoh6]

That will do the job, but personally I would consider just setting a length limit (1024 bytes would be fine). While it is in theory not compatible with PHP's native implementation, but in practice it will be.

This way you can avoid quite a bit of lines of code and possibly maintenance burden ("every line of code is a potential security flaw").

[narfbg]

Well, it's a one-time thing that shouldn't require any maintenance in practice, so I'd rather have full compatibility. Thanks for the tips. :)