From 77f082488154f5ae1aac9ad4da2c18e72149099b Mon Sep 17 00:00:00 2001
From: Christian Futterlieb <christian@futterlieb.ch>
Date: Sat, 18 Feb 2017 11:51:07 +0100
Subject: [PATCH] [TASK] Compare password hashes in constant time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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 <typo3@helhum.io>
Tested-by: Helmut Hummel <typo3@helhum.io>
Tested-by: TYPO3com <no-reply@typo3.com>
Reviewed-by: Mads Lønne Jensen <mlj@systime.dk>
Reviewed-by: Markus Klein <markus.klein@typo3.org>
Tested-by: Markus Klein <markus.klein@typo3.org>
---
.../saltedpasswords/Classes/Salt/Md5Salt.php | 2 +-
.../Classes/Salt/Pbkdf2Salt.php | 2 +-
.../Classes/Salt/PhpassSalt.php | 2 +-
.../Tests/Unit/Salt/BlowfishSaltTest.php | 27 +++++++++++++++++++
.../Tests/Unit/Salt/Md5SaltTest.php | 27 +++++++++++++++++++
.../Tests/Unit/Salt/Pbkdf2SaltTest.php | 27 +++++++++++++++++++
.../Tests/Unit/Salt/PhpassSaltTest.php | 27 +++++++++++++++++++
7 files changed, 111 insertions(+), 3 deletions(-)
diff --git a/typo3/sysext/saltedpasswords/Classes/Salt/Md5Salt.php b/typo3/sysext/saltedpasswords/Classes/Salt/Md5Salt.php
index fc7ba6af88fe..0543b4380942 100644
--- a/typo3/sysext/saltedpasswords/Classes/Salt/Md5Salt.php
+++ b/typo3/sysext/saltedpasswords/Classes/Salt/Md5Salt.php
@@ -82,7 +82,7 @@ class Md5Salt extends AbstractSalt implements SaltInterface
{
$isCorrect = false;
if ($this->isValidSalt($saltedHashPW)) {
- $isCorrect = crypt($plainPW, $saltedHashPW) == $saltedHashPW;
+ $isCorrect = \password_verify($plainPW, $saltedHashPW);
}
return $isCorrect;
}
diff --git a/typo3/sysext/saltedpasswords/Classes/Salt/Pbkdf2Salt.php b/typo3/sysext/saltedpasswords/Classes/Salt/Pbkdf2Salt.php
index 3f965c021079..55784725ab4d 100644
--- a/typo3/sysext/saltedpasswords/Classes/Salt/Pbkdf2Salt.php
+++ b/typo3/sysext/saltedpasswords/Classes/Salt/Pbkdf2Salt.php
@@ -108,7 +108,7 @@ class Pbkdf2Salt extends AbstractSalt implements SaltInterface
*/
public function checkPassword($plainPW, $saltedHashPW)
{
- return $this->isValidSalt($saltedHashPW) && $this->getHashedPassword($plainPW, $saltedHashPW) === $saltedHashPW;
+ return $this->isValidSalt($saltedHashPW) && \hash_equals($this->getHashedPassword($plainPW, $saltedHashPW), $saltedHashPW);
}
/**
diff --git a/typo3/sysext/saltedpasswords/Classes/Salt/PhpassSalt.php b/typo3/sysext/saltedpasswords/Classes/Salt/PhpassSalt.php
index 3b538bf0aed4..89d93cb16e2a 100644
--- a/typo3/sysext/saltedpasswords/Classes/Salt/PhpassSalt.php
+++ b/typo3/sysext/saltedpasswords/Classes/Salt/PhpassSalt.php
@@ -125,7 +125,7 @@ class PhpassSalt extends AbstractSalt implements SaltInterface
public function checkPassword($plainPW, $saltedHashPW)
{
$hash = $this->cryptPassword($plainPW, $saltedHashPW);
- return $hash && $saltedHashPW === $hash;
+ return $hash && \hash_equals($hash, $saltedHashPW);
}
/**
diff --git a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/BlowfishSaltTest.php b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/BlowfishSaltTest.php
index d9b4275bdfd3..c1c98e5048b0 100644
--- a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/BlowfishSaltTest.php
+++ b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/BlowfishSaltTest.php
@@ -133,6 +133,33 @@ class BlowfishSaltTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
$this->objectInstance->setHashCount(null);
}
+ /**
+ * Tests authentication procedure with fixed password and fixed (pre-generated) hash.
+ *
+ * Checks if a "plain-text password" is every time mapped to the
+ * same "salted password hash" when using the same fixed salt.
+ *
+ * @test
+ */
+ public function authenticationWithValidAlphaCharClassPasswordAndFixedHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$2a$07$Rvtl6CyMhR8GZGhHypjwOuydeN0nKFAlgo1LmmGrLowtIrtkov5Na';
+ $this->assertTrue($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
+ /**
+ * Tests that authentication procedure fails with broken hash to compare to
+ *
+ * @test
+ */
+ public function authenticationFailsWithBrokenHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$2a$07$Rvtl6CyMhR8GZGhHypjwOuydeN0nKFAlgo1LmmGrLowtIrtkov5N';
+ $this->assertFalse($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
/**
* Tests authentication procedure with alphabet characters.
*
diff --git a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Md5SaltTest.php b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Md5SaltTest.php
index 96959e5058fc..0a09ed428f79 100644
--- a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Md5SaltTest.php
+++ b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Md5SaltTest.php
@@ -117,6 +117,33 @@ class Md5SaltTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
$this->assertTrue($this->objectInstance->isValidSaltedPW($saltedHashPassword), $this->getWarningWhenMethodUnavailable());
}
+ /**
+ * Tests authentication procedure with fixed password and fixed (pre-generated) hash.
+ *
+ * Checks if a "plain-text password" is every time mapped to the
+ * same "salted password hash" when using the same fixed salt.
+ *
+ * @test
+ */
+ public function authenticationWithValidAlphaCharClassPasswordAndFixedHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$1$GNu9HdMt$RwkPb28pce4nXZfnplVZY/';
+ $this->assertTrue($this->objectInstance->checkPassword($password, $saltedHashPassword), $this->getWarningWhenMethodUnavailable());
+ }
+
+ /**
+ * Tests that authentication procedure fails with broken hash to compare to
+ *
+ * @test
+ */
+ public function authenticationFailsWithBrokenHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$1$GNu9HdMt$RwkPb28pce4nXZfnplVZY';
+ $this->assertFalse($this->objectInstance->checkPassword($password, $saltedHashPassword), $this->getWarningWhenMethodUnavailable());
+ }
+
/**
* Tests authentication procedure with alphabet characters.
*
diff --git a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Pbkdf2SaltTest.php b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Pbkdf2SaltTest.php
index 59292a352f92..4b734f30546a 100644
--- a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Pbkdf2SaltTest.php
+++ b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/Pbkdf2SaltTest.php
@@ -120,6 +120,33 @@ class Pbkdf2SaltTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
$this->objectInstance->setHashCount(null);
}
+ /**
+ * Tests authentication procedure with fixed password and fixed (pre-generated) hash.
+ *
+ * Checks if a "plain-text password" is every time mapped to the
+ * same "salted password hash" when using the same fixed salt.
+ *
+ * @test
+ */
+ public function authenticationWithValidAlphaCharClassPasswordAndFixedHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$pbkdf2-sha256$1000$woPhT0yoWm3AXJXSjuxJ3w$iZ6EvTulMqXlzr0NO8z5EyrklFcJk5Uw2Fqje68FfaQ';
+ $this->assertTrue($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
+ /**
+ * Tests that authentication procedure fails with broken hash to compare to
+ *
+ * @test
+ */
+ public function authenticationFailsWithBrokenHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$pbkdf2-sha256$1000$woPhT0yoWm3AXJXSjuxJ3w$iZ6EvTulMqXlzr0NO8z5EyrklFcJk5Uw2Fqje68Ffa';
+ $this->assertFalse($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
/**
* Tests authentication procedure with alphabet characters.
*
diff --git a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/PhpassSaltTest.php b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/PhpassSaltTest.php
index 0df190f38a13..4772385b9d90 100644
--- a/typo3/sysext/saltedpasswords/Tests/Unit/Salt/PhpassSaltTest.php
+++ b/typo3/sysext/saltedpasswords/Tests/Unit/Salt/PhpassSaltTest.php
@@ -117,6 +117,33 @@ class PhpassSaltTest extends \TYPO3\TestingFramework\Core\Unit\UnitTestCase
$this->objectInstance->setHashCount(null);
}
+ /**
+ * Tests authentication procedure with fixed password and fixed (pre-generated) hash.
+ *
+ * Checks if a "plain-text password" is every time mapped to the
+ * same "salted password hash" when using the same fixed salt.
+ *
+ * @test
+ */
+ public function authenticationWithValidAlphaCharClassPasswordAndFixedHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$P$C7u7E10SBEie/Jbdz0jDtUcWhzgOPF.';
+ $this->assertTrue($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
+ /**
+ * Tests that authentication procedure fails with broken hash to compare to
+ *
+ * @test
+ */
+ public function authenticationFailsWithBrokenHash()
+ {
+ $password = 'password';
+ $saltedHashPassword = '$P$C7u7E10SBEie/Jbdz0jDtUcWhzgOPF';
+ $this->assertFalse($this->objectInstance->checkPassword($password, $saltedHashPassword));
+ }
+
/**
* Tests authentication procedure with alphabet characters.
*
--
GitLab