- Merge request !3146Issue #3328456: Replace substr($a, 0, $i) with str_starts_with() β (Closed) created by xjm
- πΊπΈUnited States smustgrave
Did a search for on phpstorm with ".substr(.*) == " and got some additional ones.
- Status changed to Postponed
almost 2 years ago 5:59pm 10 February 2023 - πΊπΈUnited States xjm
Re: #5, again, the goal is to only make most of the changes of this sort (not all of them to avoid merge conflicts. Then, the final issue will be rebased on top of all four to make the final changes.
Let's postpone this for now on π Replace most strpos() !== FALSE or === FALSE with str_contains() Fixed .
- Status changed to Needs work
over 1 year ago 7:48pm 23 March 2023 - πΊπΈUnited States xjm
The previous issue is in, so we can work on this now.
- πΊπΈUnited States xjm
Merge conflict resolved:
++<<<<<<< HEAD + public function check($password, $hash) { + if (str_starts_with($hash, 'U$')) { ++======= + public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash) { + if (substr($hash, 0, 2) == 'U$') { ++>>>>>>> 10.1.x
As:
+ public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash) { + if (str_starts_with($hash, 'U$')) {
- Status changed to Needs review
over 1 year ago 6:06pm 10 April 2023 - πΊπΈUnited States xjm
Merge conflicts:
[ayrton:drupal | Mon 13:20:10] $ git diff diff --cc core/lib/Drupal/Core/Password/PhpassHashedPassword.php index 56844b8117,4e8c9c1092..0000000000 --- a/core/lib/Drupal/Core/Password/PhpassHashedPassword.php +++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php @@@ -2,271 -2,15 +2,281 @@@ namespace Drupal\Core\Password; + @trigger_error('\Drupal\Core\Password\PhpassHashedPassword is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. The password compatibility service has been moved to the phpass module. Use \Drupal\phpass\Password\PhpassHashedPassword instead. See https://www.drupal.org/node/3322420', E_USER_DEPRECATED); + /** - * Secure hashing functions based on Portable PHP password hashing framework. + * Deprecated legacy password hashing framework. * - * @see http://www.openwall.com/phpass/ + * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. The + * password compatibility service has been moved to the phpass module. + * Use \Drupal\phpass\Password\PhpassHashedPassword instead. + * + * @see https://www.drupal.org/node/3322420 */ ++<<<<<<< HEAD +class PhpassHashedPassword implements PasswordInterface { + /** + * The minimum allowed log2 number of iterations for password stretching. + */ + const MIN_HASH_COUNT = 7; + + /** + * The maximum allowed log2 number of iterations for password stretching. + */ + const MAX_HASH_COUNT = 30; + + /** + * The expected (and maximum) number of characters in a hashed password. + */ + const HASH_LENGTH = 55; + + /** + * Returns a string for mapping an int to the corresponding base 64 character. + * + * @var string + */ + public static $ITOA64 = './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; + + /** + * Password stretching iteration count. + * + * Specifies the number of times the hashing function will be applied when + * generating new password hashes. The number of times is calculated by + * raising 2 to the power of the given value. + * + * @var int + */ + protected $countLog2; + + /** + * Constructs a new password hashing instance. + * + * @param int $countLog2 + * Password stretching iteration count. Specifies the number of times the + * hashing function will be applied when generating new password hashes. + * The number of times is calculated by raising 2 to the power of the given + * value. + */ + public function __construct($countLog2) { + // Ensure that $countLog2 is within set bounds. + $this->countLog2 = $this->enforceLog2Boundaries($countLog2); + } + + /** + * Encodes bytes into printable base 64 using the *nix standard from crypt(). + * + * @param string $input + * The string containing bytes to encode. + * @param int $count + * The number of characters (bytes) to encode. + * + * @return string + * Encoded string. + */ + protected function base64Encode($input, $count) { + $output = ''; + $i = 0; + do { + $value = ord($input[$i++]); + $output .= static::$ITOA64[$value & 0x3f]; + if ($i < $count) { + $value |= ord($input[$i]) << 8; + } + $output .= static::$ITOA64[($value >> 6) & 0x3f]; + if ($i++ >= $count) { + break; + } + if ($i < $count) { + $value |= ord($input[$i]) << 16; + } + $output .= static::$ITOA64[($value >> 12) & 0x3f]; + if ($i++ >= $count) { + break; + } + $output .= static::$ITOA64[($value >> 18) & 0x3f]; + } while ($i < $count); + + return $output; + } + + /** + * Generates a random base 64-encoded salt prefixed with hash settings. + * + * Proper use of salts may defeat a number of attacks, including: + * - The ability to try candidate passwords against multiple hashes at once. + * - The ability to use pre-hashed lists of candidate passwords. + * - The ability to determine whether two users have the same (or different) + * password without actually having to guess one of the passwords. + * + * @return string + * A 12 character string containing the iteration count and a random salt. + */ + protected function generateSalt() { + $output = '$S$'; + // We encode the final log2 iteration count in base 64. + $output .= static::$ITOA64[$this->countLog2]; + // 6 bytes is the standard salt for a portable phpass hash. + $output .= $this->base64Encode(random_bytes(6), 6); + return $output; + } + + /** + * Ensures that $count_log2 is within set bounds. + * + * @param int $count_log2 + * Integer that determines the number of iterations used in the hashing + * process. A larger value is more secure, but takes more time to complete. + * + * @return int + * Integer within set bounds that is closest to $count_log2. + */ + protected function enforceLog2Boundaries($count_log2) { + if ($count_log2 < static::MIN_HASH_COUNT) { + return static::MIN_HASH_COUNT; + } + elseif ($count_log2 > static::MAX_HASH_COUNT) { + return static::MAX_HASH_COUNT; + } + + return (int) $count_log2; + } + + /** + * Hash a password using a secure stretched hash. + * + * By using a salt and repeated hashing the password is "stretched". Its + * security is increased because it becomes much more computationally costly + * for an attacker to try to break the hash by brute-force computation of the + * hashes of a large number of plain-text words or strings to find a match. + * + * @param string $algo + * The string name of a hashing algorithm usable by hash(), like 'sha256'. + * @param string $password + * Plain-text password up to 512 bytes (128 to 512 UTF-8 characters) to + * hash. + * @param string $setting + * An existing hash or the output of $this->generateSalt(). Must be at least + * 12 characters (the settings and salt). + * + * @return string + * A string containing the hashed password (and salt) or FALSE on failure. + * The return string will be truncated at HASH_LENGTH characters max. + */ + protected function crypt($algo, #[\SensitiveParameter] $password, $setting) { + // Prevent DoS attacks by refusing to hash large passwords. + if (strlen($password) > PasswordInterface::PASSWORD_MAX_LENGTH) { + return FALSE; + } + + // The first 12 characters of an existing hash are its setting string. + $setting = substr($setting, 0, 12); + + if ($setting[0] != '$' || $setting[2] != '$') { + return FALSE; + } + $count_log2 = $this->getCountLog2($setting); + // Stored hashes may have been encrypted with any iteration count. However + // we do not allow applying the algorithm for unreasonable low and high + // values respectively. + if ($count_log2 != $this->enforceLog2Boundaries($count_log2)) { + return FALSE; + } + $salt = substr($setting, 4, 8); + // Hashes must have an 8 character salt. + if (strlen($salt) != 8) { + return FALSE; + } + + // Convert the base 2 logarithm into an integer. + $count = 1 << $count_log2; + + $hash = hash($algo, $salt . $password, TRUE); + do { + $hash = hash($algo, $hash . $password, TRUE); + } while (--$count); + + $len = strlen($hash); + $output = $setting . $this->base64Encode($hash, $len); + // $this->base64Encode() of a 16 byte MD5 will always be 22 characters. + // $this->base64Encode() of a 64 byte sha512 will always be 86 characters. + $expected = 12 + ceil((8 * $len) / 6); + return (strlen($output) == $expected) ? substr($output, 0, static::HASH_LENGTH) : FALSE; + } + + /** + * Parses the log2 iteration count from a stored hash or setting string. + * + * @param string $setting + * An existing hash or the output of $this->generateSalt(). Must be at least + * 12 characters (the settings and salt). + * + * @return int + * The log2 iteration count. + */ + public function getCountLog2($setting) { + return strpos(static::$ITOA64, $setting[3]); + } + + /** + * {@inheritdoc} + */ + public function hash(#[\SensitiveParameter] $password) { + return $this->crypt('sha512', $password, $this->generateSalt()); + } + + /** + * {@inheritdoc} + */ + public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash) { + if (str_starts_with($hash, 'U$')) { + // This may be an updated password from user_update_7000(). Such hashes + // have 'U' added as the first character and need an extra md5() (see the + // Drupal 7 documentation). + $stored_hash = substr($hash, 1); + $password = md5($password); + } + else { + $stored_hash = $hash; + } + + $type = substr($stored_hash, 0, 3); + switch ($type) { + case '$S$': + // A normal Drupal 7 password using sha512. + $computed_hash = $this->crypt('sha512', $password, $stored_hash); + break; + + case '$H$': + // phpBB3 uses "$H$" for the same thing as "$P$". + case '$P$': + // A phpass password generated using md5. This is an + // imported password or from an earlier Drupal version. + $computed_hash = $this->crypt('md5', $password, $stored_hash); + break; + + default: + return FALSE; + } + + // Compare using hash_equals() instead of === to mitigate timing attacks. + return $computed_hash && hash_equals($stored_hash, $computed_hash); + } + + /** + * {@inheritdoc} + */ + public function needsRehash(#[\SensitiveParameter] $hash) { + // Check whether this was an updated password. + if (!str_starts_with($hash, '$S$') || (strlen($hash) != static::HASH_LENGTH)) { + return TRUE; + } + // Ensure that $count_log2 is within set bounds. + $count_log2 = $this->enforceLog2Boundaries($this->countLog2); + // Check whether the iteration count used differs from the standard number. + return ($this->getCountLog2($hash) !== $count_log2); + } + +} ++======= + class PhpassHashedPassword extends PhpassHashedPasswordBase {} ++>>>>>>> 10.1.x diff --cc core/tests/Drupal/Tests/Core/File/FileSystemTest.php index ec8fa6e460,acfb1d6430..0000000000 --- a/core/tests/Drupal/Tests/Core/File/FileSystemTest.php +++ b/core/tests/Drupal/Tests/Core/File/FileSystemTest.php @@@ -144,18 -144,6 +144,21 @@@ class FileSystemTest extends UnitTestCa protected function assertFilePermissions(int $expected_mode, string $uri, string $message = ''): void { // Mask out all but the last three octets. $actual_mode = fileperms($uri) & 0777; ++<<<<<<< HEAD + + // PHP on Windows has limited support for file permissions. Usually each of + // "user", "group" and "other" use one octal digit (3 bits) to represent the + // read/write/execute bits. On Windows, chmod() ignores the "group" and + // "other" bits, and fileperms() returns the "user" bits in all three + // positions. $expected_mode is updated to reflect this. + if (str_starts_with(PHP_OS, 'WIN')) { + // Reset the "group" and "other" bits. + $expected_mode = $expected_mode & 0700; + // Shift the "user" bits to the "group" and "other" positions also. + $expected_mode = $expected_mode | $expected_mode >> 3 | $expected_mode >> 6; + } ++======= ++>>>>>>> 10.1.x $this->assertSame($expected_mode, $actual_mode, $message); }
For the first file, the issue that introduced this change in π Replace custom password hashing library with PHP password_hash() Fixed moved the existing
substr()
usages tocore/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php
. So, it makes sense to take the 10.1.x version of the file for the merge, then fix thesubstr()
calls in the new file.For the second, the issue is π Unneccessary bitwise operation for file permissions check on Windows Fixed . There, the relevant hunk was simply removed, not moved, so we can do the same in the MR.
Pushed fixes for the above.
- Status changed to Needs work
over 1 year ago 6:12pm 11 April 2023 - First commit to issue fork.
- π§π·Brazil murilohp
Just trying to move this forward, @xjm, I've done what you said on #12 and PHPStan keeps failing here, so I've updated the baseline, this helped a little, but still, some errors started happened:
9339/9339 [ββββββββββββββββββββββββββββ] 100% ------ ---------------------------------------------------------------------------------------------------------------------- Line core/lib/Drupal/Core/Password/PhpassHashedPassword.php ------ ---------------------------------------------------------------------------------------------------------------------- 16 Class Drupal\Core\Password\PhpassHashedPassword extends unknown class Drupal\Core\Password\PhpassHashedPasswordBase. π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols ------ ---------------------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------ Line core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php ------ ------------------------------------------------------------ 249 Syntax error, unexpected ';' on line 249 252 Syntax error, unexpected T_ELSE on line 252 281 Syntax error, unexpected '}', expecting EOF on line 281 ------ ------------------------------------------------------------ ------ ------------------------------------------------------------------------------------------------------------------------ Line core/modules/phpass/src/Password/PhpassHashedPassword.php ------ ------------------------------------------------------------------------------------------------------------------------ 12 Class Drupal\phpass\Password\PhpassHashedPassword extends unknown class Drupal\Core\Password\PhpassHashedPasswordBase. π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols ------ ------------------------------------------------------------------------------------------------------------------------
I don't get why these errors are being displayed, both files seems right to me. So I've just fixed the merge conflict and updated the baseline, but now I don't know how to fix the rest of the errors :(
Just for you to know, if you don't update the baseline, the following errors will be displayed:
------ ---------------------------------------------------------------------------------------------------------------------- Line core/lib/Drupal/Core/Password/PhpassHashedPassword.php ------ ---------------------------------------------------------------------------------------------------------------------- 16 Class Drupal\Core\Password\PhpassHashedPassword extends unknown class Drupal\Core\Password\PhpassHashedPasswordBase. π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols ------ ---------------------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------ Line core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php ------ ------------------------------------------------------------ 249 Syntax error, unexpected ';' on line 249 252 Syntax error, unexpected T_ELSE on line 252 281 Syntax error, unexpected '}', expecting EOF on line 281 ------ ------------------------------------------------------------ ------ ------------------------------------------------------------------------------------------------------------------------ Line core/modules/phpass/src/Password/PhpassHashedPassword.php ------ ------------------------------------------------------------------------------------------------------------------------ 12 Class Drupal\phpass\Password\PhpassHashedPassword extends unknown class Drupal\Core\Password\PhpassHashedPasswordBase. π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols ------ ------------------------------------------------------------------------------------------------------------------------ ------ -------------------------------------------------------------------------------------------------------------------------------- Line core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php ------ -------------------------------------------------------------------------------------------------------------------------------- 58 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 68 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 76 @covers value \Drupal\phpass\Password\PhpassHashedPassword::needsRehash references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::base64Encode references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::check references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::generateSalt references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::getCountLog2 references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::hash references an invalid method. 91 @covers value \Drupal\phpass\Password\PhpassHashedPassword::needsRehash references an invalid method. 92 Access to undefined constant Drupal\phpass\Password\PhpassHashedPassword::MIN_HASH_COUNT. 110 @covers value \Drupal\phpass\Password\PhpassHashedPassword::__construct references an invalid method. 110 @covers value \Drupal\phpass\Password\PhpassHashedPassword::check references an invalid method. 110 @covers value \Drupal\phpass\Password\PhpassHashedPassword::getCountLog2 references an invalid method. 110 @covers value \Drupal\phpass\Password\PhpassHashedPassword::hash references an invalid method. 110 @covers value \Drupal\phpass\Password\PhpassHashedPassword::needsRehash references an invalid method. 112 Access to undefined constant Drupal\phpass\Password\PhpassHashedPassword::MIN_HASH_COUNT. 112 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 116 Access to undefined constant Drupal\phpass\Password\PhpassHashedPassword::MIN_HASH_COUNT. ------ -------------------------------------------------------------------------------------------------------------------------------- ------ -------------------------------------------------------------------------------------------------------------------------------- Line core/modules/phpass/tests/src/Tests/PasswordVerifyTest.php ------ -------------------------------------------------------------------------------------------------------------------------------- 23 @covers value \Drupal\phpass\Password\PhpassHashedPassword::hash references an invalid method. 30 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 41 @covers value \Drupal\phpass\Password\PhpassHashedPassword::needsRehash references an invalid method. 47 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 57 @covers value \Drupal\phpass\Password\PhpassHashedPassword::check references an invalid method. 64 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 78 @covers value \Drupal\phpass\Password\PhpassHashedPassword::base64Encode references an invalid method. 78 @covers value \Drupal\phpass\Password\PhpassHashedPassword::check references an invalid method. 78 @covers value \Drupal\phpass\Password\PhpassHashedPassword::crypt references an invalid method. 78 @covers value \Drupal\phpass\Password\PhpassHashedPassword::enforceLog2Boundaries references an invalid method. 78 @covers value \Drupal\phpass\Password\PhpassHashedPassword::getCountLog2 references an invalid method. 91 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 109 @covers value \Drupal\phpass\Password\PhpassHashedPassword::enforceLog2Boundaries references an invalid method. 111 Access to undefined constant Drupal\phpass\Password\PhpassHashedPassword::MIN_HASH_COUNT. 112 Access to undefined constant Drupal\phpass\Password\PhpassHashedPassword::MAX_HASH_COUNT. 122 @covers value \Drupal\phpass\Password\PhpassHashedPassword::crypt references an invalid method. 127 Class Drupal\phpass\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. 179 Call to an undefined static method Drupal\phpass\Password\PhpassHashedPassword::enforceLog2Boundaries(). ------ -------------------------------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------------------------------------------------------ Line core/tests/Drupal/Tests/Core/Password/PasswordHashingLegacyTest.php ------ ------------------------------------------------------------------------------------------------------------------------------ 22 Class Drupal\Core\Password\PhpassHashedPassword does not have a constructor and must be instantiated without any parameters. ------ ------------------------------------------------------------------------------------------------------------------------------
- First commit to issue fork.
- Merge request !6042Resolve #3328456 "Replace substr($a, 0, $i) with str_starts_with()" β (Open) created by dimitriskr
- π¬π·Greece dimitriskr
Any tips on why is phpstan failing, that'd be nice
What is the purpose of this commit inside the MR? Isn't it irrelevant with this issue?
- π¬π·Greece dimitriskr
dimitriskr β changed the visibility of the branch 11.x to hidden.
- πΊπΈUnited States smustgrave
Is the branch currently up to date? This change shouldn't require adding to the phpstan-baseline.
- π¬π·Greece dimitriskr
Yes, the branch has been rebased to 11.x and MR 6042 is towards 11.x too
- π¬π·Greece dimitriskr
Reverting these changes would indeed fix the phpstan errors.
Shall we wait for someone else to agree with this or shall I go for it? - πΊπΈUnited States smustgrave
Say go for it. If we need to go back we can see in the commit history/get new artifacts from gitlab if needed.
- Status changed to Needs review
11 months ago 4:11pm 5 January 2024 - Status changed to RTBC
10 months ago 6:22pm 7 January 2024 - Open on Drupal.org βEnvironment: PHP 8.1 & MySQL 5.7last update
10 months ago Not currently mergeable. - Status changed to Fixed
10 months ago 10:51am 8 January 2024 - π¬π§United Kingdom catch
- // login attempt, bu this should be the less common case. + // login attempt, but this should be the less common case.
This is technically out of scope but I don't see any reason to open a new issue for the one character typo fix, assume it slipped in with another change to that file that then didn't make it through some rebasing or similar.
Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.
- π¬π·Greece dimitriskr
Can someone close the MR 6042 too? Since this issue is committed. Thank you