Merge pull request from GHSA-c5vj-f36q-p9vg

* feat: copy the current methods as depreacted * fix: password shucking * feat: support old dangerous passwords * docs: add Upgrade Guide * refactor: fix variable name typo Co-authored-by: MGatner <[email protected]> * refactor: fix variable name typo * fix: $options is missing for password_needs_rehash() * docs: add OWASP Cheat Sheet in README * docs: add explanation in UPGRADING.md * docs: replace Config/Auth.php with app/Config/Auth.php * docs: add How to Strengthen the Password * docs: UPGRADING.md * docs: add note in "Minimum Password Length" * docs: $supportOldDangerousPassword will be removed in v1.0.0 * feat: add validation rule for max password length PASSWORD_BCRYPT -> 72 bytes Others -> 255 characters * docs: update docs * feat: add new lang item to new lang files * feat: add errorPasswordTooLongBytes to Language/pt-BR * refactor: ChangeIfElseValueAssignToEarlyReturnRector --------- Co-authored-by: MGatner <[email protected]>
codeigniter4 · Mar 11, 2023 · ea9688d · ea9688d
1 parent 247c53f
commit ea9688d
Show file tree

Hide file tree

Showing 26 changed files with 435 additions and 17 deletions.
diff --git a/README.md b/README.md
@@ -86,3 +86,4 @@ within this library, in no particular order:
 - [Google Cloud: 13 best practices for user account, authentication, and password management, 2021 edition](https://cloud.google.com/blog/products/identity-security/account-authentication-and-password-management-best-practices)
 - [NIST Digital Identity Guidelines](https://pages.nist.gov/800-63-3/sp800-63b.html)
 - [Implementing Secure User Authentication in PHP Applications with Long-Term Persistence (Login with "Remember Me" Cookies) ](https://paragonie.com/blog/2015/04/secure-authentication-php-with-long-term-persistence)
+- [Password Storage - OWASP Cheat Sheet Series](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html)
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -0,0 +1,51 @@
+# Upgrade Guide
+
+## Version 1.0.0-beta.3 to 1.0.0-beta.4
+
+### Important Password Changes
+
+#### Password Incompatibility
+
+Shield 1.0.0-beta.4 fixes a [vulnerability related to password storage](https://github.com/codeigniter4/shield/security/advisories/GHSA-c5vj-f36q-p9vg).
+As a result, hashed passwords already stored in the database are no longer compatible
+and cannot be used by default.
+
+All hashed passwords stored in Shield v1.0.0-beta.3 or earlier are easier to
+crack than expected due to the above vulnerability. Therefore, they should be
+removed as soon as possible.
+
+Existing users will no longer be able to log in with their passwords and will
+need to log in with the magic link and then set their passwords again.
+
+#### If You Want to Allow Login with Existing Passwords
+
+If you want to use passwords saved in Shield v1.0.0-beta.3 or earlier,
+you must add the following property in `app/Config/Auth.php`:
+
+```php
+    public bool $supportOldDangerousPassword = true;
+```
+
+After upgrading, with the above setting, once a user logs in with the password,
+the hashed password is updated and stored in the database.
+
+In this case, the existing hashed passwords are still easier to crack than expected.
+Therefore, this setting should not be used for an extended period of time.
+So you should change the setting to `false` as soon as possible, and remove old
+hashed password.
+
+> **Note**
+>
+> This setting is deprecated. It will be removed in v1.0.0 official release.
+
+#### Limitations for the Default Password Handling
+
+By default, Shield uses the hashing algorithm `PASSWORD_DEFAULT` (see `app/Config/Auth.php`),
+that is, `PASSWORD_BCRYPT` at the time of writing.
+
+Now there are two limitations when you use `PASSWORD_BCRYPT`.
+
+1. the password will be truncated to a maximum length of 72 bytes.
+2. the password will be truncated at the first NULL byte (`\0`).
+
+If these behaviors are unacceptable, see [How to Strengthen the Password](https://github.com/codeigniter4/shield/blob/develop/docs/guides/strengthen_password.md).
diff --git a/docs/guides/strengthen_password.md b/docs/guides/strengthen_password.md
@@ -0,0 +1,122 @@
+# How to Strengthen the Password
+
+Shield allows you to customize password-related settings to make your passwords more secure.
+
+## Minimum Password Length
+
+The most important factor when it comes to passwords is the number of characters in the password.
+You can check password strength with [Password Strength Testing Tool](https://bitwarden.com/password-strength/).
+Short passwords may be cracked in less than one day.
+
+In Shield, you can set the users' minimum password length. The setting is
+`$minimumPasswordLength` in `app/Config/Auth.php`. The default value is 8 characters.
+It is the recommended minimum value by NIST. However, some organizations recommend
+12 to 14 characters.
+
+The longer the password, the stronger it is. Consider increasing the value.
+
+> **Note**
+>
+> This checking works when you validate passwords with the `strong_password`
+> validation rule.
+>
+> If you disable `CompositionValidator` (enabled by default) in `$passwordValidators`,
+> this checking will not work.
+
+## Password Hashing Algorithm
+
+You can change the password hashing algorithm by `$hashAlgorithm` in `app/Config/Auth.php`.
+The default value is `PASSWORD_DEFAULT` that is `PASSWORD_BCRYPT` at the time of writing.
+
+`PASSWORD_BCRYPT` means to create new password hashes using the bcrypt algorithm.
+
+You can use `PASSWORD_ARGON2ID` if your PHP has been compiled with Argon2 support.
+
+### PASSWORD_BCRYPT
+
+`PASSWORD_BCRYPT` has one configuration `$hashCost`. The bigger the cost, hashed passwords will be the stronger.
+
+You can find your appropriate cost with the following code:
+
+```php
+<?php
+/**
+ * This code will benchmark your server to determine how high of a cost you can
+ * afford. You want to set the highest cost that you can without slowing down
+ * you server too much. 8-10 is a good baseline, and more is good if your servers
+ * are fast enough. The code below aims for ≤ 50 milliseconds stretching time,
+ * which is a good baseline for systems handling interactive logins.
+ *
+ * From: https://www.php.net/manual/en/function.password-hash.php#refsect1-function.password-hash-examples
+ */
+$timeTarget = 0.05; // 50 milliseconds
+
+$cost = 8;
+do {
+    $cost++;
+    $start = microtime(true);
+    password_hash("test", PASSWORD_BCRYPT, ["cost" => $cost]);
+    $end = microtime(true);
+} while (($end - $start) < $timeTarget);
+
+echo "Appropriate Cost Found: " . $cost;
+```
+
+#### Limitations
+
+There are two limitations when you use `PASSWORD_BCRYPT`:
+
+1. the password will be truncated to a maximum length of 72 bytes.
+2. the password will be truncated at the first NULL byte (`\0`).
+
+##### 72 byte issue
+
+If a user submits a password longer than 72 bytes, the validation error will occur.
+If this behavior is unacceptable, consider:
+
+1. change the hashing algorithm to `PASSWORD_ARGON2ID`. It does not have such a limitation.
+
+##### NULL byte issue
+
+This is because `PASSWORD_BCRYPT` is not binary-safe. Normal users cannot
+send NULL bytes in a password string, so this is not a problem in most cases.
+
+But if this behavior is unacceptable, consider:
+
+1. adding a validation rule to prohibit NULL bytes or control codes.
+2. or change the hashing algorithm to `PASSWORD_ARGON2ID`. It is binary-safe.
+
+### PASSWORD_ARGON2ID
+
+`PASSWORD_ARGON2ID` has three configuration `$hashMemoryCost`, `$hashTimeCost`,
+and `$hashThreads`.
+
+If you use `PASSWORD_ARGON2ID`, you should use PHP's constants:
+
+```php
+    public int $hashMemoryCost = PASSWORD_ARGON2_DEFAULT_MEMORY_COST;
+
+    public int $hashTimeCost = PASSWORD_ARGON2_DEFAULT_TIME_COST;
+    public int $hashThreads  = PASSWORD_ARGON2_DEFAULT_THREADS;
+```
+
+## Maximum Password Length
+
+By default, Shield has the validation rules for maximum password length.
+
+- 72 bytes for PASSWORD_BCRYPT
+- 255 characters for others
+
+You can customize the validation rule. See [Customizing Shield](../customization.md).
+
+## $supportOldDangerousPassword
+
+In `app/Config/Auth.php` there is `$supportOldDangerousPassword`, which is a
+setting for using passwords stored in older versions of Shield that were [vulnerable](https://github.com/codeigniter4/shield/security/advisories/GHSA-c5vj-f36q-p9vg).
+
+This setting is deprecated.  If you have this setting set to `true`, you should change
+it to `false` as soon as possible, and remove old hashed password in your database.
+
+> **Note**
+>
+> This setting will be removed in v1.0.0 official release.
diff --git a/docs/index.md b/docs/index.md
@@ -14,3 +14,4 @@
 ## Guides
 * [Protecting an API with Access Tokens](guides/api_tokens.md)
 * [Mobile Authentication with Access Tokens](guides/mobile_apps.md)
+* [How to Strengthen the Password](guides/strengthen_password.md)
diff --git a/mkdocs.yml b/mkdocs.yml
@@ -51,3 +51,4 @@ nav:
   - Guides:
     - guides/api_tokens.md
     - guides/mobile_apps.md
+    - guides/strengthen_password.md
diff --git a/src/Authentication/Authenticators/Session.php b/src/Authentication/Authenticators/Session.php
@@ -336,19 +336,30 @@ public function check(array $credentials): Result
         /** @var Passwords $passwords */
         $passwords = service('passwords');
 
+        // This is only for supportOldDangerousPassword.
+        $needsRehash = false;
+
         // Now, try matching the passwords.
         if (! $passwords->verify($givenPassword, $user->password_hash)) {
-            return new Result([
-                'success' => false,
-                'reason'  => lang('Auth.invalidPassword'),
-            ]);
+            if (
+                ! setting('Auth.supportOldDangerousPassword')
+                || ! $passwords->verifyDanger($givenPassword, $user->password_hash) // @phpstan-ignore-line
+            ) {
+                return new Result([
+                    'success' => false,
+                    'reason'  => lang('Auth.invalidPassword'),
+                ]);
+            }
+
+            // Passed with old dangerous password.
+            $needsRehash = true;
         }
 
         // Check to see if the password needs to be rehashed.
         // This would be due to the hash algorithm or hash
         // cost changing since the last time that a user
         // logged in.
-        if ($passwords->needsRehash($user->password_hash)) {
+        if ($passwords->needsRehash($user->password_hash) || $needsRehash) {
             $user->password_hash = $passwords->hash($givenPassword);
             $this->provider->save($user);
         }

diff --git a/src/Authentication/Passwords.php b/src/Authentication/Passwords.php
@@ -31,26 +31,42 @@ public function __construct(Auth $config)
      */
     public function hash(string $password)
     {
-        if ((defined('PASSWORD_ARGON2I') && $this->config->hashAlgorithm === PASSWORD_ARGON2I)
+        return password_hash($password, $this->config->hashAlgorithm, $this->getHashOptions());
+    }
+
+    private function getHashOptions(): array
+    {
+        if (
+            (defined('PASSWORD_ARGON2I') && $this->config->hashAlgorithm === PASSWORD_ARGON2I)
             || (defined('PASSWORD_ARGON2ID') && $this->config->hashAlgorithm === PASSWORD_ARGON2ID)
         ) {
-            $hashOptions = [
+            return [
                 'memory_cost' => $this->config->hashMemoryCost,
                 'time_cost'   => $this->config->hashTimeCost,
                 'threads'     => $this->config->hashThreads,
             ];
-        } else {
-            $hashOptions = [
-                'cost' => $this->config->hashCost,
-            ];
         }
 
+        return [
+            'cost' => $this->config->hashCost,
+        ];
+    }
+
+    /**
+     * Hash a password.
+     *
+     * @return false|string|null
+     *
+     * @deprecated This is only for backward compatibility.
+     */
+    public function hashDanger(string $password)
+    {
         return password_hash(
             base64_encode(
                 hash('sha384', $password, true)
             ),
             $this->config->hashAlgorithm,
-            $hashOptions
+            $this->getHashOptions()
         );
     }
 
@@ -61,6 +77,19 @@ public function hash(string $password)
      * @param string $hash     The previously hashed password
      */
     public function verify(string $password, string $hash): bool
+    {
+        return password_verify($password, $hash);
+    }
+
+    /**
+     * Verifies a password against a previously hashed password.
+     *
+     * @param string $password The password we're checking
+     * @param string $hash     The previously hashed password
+     *
+     * @deprecated This is only for backward compatibility.
+     */
+    public function verifyDanger(string $password, string $hash): bool
     {
         return password_verify(base64_encode(
             hash('sha384', $password, true)
@@ -72,7 +101,7 @@ public function verify(string $password, string $hash): bool
      */
     public function needsRehash(string $hashedPassword): bool
     {
-        return password_needs_rehash($hashedPassword, $this->config->hashAlgorithm);
+        return password_needs_rehash($hashedPassword, $this->config->hashAlgorithm, $this->getHashOptions());
     }
 
     /**
@@ -110,4 +139,16 @@ public function check(string $password, ?User $user = null): Result
             'success' => true,
         ]);
     }
+
+    /**
+     * Returns the validation rule for max length.
+     */
+    public static function getMaxLenghtRule(): string
+    {
+        if (config('Auth')->hashAlgorithm === PASSWORD_BCRYPT) {
+            return 'max_byte[72]';
+        }
+
+        return 'max_length[255]';
+    }
 }
diff --git a/src/Authentication/Passwords/ValidationRules.php b/src/Authentication/Passwords/ValidationRules.php
@@ -55,6 +55,14 @@ public function strong_password(string $value, ?string &$error1 = null, array $d
         return $result->isOk();
     }
 
+    /**
+     * Returns true if $str is $val or fewer bytes in length.
+     */
+    public function max_byte(?string $str, string $val): bool
+    {
+        return is_numeric($val) && $val >= strlen($str ?? '');
+    }
+
     /**
      * Builds a new user instance from the global request.
      */

diff --git a/src/Config/Auth.php b/src/Config/Auth.php
@@ -360,6 +360,16 @@ class Auth extends BaseConfig
      */
     public int $hashCost = 10;
 
+    /**
+     * If you need to support passwords saved in versions prior to Shield v1.0.0-beta.4.
+     * set this to true.
+     *
+     * See https://github.com/codeigniter4/shield/security/advisories/GHSA-c5vj-f36q-p9vg
+     *
+     * @deprecated This is only for backward compatibility.
+     */
+    public bool $supportOldDangerousPassword = false;
+
     /**
      * ////////////////////////////////////////////////////////////////////
      * OTHER SETTINGS

diff --git a/src/Controllers/LoginController.php b/src/Controllers/LoginController.php
@@ -7,6 +7,7 @@
 use App\Controllers\BaseController;
 use CodeIgniter\HTTP\RedirectResponse;
 use CodeIgniter\Shield\Authentication\Authenticators\Session;
+use CodeIgniter\Shield\Authentication\Passwords;
 use CodeIgniter\Shield\Traits\Viewable;
 
 class LoginController extends BaseController
@@ -90,8 +91,11 @@ protected function getValidationRules(): array
                 'rules' => config('AuthSession')->emailValidationRules,
             ],
             'password' => [
-                'label' => 'Auth.password',
-                'rules' => 'required',
+                'label'  => 'Auth.password',
+                'rules'  => 'required|' . Passwords::getMaxLenghtRule(),
+                'errors' => [
+                    'max_byte' => 'Auth.errorPasswordTooLongBytes',
+                ],
             ],
         ];
     }

diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php
@@ -10,6 +10,7 @@
 use CodeIgniter\HTTP\RequestInterface;
 use CodeIgniter\HTTP\ResponseInterface;
 use CodeIgniter\Shield\Authentication\Authenticators\Session;
+use CodeIgniter\Shield\Authentication\Passwords;
 use CodeIgniter\Shield\Config\Auth;
 use CodeIgniter\Shield\Entities\User;
 use CodeIgniter\Shield\Exceptions\ValidationException;
@@ -195,8 +196,11 @@ protected function getValidationRules(): array
                 'rules' => $registrationEmailRules,
             ],
             'password' => [
-                'label' => 'Auth.password',
-                'rules' => 'required|strong_password',
+                'label'  => 'Auth.password',
+                'rules'  => 'required|' . Passwords::getMaxLenghtRule() . '|strong_password',
+                'errors' => [
+                    'max_byte' => 'Auth.errorPasswordTooLongBytes',
+                ],
             ],
             'password_confirm' => [
                 'label' => 'Auth.passwordConfirm',