Remote Code Execution Vulnerability in Validation Placeholders

@@ -16,8 +16,10 @@
 use CodeIgniter\HTTP\RequestInterface;
 use CodeIgniter\Validation\Exceptions\ValidationException;
 use CodeIgniter\View\RendererInterface;
+use Config\Services;
 use Config\Validation as ValidationConfig;
 use InvalidArgumentException;
+use LogicException;
 use TypeError;
 
 /**
@@ -40,10 +42,18 @@ class Validation implements ValidationInterface
     protected $ruleSetInstances = [];
 
     /**
-     * Stores the actual rules that should
-     * be ran against $data.
+     * Stores the actual rules that should be run against $data.
      *
      * @var array
+     *
+     * [
+     *     field1 => [
+     *         'label' => label,
+     *         'rules' => [
+     *              rule1, rule2, ...
+     *          ],
+     *     ],
+     * ]
      */
     protected $rules = [];
 
@@ -133,8 +143,7 @@ public function run(?array $data = null, ?string $group = null, ?string $dbGroup
         // Run through each rule. If we have any field set for
         // this rule, then we need to run them through!
         foreach ($this->rules as $field => $setup) {
-            // Blast $rSetup apart, unless it's already an array.
-            $rules = $setup['rules'] ?? $setup;
+            $rules = $setup['rules'];
 
             if (is_string($rules)) {
                 $rules = $this->splitRules($rules);
@@ -483,6 +492,14 @@ public function setRules(array $rules, array $errors = []): ValidationInterface
                     $rule = ['rules' => $rule];
                 }
             }
+
+            if (isset($rule['rules']) && is_string($rule['rules'])) {
+                $rule['rules'] = $this->splitRules($rule['rules']);
+            }
+
+            if (is_string($rule)) {
+                $rule = ['rules' => $this->splitRules($rule)];
+            }
         }
 
         $this->rules = $rules;
@@ -645,47 +662,70 @@ public function loadRuleGroup(?string $group = null)
      *
      * and the following rule:
      *
-     *  'required|is_unique[users,email,id,{id}]'
+     *  'is_unique[users,email,id,{id}]'
      *
      * The value of {id} would be replaced with the actual id in the form data:
      *
-     *  'required|is_unique[users,email,id,13]'
+     *  'is_unique[users,email,id,13]'
      */
     protected function fillPlaceholders(array $rules, array $data): array
     {
-        $replacements = [];
+        foreach ($rules as &$rule) {
+            $ruleSet = $rule['rules'];
 
-        foreach ($data as $key => $value) {
-            $replacements["{{$key}}"] = $value;
-        }
+            foreach ($ruleSet as &$row) {
+                if (is_string($row)) {
+                    $placeholderFields = $this->retrievePlaceholders($row, $data);
 
-        if ($replacements !== []) {
-            foreach ($rules as &$rule) {
-                $ruleSet = $rule['rules'] ?? $rule;
+                    foreach ($placeholderFields as $field) {
+                        $validator ??= Services::validation(null, false);
 
-                if (is_array($ruleSet)) {
-                    foreach ($ruleSet as &$row) {
-                        if (is_string($row)) {
-                            $row = strtr($row, $replacements);
+                        $placeholderRules = $rules[$field]['rules'] ?? null;
+
+                        // Check if the validation rule for the placeholder exists
+                        if ($placeholderRules === null) {
+                            throw new LogicException(
+                                'No validation rules for the placeholder: ' . $field
+                            );
                         }
-                    }
-                }
 
-                if (is_string($ruleSet)) {
-                    $ruleSet = strtr($ruleSet, $replacements);
-                }
+                        // Check if the rule does not have placeholders
+                        foreach ($placeholderRules as $placeholderRule) {
+                            if ($this->retrievePlaceholders($placeholderRule, $data)) {
+                                throw new LogicException(
+                                    'The placeholder field cannot use placeholder: ' . $field
+                                );
+                            }
+                        }
 
-                if (isset($rule['rules'])) {
-                    $rule['rules'] = $ruleSet;
-                } else {
-                    $rule = $ruleSet;
+                        // Validate the placeholder field
+                        if (! $validator->check($data[$field], implode('|', $placeholderRules))) {
+                            // if fails, do nothing
+                            continue;
+                        }
+
+                        // Replace the placeholder in the rule
+                        $ruleSet = str_replace('{' . $field . '}', $data[$field], $ruleSet);
+                    }
                 }
             }
+
+            $rule['rules'] = $ruleSet;
         }
 
         return $rules;
     }
 
+    /**
+     * Retrieves valid placeholder fields.
+     */
+    private function retrievePlaceholders(string $rule, array $data): array
+    {
+        preg_match_all('/{(.+?)}/', $rule, $matches);
+
+        return array_intersect($matches[1], array_keys($data));
+    }
+
     /**
      * Checks to see if an error exists for the given field.
      */

@@ -121,7 +121,10 @@ public function testIsUniqueIgnoresParamsPlaceholders(): void
             'email' => 'derek@world.co.uk',
         ];
 
-        $this->validation->setRules(['email' => 'is_unique[user.email,id,{id}]']);
+        $this->validation->setRules([
+            'id'    => 'is_natural_no_zero',
+            'email' => 'is_unique[user.email,id,{id}]',
+        ]);
         $this->assertTrue($this->validation->run($data));
     }
 
@@ -221,7 +224,10 @@ public function testIsNotUniqueIgnoresParamsPlaceholders(): void
             'id'    => $row->id,
             'email' => 'derek@world.co.uk',
         ];
-        $this->validation->setRules(['email' => 'is_not_unique[user.email,id,{id}]']);
+        $this->validation->setRules([
+            'id'    => 'is_natural_no_zero',
+            'email' => 'is_not_unique[user.email,id,{id}]',
+        ]);
         $this->assertTrue($this->validation->run($data));
     }
 

@@ -86,7 +86,12 @@ public function testSetRulesStoresRules(): void
             'bar' => 'baz|belch',
         ];
         $this->validation->setRules($rules);
-        $this->assertSame($rules, $this->validation->getRules());
+
+        $expected = [
+            'foo' => ['rules' => ['bar', 'baz']],
+            'bar' => ['rules' => ['baz', 'belch']],
+        ];
+        $this->assertSame($expected, $this->validation->getRules());
     }
 
     public function testSetRuleStoresRule()
@@ -97,7 +102,7 @@ public function testSetRuleStoresRule()
         $this->assertSame([
             'foo' => [
                 'label' => null,
-                'rules' => 'bar|baz',
+                'rules' => ['bar', 'baz'],
             ],
         ], $this->validation->getRules());
     }
@@ -110,7 +115,7 @@ public function testSetRuleMultipleWithIndividual()
         $this->assertSame([
             'username' => [
                 'label' => 'Username',
-                'rules' => 'required|min_length[3]',
+                'rules' => ['required', 'min_length[3]'],
             ],
             'password' => [
                 'label' => 'Password',
@@ -136,11 +141,11 @@ public function testSetRuleAddsRule()
         $this->assertSame([
             'bar' => [
                 'label' => null,
-                'rules' => 'bar|baz',
+                'rules' => ['bar', 'baz'],
             ],
             'foo' => [
                 'label' => null,
-                'rules' => 'foo|foz',
+                'rules' => ['foo', 'foz'],
             ],
         ], $this->validation->getRules());
     }
@@ -158,7 +163,7 @@ public function testSetRuleOverwritesRule()
         $this->assertSame([
             'foo' => [
                 'label' => null,
-                'rules' => 'foo|foz',
+                'rules' => ['foo', 'foz'],
             ],
         ], $this->validation->getRules());
     }
@@ -176,7 +181,7 @@ public function testSetRuleOverwritesRuleReverse()
         $this->assertSame([
             'foo' => [
                 'label' => null,
-                'rules' => 'bar|baz',
+                'rules' => ['bar', 'baz'],
             ],
         ], $this->validation->getRules());
     }
@@ -549,7 +554,7 @@ public function testSetRuleGroup(): void
     {
         $this->validation->setRuleGroup('groupA');
         $this->assertSame([
-            'foo' => 'required|min_length[5]',
+            'foo' => ['rules' => ['required', 'min_length[5]']],
         ], $this->validation->getRules());
     }
 
@@ -1386,7 +1391,7 @@ public function provideStringRulesCases(): iterable
     protected function placeholderReplacementResultDetermination(string $placeholder = 'id', ?array $data = null)
     {
         if ($data === null) {
-            $data = [$placeholder => 'placeholder-value'];
+            $data = [$placeholder => '12'];
         }
 
         $validationRules = $this->getPrivateMethodInvoker($this->validation, 'fillPlaceholders')($this->validation->getRules(), $data);
@@ -1421,13 +1426,15 @@ public function testPlaceholderReplacementTestFails()
 
     public function testPlaceholderReplacementSetSingleRuleString()
     {
+        $this->validation->setRule('id', null, 'required|is_natural_no_zero');
         $this->validation->setRule('foo', null, 'required|filter[{id}]');
 
         $this->placeholderReplacementResultDetermination();
     }
 
     public function testPlaceholderReplacementSetSingleRuleArray()
     {
+        $this->validation->setRule('id', null, ['required', 'is_natural_no_zero']);
         $this->validation->setRule('foo', null, ['required', 'filter[{id}]']);
 
         $this->placeholderReplacementResultDetermination();
@@ -1436,6 +1443,7 @@ public function testPlaceholderReplacementSetSingleRuleArray()
     public function testPlaceholderReplacementSetMultipleRulesSimpleString()
     {
         $this->validation->setRules([
+            'id'  => 'required|is_natural_no_zero',
             'foo' => 'required|filter[{id}]',
         ]);
 
@@ -1445,6 +1453,7 @@ public function testPlaceholderReplacementSetMultipleRulesSimpleString()
     public function testPlaceholderReplacementSetMultipleRulesSimpleArray()
     {
         $this->validation->setRules([
+            'id'  => ['required', 'is_natural_no_zero'],
             'foo' => ['required', 'filter[{id}]'],
         ]);
 
@@ -1454,6 +1463,9 @@ public function testPlaceholderReplacementSetMultipleRulesSimpleArray()
     public function testPlaceholderReplacementSetMultipleRulesComplexString()
     {
         $this->validation->setRules([
+            'id' => [
+                'rules' => 'required|is_natural_no_zero',
+            ],
             'foo' => [
                 'rules' => 'required|filter[{id}]',
             ],
@@ -1465,6 +1477,9 @@ public function testPlaceholderReplacementSetMultipleRulesComplexString()
     public function testPlaceholderReplacementSetMultipleRulesComplexArray()
     {
         $this->validation->setRules([
+            'id' => [
+                'rules' => ['required', 'is_natural_no_zero'],
+            ],
             'foo' => [
                 'rules' => ['required', 'filter[{id}]'],
             ],

@@ -12,6 +12,9 @@ Release Date: Unreleased
 SECURITY
 ********
 
+- *Remote Code Execution Vulnerability in Validation Placeholders* was fixed.
+  See the `Security advisory GHSA-m6m8-6gq8-c9fj <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-m6m8-6gq8-c9fj>`_
+  for more information.
 - Fixed that ``Session::stop()`` did not destroy the session.
   See :ref:`Session Library <session-stop>` for details.
 

@@ -18,6 +18,12 @@ Mandatory File Changes
 Breaking Changes
 ****************
 
+Validation Placeholders
+=======================
+
+- To use :ref:`validation-placeholders` securely, please remember to create a validation rule for the field you will use as a placeholder.
+  
+
 Session::stop()
 ===============
 

@@ -1,5 +1,6 @@
 <?php
 
 $validation->setRules([
+    'id'    => 'is_natural_no_zero',
     'email' => 'required|valid_email|is_unique[users.email,id,{id}]',
 ]);

@@ -1,5 +1,6 @@
 <?php
 
 $validation->setRules([
+    'id'    => 'is_natural_no_zero',
     'email' => 'required|valid_email|is_unique[users.email,id,4]',
 ]);

@@ -436,6 +436,8 @@ This method sets a rule group from the validation configuration to the validatio
 
 .. literalinclude:: validation/018.php
 
+.. _validation-placeholders:
+
 Validation Placeholders
 =======================
 
@@ -446,6 +448,9 @@ replaced by the **value** of the matched incoming field. An example should clari
 
 .. literalinclude:: validation/020.php
 
+.. note:: Since v4.3.5, you must set the validation rules for the placeholder
+    field (``id``).
+
 In this set of rules, it states that the email address should be unique in the database, except for the row
 that has an id matching the placeholder's value. Assuming that the form POST data had the following:
 
@@ -457,6 +462,9 @@ then the ``{id}`` placeholder would be replaced with the number **4**, giving th
 
 So it will ignore the row in the database that has ``id=4`` when it verifies the email is unique.
 
+.. note:: Since v4.3.5, if the placeholder (``id``) value does not pass the
+    validation, the placeholder would not be replaced.
+
 This can also be used to create more dynamic rules at runtime, as long as you take care that any dynamic
 keys passed in don't conflict with your form data.