maj: sémantique, révision vérification password #3

Merged
cmsassot merged 6 commits from maj into develop 2023-01-05 14:09:41 +01:00
9 changed files with 26 additions and 117 deletions
Showing only changes of commit 422e97d329 - Show all commits

1
.env
View File

@ -30,7 +30,6 @@ BASE_URL='http://localhost:8080'
HYDRA_ADMIN_BASE_URL='http://hydra:4445'
APP_LOCALES="fr,en"
SECURITY_PATTERN=
NEW_HASH_ALGO=
HASH_ALGO_LEGACY="sha256"
###> symfony/lock ###
# Choose one of the stores below

View File

@ -13,9 +13,6 @@ parameters:
env(HASH_ALGO_LEGACY): "sha256"
hashAlgoLegacy: '%env(resolve:HASH_ALGO_LEGACY)%'
env(NEW_HASH_ALGO): "sha256"
newHashAlgo: '%env(resolve:NEW_HASH_ALGO)%'
# adresse du site hote
issuer_url: '%env(resolve:ISSUER_URL)%'
# adresse de hydra
@ -71,7 +68,6 @@ services:
arguments:
$pepper: '%pepper%'
$hashAlgoLegacy: '%hashAlgoLegacy%'
$newHashAlgo: '%newHashAlgo%'
$securityPattern: '%security_pattern%'
# add more service definitions when explicit configuration is needed
# please note that last definitions always *replace* previous ones

View File

@ -10,6 +10,6 @@ INSERT INTO usager (email, password, salt, lastname, firstname) VALUES
('test3@test.com', '504ae1c3e2f5fdaf41f868164dabcef21e17059f5f388b452718a1ce92692c67', 'cesaltestunautreexemple', 'Dupont', 'Henri');
INSERT INTO usager (email, password, lastname, firstname) VALUES
('test4@test.com', '$2a$12$zFN0VJ..Cuu.2itWQwmHJe5EUhNHazbMfCSJFpNiEfdwpLzjjDM0u', 'Durand', 'Isabelle'),
('test4@test.com', '$2a$12$91AHN7WFXieeadvUfZ88mO.9N7oS5adeXbdERnRno9oLAbqqDW4IG', 'Durand', 'Isabelle'),
('test2@test.com', '50626fa21f45a275cea0efff13ff78fd02234cade322da08b7191c7e9150141d', 'Dubois', 'Angela');
GRANT ALL PRIVILEGES ON DATABASE usager TO lasql

View File

@ -38,7 +38,6 @@ services:
- DEFAULT_LOCALE=fr
- DSN_REMOTE_DATABASE=pgsql:host='postgres';port=5432;dbname=lasql;
- HASH_ALGO_LEGACY=sha256
- NEW_HASH_ALGO=bcrypt
- SECURITY_PATTERN=password,salt,pepper

View File

@ -34,23 +34,26 @@ BASE_URL='http://localhost:8080'
HYDRA_ADMIN_BASE_URL='http://hydra:4445'
DSN_REMOTE_DATABASE="pgsql:host='postgres';port=5432;dbname=lasql"
APP_LOCALES="fr,en"
NEW_HASH_ALGO="argon2id"
HASH_ALGO_LEGACY="sha256, bcrypt"
SECURITY_PATTERN="password,salt,pepper"
PEPPER=
```
## Tests password
Dans le cas où plusieurs méthodes de hashage cohabitent (migration de méthode par exemple dans l'application principale), toutes les méthode seront testées pour comparer les hashs jusqu'à un succès
Indication des algorythmes de hashage obsolètes (legacy) sous forme d'une string séparé par des virgules
Indication du nouvel algorythme à utliser
Au login, vérification du mot de passe, et si méthode de hashage obsolète, update du hashage avec nouvelle méthode et requete d'update à la bdd distante.
## Pattern de hashage
```
Définir le pattern utilisé avec les mots clés autorisé: password | salt | pepper dans un string, séparé par des virgules pour représenter la séquence à employer pour le hashage du mot de passe.
```
### Postgres
```
Les mot de passe inscrits en bdd sont hachés en tenant compte du salt si non vide (cf données de la bdd plus bas) et du pepper inscrit en variable d'environnement (généré avec : bin2hex(random_bytes(32))
Les mot de passe inscrits en bdd sont hachés en tenant compte du salt si non vide (cf données de la bdd plus bas) et du pepper inscrit en variable d'environnement (généré avec : bin2hex(random_bytes(32) pour l'exemple), le pepper peut être vide
Indiquer le nom de la colonne contenant le salt:
Il faut inscrire dans slq_login_configuration salt_column_name: salt
Indiquer le pepper utilisé:
et conserver le pepper dans service.yaml
env(PEPPER): "257d62c24cd352c21b51c26dba678c8ff05011a89022aec106185bf67c69aa8b"
@ -78,7 +81,7 @@ DSN_REMOTE_DATABASE="mysql:host=mariadb;port=3306;dbname=lasql;"
|test1@test.com| 8ad4025044b77ae6a5e3fcf99e53e44b15db9a4ecf468be21cbc6b9fbdae6d9f| cesaltestunexemple| Locke|John|
|test2@test.com| 50626fa21f45a275cea0efff13ff78fd02234cade322da08b7191c7e9150141d| NULL| Dubois| Angela|
|test3@test.com| 504ae1c3e2f5fdaf41f868164dabcef21e17059f5f388b452718a1ce92692c67| cesaltestunautreexemple| Dupont| Henri|
|test4@test.com| $2a$12$zFN0VJ..Cuu.2itWQwmHJe5EUhNHazbMfCSJFpNiEfdwpLzjjDM0u| NULL| Durand|Isabelle|
|test4@test.com| $2a$12$91AHN7WFXieeadvUfZ88mO.9N7oS5adeXbdERnRno9oLAbqqDW4IG| NULL| Durand|Isabelle|
A noter que le hash de test4 est en bcrypt
```

View File

@ -88,15 +88,4 @@ class SQLLoginRequest
return 'SELECT '.$fields.' FROM '.$this->getTableName().' WHERE '.$this->getLoginColumnName().' = :'.$this->getLoginColumnName().';';
}
public function getRequestUpdatePassword()
{
$fieldsToUpdate = $this->getPasswordColumnName().'= :'.$this->getPasswordColumnName().',';
if (!empty($this->getSaltColumnName())) {
$fieldsToUpdate .= $this->getSaltColumnName().'= :'.$this->getSaltColumnName().',';
}
$fieldsToUpdate = substr($fieldsToUpdate, 0, -1);
return 'UPDATE '.$this->getTableName().' SET '.$fieldsToUpdate.' WHERE '.$this->getLoginColumnName().' = :'.$this->getLoginColumnName().';';
}
}

View File

@ -2,7 +2,6 @@
namespace App\Security\Hasher;
use App\SQLLogin\Exception\InvalidSQLLoginAlgoException;
use App\SQLLogin\Exception\InvalidSQLLoginConfigurationException;
use App\SQLLogin\Exception\InvalidSQLPasswordException;
use Symfony\Component\PasswordHasher\Exception\InvalidPasswordException;
@ -18,31 +17,25 @@ class PasswordEncoder implements LegacyPasswordHasherInterface
protected ?string $pepper;
protected array $hashAlgoLegacy;
protected ?string $newHashAlgo;
protected array $securityPattern;
public function __construct(?string $pepper, string $hashAlgoLegacy, ?string $newHashAlgo, string $securityPattern)
public function __construct(?string $pepper, string $hashAlgoLegacy, string $securityPattern)
{
$this->pepper = $pepper;
$this->hashAlgoLegacy = explode(',', $hashAlgoLegacy);
$this->newHashAlgo = $newHashAlgo;
$this->securityPattern = explode(',', $securityPattern);
}
rmasson marked this conversation as resolved Outdated

Je pense que ce code ne sera pas compatible avec les applications ayant choisies d'utiliser les algorithmes argon2id, scrypt et bcrypt (qui sont d'ailleurs les recommandations OWASP 1 aujourd'hui).

Normalement, en PHP on utilise les méthodes password_hash() 2 et password_verify() 3 pour utiliser ces algorithmes (et d'ailleurs le salt est directement stocké dans le hash final 4).

Autre chose: la méthode assume un certain format pour la concaténation des différents éléments à hacher ($plainPassword.$salt.$this->pepper). Il est fort peu probable que cette séquence soit toujours respectée dans toutes les applications. À mon avis il serait certainement préférable d'utiliser un patron (avec la méthode strstr() 5 par exemple) pour permettre à l'utilisateur de spécifiquer le format de concaténation.

Je pense que ce code ne sera pas compatible avec les applications ayant choisies d'utiliser les algorithmes `argon2id`, `scrypt` et `bcrypt` (qui sont d'ailleurs les recommandations OWASP [^1] aujourd'hui). Normalement, en PHP on utilise les méthodes `password_hash()` [^2] et `password_verify()` [^3] pour utiliser ces algorithmes (et d'ailleurs le salt est directement stocké dans le hash final [^4]). Autre chose: la méthode assume un certain format pour la concaténation des différents éléments à hacher (`$plainPassword.$salt.$this->pepper`). Il est fort peu probable que cette séquence soit toujours respectée dans toutes les applications. À mon avis il serait certainement préférable d'utiliser un patron (avec la méthode `strstr()` [^5] par exemple) pour permettre à l'utilisateur de spécifiquer le format de concaténation. [^1]: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html [^2]: https://www.php.net/manual/en/function.password-hash.php [^3]: https://www.php.net/manual/en/function.password-verify.php [^4]: https://stackoverflow.com/questions/40993645/understanding-bcrypt-salt-as-used-by-php-password-hash [^5]: https://www.php.net/manual/en/function.strtr.php

j'ai utilisé les fonction password_algos() et hash_algos() pour faire la différence entre les algos, pour utiliser hash() ou password_hash()

Mais effectivement le pattern est pas pris en compte

j'ai utilisé les fonction password_algos() et hash_algos() pour faire la différence entre les algos, pour utiliser hash() ou password_hash() Mais effectivement le pattern est pas pris en compte
/**
* Utilise l'algo legacy
* Pas utilisé
*/
public function hash(string $plainPassword, string $salt = null): string
{
if ($this->isPasswordTooLong($plainPassword)) {
throw new InvalidPasswordException();
}
$completedPlainPassword = $this->getPasswordToHash($plainPassword, $salt);
if ($this->isObsoleteAlgo($this->newHashAlgo)) {
throw new InvalidSQLLoginAlgoException();
}
return password_hash($completedPlainPassword, $this->getValidALgo($this->newHashAlgo));
return hash($plainPassword.$salt, $this->hashAlgoLegacy[0]);

Il ne faut pas utiliser l'opérateur === pour faire des comparaisons de mots de passe (ça ouvre à des failles du type "Timing attack" 1).

Il serait mieux d'utiliser https://www.php.net/manual/en/function.hash-equals.php je pense dans ce cas.

Il ne faut pas utiliser l'opérateur `===` pour faire des comparaisons de mots de passe (ça ouvre à des failles du type "Timing attack" [^1]). Il serait mieux d'utiliser https://www.php.net/manual/en/function.hash-equals.php je pense dans ce cas. [^1]: https://www.chosenplaintext.ca/articles/beginners-guide-constant-time-cryptography.html
}
public function verify(string $hashedPassword, string $plainPassword, string $salt = null): bool
@ -51,19 +44,8 @@ class PasswordEncoder implements LegacyPasswordHasherInterface
return false;
}
$isNewest = password_get_info($hashedPassword)['algo'] === $this->newHashAlgo;
$completedPassword = $this->getPasswordToHash($plainPassword, $salt);
if ($isNewest) {
if (password_verify($completedPassword, $hashedPassword)) {
return true;
} else {
throw new InvalidSQLPasswordException();
}
}
// Si le mot de passe a besoin d'un rehash ou qu'il n'y a pas de nouvelle méthode indiquée, on sait qu'il faut utiliser l'une des méthodes legacy pour vérifier le mot de passe
if ($this->needsRehash($hashedPassword) || empty($this->newHashAlgo)) {
foreach ($this->hashAlgoLegacy as $algo) {
if ($this->isObsoleteAlgo($algo)) {
if (hash_equals(hash($algo, $completedPassword), $hashedPassword)) {
@ -74,57 +56,15 @@ class PasswordEncoder implements LegacyPasswordHasherInterface
return true;
}
}
// On vérifie si la méthode legacy est obsolète, si oui on ne peut pas utiliser password_verify, on doit hasher et comparer
}
// si on on n'a pas encore retourné de résultat, le mot de passe doit être incorrect
throw new InvalidSQLPasswordException();
}
// Si on n'est pas rentré dans les conditions précedéntes, on peut utiliser password_verify
if (password_verify($completedPassword, $hashedPassword)) {
return true;
} else {
throw new InvalidSQLPasswordException();
}
}
public function needsRehash(string $hashedPassword): bool
{
// Il y a besoin de tester si on veut mettre à jour le hashage de mot de passe uniquement si le nouveau algo de hashage est indiqué et qu'il est moderne (BCRYPT, ARGON, ...)
if (!empty($this->newHashAlgo) && !$this->isObsoleteAlgo($this->newHashAlgo)) {
return password_needs_rehash($hashedPassword, $this->getValidAlgo($this->newHashAlgo));
}
return false;
}
/**
* @param mixed $algo
*
* @return string
*/
public function getValidAlgo($algo)
{
if ($algo) {
if (in_array($algo, hash_algos())) {
return $algo;
}
$informalAlgos = [
'default' => PASSWORD_DEFAULT,
'bcrypt' => PASSWORD_BCRYPT,
'argon2i' => PASSWORD_ARGON2I,
'argon2id' => PASSWORD_ARGON2ID,
];
if (in_array($algo, array_keys($informalAlgos))) {
return $informalAlgos[$algo];
}
if (in_array($algo, password_algos())) {
return $algo;
}
throw new InvalidSQLLoginAlgoException('Invalid Algorythme');
}
}
public function isObsoleteAlgo($algo): bool
{
return in_array($algo, hash_algos());

View File

@ -82,10 +82,10 @@ class SQLLoginUserAuthenticator extends AbstractAuthenticator
// Comparaison remote hash et hash du input password + salt
// dump($remoteHashedPassword, $plaintextPassword, $remoteSalt, password_verify($plaintextPassword, $remoteHashedPassword));
$this->passwordHasher->verify($remoteHashedPassword, $plaintextPassword, $remoteSalt);
if ($this->passwordHasher->needsRehash($remoteHashedPassword)) {
$hash = $this->passwordHasher->hash($plaintextPassword);
$this->sqlLoginService->updatePassword($login, $hash, null);
}
// if ($this->passwordHasher->needsRehash($remoteHashedPassword)) {
// $hash = $this->passwordHasher->hash($plaintextPassword);
// $this->sqlLoginService->updatePassword($login, $hash, null);
// }
$attributes = $this->sqlLoginService->fetchDatas($login);
$user = new User($login, $remoteHashedPassword, $attributes, $rememberMe);

View File

@ -63,23 +63,6 @@ class SQLLoginService extends AbstractController
return false;
}
public function updatePassword($login, $hashedPassword, $salt)
{
try {
$dbh = $this->getConnection();
$request = $this->sqlLoginRequest->getRequestUpdatePassword();
$query = $dbh->prepare($request);
$query->execute([
$this->sqlLoginRequest->getLoginColumnName() => $login,
$this->sqlLoginRequest->getPasswordColumnName() => $hashedPassword,
$this->sqlLoginRequest->getSaltColumnName() => $salt,
]);
} catch (PDOException $e) {
\Sentry\captureException($e);
throw new PDOException();
}
}
public function getConnection()
{
// Appel du singleton