Feature #79795

Improve saltedpasswords

Added by Christian Futterlieb over 2 years ago. Updated 11 months ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Authentication
Target version:
-
Start date:
2017-12-12
Due date:
% Done:

100%

PHP Version:
Tags:
Complexity:
Sprint Focus:

Description

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


Subtasks

Task #83294: Clean up of the saltedpasswords APIClosedBenni Mack


Related issues

Related to TYPO3 Core - Feature #79888: Constant-time password checking Closed 2017-02-18
Related to TYPO3 Core - Feature #79889: Cleanup saltedpasswords salt api and add the PHP password api as salt Closed 2017-02-18

Associated revisions

Revision 77f08248 (diff)
Added by Christian Futterlieb over 2 years ago

[TASK] Compare password hashes in constant time

In order to avoid time-based hash-based attacks, the native
PHP security functions are used instead of simple string
comparisons, when comparing passwords with hashes.

Change-Id: I0dbe2c12c5017f9d71ea7628ddd35d919510ac12
Releases: master
Resolves: #79888
Related: #79795
Reviewed-on: https://review.typo3.org/51737
Reviewed-by: Helmut Hummel <>
Tested-by: Helmut Hummel <>
Tested-by: TYPO3com <>
Reviewed-by: Mads Lønne Jensen <>
Reviewed-by: Markus Klein <>
Tested-by: Markus Klein <>

Revision 04810ae2 (diff)
Added by Christian Futterlieb over 2 years ago

[FOLLOWUP][TASK] Compare password hashes in constant time

Apply constant-time comparison to the fallback password checks as well.

Change-Id: I8d2aa6448c95266a45b2862f12f1a5d8259f4f0b
Releases: master
Resolves: #79888
Related: #79795
Reviewed-on: https://review.typo3.org/51853
Tested-by: TYPO3com <>
Reviewed-by: Christian Kuhn <>
Tested-by: Christian Kuhn <>
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>

History

#1 Updated by Christian Futterlieb over 2 years ago

  • Description updated (diff)

Correction: PHP password api is available in PHP 5 >= 5.5.0, PHP 7

#2 Updated by Christian Futterlieb over 2 years ago

  • Description updated (diff)

#3 Updated by Christian Kuhn 11 months ago

  • Status changed from New to Closed

I'm closing this issue now since most parts have been done in single patches: saltedpassword itself has been merged into core, the api has been cleaned up, the install tool now selects "good" hash method by default, constant time comparison has been done.
If we still need changes in this area, let's do them in single issues.

#4 Updated by Christian Futterlieb 11 months ago

Thank you @Christian Kuhn and all the other people who worked at the things for taking care.

Also available in: Atom PDF