CVE-2022-2821: Missing Critical Step in Authentication (CWE-304)
value: Missing Critical Step in Authentication in GitHub repository namelessmc/nameless prior to v2.0.2.
CWE description: The product implements an authentication technique, but it skips a step that weakens the technique.
CVSS v3 base score: 7.5
Published: 2022-08-15T11:21Z
Repository: https://github.com/namelessmc/nameless
Language: PHP
Files changed: core/classes/Core/Validate.php, custom/languages/en_UK.json, modules/Core/pages/login.php
Commit date: 2022-08-07 13:32:07 -0600
Commit message: Add ability to rate limit via Validate class (#2998)


Diff: diff --git a/core/classes/Core/Validate.php b/core/classes/Core/Validate.php
index b7e3f57fa..fdb5679bf 100644
--- a/core/classes/Core/Validate.php
+++ b/core/classes/Core/Validate.php
@@ -79,10 +79,15 @@ class Validate {
     /**
      * @var string Check that the value does not start with a pattern
      */
     public const NOT_START_WITH = 'not_start_with';
 
+    /**
+     * @var string Set a rate limit
+     */
+    public const RATE_LIMIT = 'rate_limit';
+
     private DB $_db;
 
     private ?string $_message = null;
     private array $_messages = [];
     private bool $_passed = false;
@@ -110,10 +115,11 @@ class Validate {
      *
      * @param array $source inputs (eg: $_POST)
      * @param array $items subset of inputs to be validated
      *
      * @return Validate New instance of Validate.
+     * @throws Exception If provided configuration for a rule is invalid - not if a provided value is invalid!
      */
     public static function check(array $source, array $items = []): Validate {
         $validator = new Validate();
 
         // Loop through the items which need validating
@@ -316,10 +322,55 @@ class Validate {
                                 ]);
                             }
                             break;
                         }
                         break;
+
+                    case self::RATE_LIMIT:
+                        if (is_array($rule_value) && count($rule_value) === 2) {
+                            // If array treat as [limit, seconds]
+                            [$limit, $seconds] = $rule_value;
+                        } else if (is_int($rule_value)) {
+                            // If integer default seconds to 60
+                            [$limit, $seconds] = [$rule_value, 60];
+                        }
+
+                        if (!isset($limit) || !isset($seconds)) {
+                            throw new Exception('Invalid rate limit configuration');
+                        }
+
+                        $key = "rate_limit_{$item}";
+                        $session = $_SESSION[$key];
+                        $time = date('U');
+                        $limit_end = $time + $seconds;
+
+                        if (isset($session) && is_array($session) && count($session) === 2) {
+                            [$count, $expires] = $session;
+                            $diff = $expires - $time;
+
+                            if (++$count >= $limit && $diff > 0) {
+                                $validator->addError([
+                                    'field' => $item,
+                                    'rule' => self::RATE_LIMIT,
+                                    'fallback' => "$item has reached the rate limit which expires in $diff seconds.",
+                                    'meta' => ['expires' => $diff],
+                                ]);
+                                break;
+                            }
+
+                            if ($diff <= 0) {
+                                // Reset
+                                $_SESSION[$key] = [1, $limit_end];
+                                break;
+                            }
+
+                            $_SESSION[$key] = [$count, $expires];
+                        } else {
+                            $_SESSION[$key] = [1, $limit_end];
+                        }
+
+                        break;
                 }
             }
         }
 
         if (empty($validator->_to_convert)) {
@@ -377,11 +428,11 @@ class Validate {
         }
 
         // Loop all errors to convert and get their custom messages
         foreach ($this->_to_convert as $error) {
 
-            $message = $this->getMessage($error['field'], $error['rule'], $error['fallback']);
+            $message = $this->getMessage($error['field'], $error['rule'], $error['fallback'], $error['meta']);
 
             // If there is no generic `message()` set or the translated message is not equal to generic message
             // we can continue without worrying about duplications
             if ($this->_message === null || ($message != $this->_message && !in_array($message, $this->_errors))) {
                 $this->_errors[] = $message;
@@ -407,14 +458,15 @@ class Validate {
      *  - Fallback message for rule
      *
      * @param string $field name of field to search for.
      * @param string $rule rule which check failed. should be from the constants defined above.
      * @param string $fallback fallback default message if custom message and generic message are not supplied.
+     * @param ?array $meta optional meta to provide to message.
      *
      * @return string Message for this field and rule.
      */
-    private function getMessage(string $field, string $rule, string $fallback): string {
+    private function getMessage(string $field, string $rule, string $fallback, ?array $meta = []): string {
 
         // No custom messages defined for this field
         if (!isset($this->_messages[$field])) {
             if (isset($this->_messages['*'])) {
                 $message = $this->_messages['*']($field);
@@ -434,10 +486,15 @@ class Validate {
         // Array of custom messages supplied, but none of their rules matches this rule
         if (!array_key_exists($rule, $this->_messages[$field])) {
             return $this->_message ?? $fallback;
         }
 
+        // If the message is a callback function, provide it with meta
+        if (is_callable($this->_messages[$field][$rule])) {
+            return $this->_messages[$field][$rule]($meta);
+        }
+
         // Rule-specific custom message was supplied
         return $this->_messages[$field][$rule];
     }
 
     /**
diff --git a/custom/languages/en_UK.json b/custom/languages/en_UK.json
index de726824e..29b69f1db 100644
--- a/custom/languages/en_UK.json
+++ b/custom/languages/en_UK.json
@@ -796,10 +796,11 @@
     "general/page_x": "Page {{page}}",
     "general/player_count": "{{onlinePlayers}}\/{{totalPlayers}}",
     "general/previous": "Previous",
     "general/privacy_policy": "Privacy Policy",
     "general/profile": "Profile",
+    "general/rate_limit": "Please try again in {{expires}} seconds",
     "general/register": "Register",
     "general/remove": "Remove",
     "general/report": "Report",
     "general/report_origin": "Origin",
     "general/reported_by": "Reported by {{author}}",
diff --git a/modules/Core/pages/login.php b/modules/Core/pages/login.php
index 08e5af7ea..bc452ae37 100644
--- a/modules/Core/pages/login.php
+++ b/modules/Core/pages/login.php
@@ -52,27 +52,31 @@ if (Input::exists()) {
                 $_POST['password'] = $_SESSION['password'];
 
                 unset($_SESSION['remember'], $_SESSION['password'], $_SESSION['tfa']);
             }
 
+            $rate_limit = [5, 60]; // 5 attempts in 60 seconds - TODO allow this to be customised?
+
             if ($login_method == 'email') {
                 $to_validate = [
                     'email' => [
                         Validate::REQUIRED => true,
                         Validate::IS_BANNED => true,
-                        Validate::IS_ACTIVE => true
+                        Validate::IS_ACTIVE => true,
+                        Validate::RATE_LIMIT => $rate_limit,
                     ],
                     'password' => [
                         Validate::REQUIRED => true
                     ]
                 ];
             } else {
                 $to_validate = [
                     'username' => [
                         Validate::REQUIRED => true,
                         Validate::IS_BANNED => true,
-                        Validate::IS_ACTIVE => true
+                        Validate::IS_ACTIVE => true,
+                        Validate::RATE_LIMIT => $rate_limit,
                     ],
                     'password' => [
                         Validate::REQUIRED => true
                     ]
                 ];
@@ -80,16 +84,18 @@ if (Input::exists()) {
 
             $validation = Validate::check($_POST, $to_validate)->messages([
                 'email' => [
                     Validate::REQUIRED => $language->get('user', 'must_input_email'),
                     Validate::IS_BANNED => $language->get('user', 'account_banned'),
-                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account')
+                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account'),
+                    Validate::RATE_LIMIT => fn($meta) => $language->get('general', 'rate_limit', $meta),
                 ],
                 'username' => [
                     Validate::REQUIRED => ($login_method == 'username' ? $language->get('user', 'must_input_username') : $language->get('user', 'must_input_email_or_username')),
                     Validate::IS_BANNED => $language->get('user', 'account_banned'),
-                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account')
+                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account'),
+                    Validate::RATE_LIMIT => fn($meta) => $language->get('general', 'rate_limit', $meta),
                 ],
                 'password' => $language->get('user', 'must_input_password')
             ]);
 
             // Check if validation passed

Vulnerable code:             $message = $this->getMessage($error['field'], $error['rule'], $error['fallback']);
    private function getMessage(string $field, string $rule, string $fallback): string {
                        Validate::IS_ACTIVE => true
                        Validate::IS_ACTIVE => true
                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account')
                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account')
Fixed code:     /**
     * @var string Set a rate limit
     */
    public const RATE_LIMIT = 'rate_limit';

     * @throws Exception If provided configuration for a rule is invalid - not if a provided value is invalid!

                    case self::RATE_LIMIT:
                        if (is_array($rule_value) && count($rule_value) === 2) {
                            // If array treat as [limit, seconds]
                            [$limit, $seconds] = $rule_value;
                        } else if (is_int($rule_value)) {
                            // If integer default seconds to 60
                            [$limit, $seconds] = [$rule_value, 60];
                        }

                        if (!isset($limit) || !isset($seconds)) {
                            throw new Exception('Invalid rate limit configuration');
                        }

                        $key = "rate_limit_{$item}";
                        $session = $_SESSION[$key];
                        $time = date('U');
                        $limit_end = $time + $seconds;

                        if (isset($session) && is_array($session) && count($session) === 2) {
                            [$count, $expires] = $session;
                            $diff = $expires - $time;

                            if (++$count >= $limit && $diff > 0) {
                                $validator->addError([
                                    'field' => $item,
                                    'rule' => self::RATE_LIMIT,
                                    'fallback' => "$item has reached the rate limit which expires in $diff seconds.",
                                    'meta' => ['expires' => $diff],
                                ]);
                                break;
                            }

                            if ($diff <= 0) {
                                // Reset
                                $_SESSION[$key] = [1, $limit_end];
                                break;
                            }

                            $_SESSION[$key] = [$count, $expires];
                        } else {
                            $_SESSION[$key] = [1, $limit_end];
                        }

                        break;
            $message = $this->getMessage($error['field'], $error['rule'], $error['fallback'], $error['meta']);
     * @param ?array $meta optional meta to provide to message.
    private function getMessage(string $field, string $rule, string $fallback, ?array $meta = []): string {
        // If the message is a callback function, provide it with meta
        if (is_callable($this->_messages[$field][$rule])) {
            return $this->_messages[$field][$rule]($meta);
        }

    "general/rate_limit": "Please try again in {{expires}} seconds",
            $rate_limit = [5, 60]; // 5 attempts in 60 seconds - TODO allow this to be customised?

                        Validate::IS_ACTIVE => true,
                        Validate::RATE_LIMIT => $rate_limit,
                        Validate::IS_ACTIVE => true,
                        Validate::RATE_LIMIT => $rate_limit,
                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account'),
                    Validate::RATE_LIMIT => fn($meta) => $language->get('general', 'rate_limit', $meta),
                    Validate::IS_ACTIVE => $language->get('user', 'inactive_account'),
                    Validate::RATE_LIMIT => fn($meta) => $language->get('general', 'rate_limit', $meta),