TYPO3 Core

Hi TYPO3 folks and core team!

I’m working on some ideas to improve the saltedpasswords extension. In the following I’m going to describe the fields. Every task can be separated from the other, although some could influence others.

1. Constant-time password checking, see #79888
This one is a no-brainer: replace all $knownPwassword == $givenPassword by either password_verify() (if the algo is supported) or hash_equals() otherwise.
Suggested branch: master, 7.6

2. Constant-time base64 encoding
I suggest using paragonie/constant_time_encoding in AbstractSalt::base64Encode():

• Avoids passing secret data to chr() and ord() (which would leak timing information)
• Delivers various base64-flavours, of which we can use Base64DotSlash to stay crypt()-compatible
• Delivers a nice "Binary" class that handles mbstring function overloading with strlen() and substr(). That is off the current topic, I just name it to be complete

Suggested branch: master, 7.6

3. Add the PHP password api as Salt
See https://secure.php.net/manual/en/book.password.php. It’s part of php (>= 5.5.0, 7) standard and thus available everywhere. At the moment, bcrypt is used as the default algorithm, which is widely considered secure (as long as the cost parameter is at an adequate level). The algorithm is subject to change in the future, most probably argon2i (winner of the password hashing competition) will be a candidate for the next default.

Because of the simple api this can be easily abstracted and implemented.

But there is a problem with the current saltedpasswords api: it is designed for classes that compose a password hash themselves. The new php password api does this internally, as well as generating a secure salt. Therefor following methods are useless for the new method:

• SaltInterface::getSaltLength()
• SaltInterface::isValidSalt()
• The complete class AbstractSalt

While digging deeper into the code, I recognised, that the AbstractSalt is used only once, namely in SaltFactory::setPreferredHashingMethod(). In there SaltFactory::$instance is set only when the object extends AbstractSalt. Which makes SaltFactory::getSaltingInstance() return an AbstractSalt instead of a SaltInterface. Which is obviously a bug. This combination of AbstractSalt and SaltInterface is irritating but in the current situation not a problem, because all available Salt classes implement the interface and extend AbstractSalt.

To resolve the inconsistency I propose following changes:

1. SaltFactory deals only with SaltInterface objects (fix the bug in SaltFactory::setPreferredHashingMethod())
2. Move AbstractSalt to AbstractComposedSalt (for classes that compose the password hash themselves). It implements SaltInterface
3. Move SaltInterface::getSaltLenght() and SaltInterface::isValidSalt() to AbstractComposedSalt, because they are only needed there

Then clean up:

1. Deprecate usage of $salt in SaltInterface::getHashedPassword($password, $salt = null). Salt classes should generate the random salt with Random::generateRandomBytes(). This is no big deal, throughout the core, no userdefined salts are used.

And finally come to the main topic:

1. Introduce new abstract class AbstractPhpPasswordHashSalt, which implements SaltInterface. It abstracts the php password api for child classes, which then only have to manage the algorithm constant and the options array
2. Introduce new class PhpPasswordHashBcryptSalt that extends AbstractPhpPasswordHashSalt

Suggested branch: master. Some of the proposed changes may go to 7.6 as well, listening for your opinions.

4. Get rid of the old Salt classes
IMO, salts like MD5 and phpass should not be part of the core anymore, as they cannot be considered secure (enough) anymore. I suggest moving them to a TER extension. If we introduce the php password api, BlowfishSalt should go there too.
The old methods should be there for interoperability with 3rd party systems only and not decrease password security in every TYPO3 installation (that typically runs all alone).

Suggested branch: master

5. Secure defaults
Define PhpPasswordHashBcryptSalt as the default salt. It delivers good security and is available in php standard.

Suggested branch: master

6. Install Tool
Install tool should use the most secure password storage method that is available. Because if the hash is leaked somehow, an attacker should not be able to compute the actual password. And the strength should be improved even more by for example increasing cost or iterations.

Suggested branch: master, 7.6

7. Utility for the admin
Add some functionality for admins to test the best settings for the password hashes, maybe in the "Important Actions" in install tool. It should run the password hashing methods with different cost/iteration parameters and suggest a good value by measuring the time. Like this, the admin can determine a good setting in a system under load.

Suggested branch: master

I know, this is a lot of stuff in here. But I decided to collect them together to start a first overall-discussion. The tasks can later be split up when it comes to implementing.

I'd love to hear your feedback =)
Thanks and best regards, Christian